3
\$\begingroup\$

I'm using nodegit to develop the backend for a nodejs app that uses a mix of mongodb and git for persistence. The app has to version documents and be able to present diffs between versions, thus the use of git in the backend.

For one of the API endpoints, I needed to get the diff between two commits, so I've develop the express middleware getDiffBetweenCommits detailed below.

AFAIK, Nodegit does not provide this natively. It just provides with commit.getDiff() which only compares a commit to its parent, but I needed to compare to arbitrary commits.

Git model is as follows:

commit -> patches -> hunks -> modified lines

I need to traverse this tree, gather all the modified lines for all the hunks in all the patches, and return.

The thing is, all the calls that go deeper into this tree (ie. commit.patches(), patch.hunks(), hunks.lines(), are all async (Promises), so instead of simply doing a forEach at each level, I had to recursively visit the patches tree.

I created a context object to help me iterate it, maintain cursors and stack state, and call expressJS next() function to pass to the next middleware when all the tree has been traversed and there are no more patches to iterate.

exports.getDiffBetweenCommits = function (req, res, next) {
  var commitID1 = req.params.stageID1;
  var commitID2 = req.params.stageID2;

  var repo;

  Git.Repository.open('/path/to/git/repo')
    .then(function (repository) {
      repo = repository;
      return repo.getCommit(commitID1);
    })
    .then(function (firstCommit) {
      req.commit1 = firstCommit;
      return repo.getCommit(commitID2);
    })
    .then(function (commit2) {
      req.commit2 = commit2;
      req.gitOptions = null;
      return;
    })
    .then(function () {
      return req.commit1.getTree();
    })
    .then(function (commitTree) {
      req.commit1Tree = commitTree;
      return req.commit2.getTree();
    })
    .then(function (commitTree) {
      req.commit2Tree = commitTree;
      return;
    })
    .then(function () {
      return req.commit1Tree.diff(req.commit2Tree);
    })
    .then(function (diffs) {
      var diffResult = [];
      req.diffResult = diffResult;

      var context = {
        next: next,
        diffResult: diffResult,
        modificationCount: 0,
        pushPatches: function (patches) {
          this.patches = patches;
          this.currentPatch = -1;
        },
        nextPatch: function () {
          this.currentPatch++;
          if (this.currentPatch !== this.patches.length) {
            this.patches[this.currentPatch].hunks().then(processHunks.bind(null, this));
          } else {
            this.next();
          }
        },
        pushHunks: function (hunks) {
          this.hunks = hunks;
          this.currentHunk = -1;
        },
        nextHunk: function () {
          this.currentHunk++;
          if (this.currentHunk !== this.hunks.length) {
            var hunkHeader = this.hunks[this.currentHunk].header().trim();
            log(hunkHeader.substring(0, hunkHeader.length - 1));

            this.hunks[this.currentHunk].lines().then(processLines.bind(null, this));
          } else {
            this.nextPatch();
          }
        },
        increaseModificationCount: function () {
          this.modificationCount++;
          req.modificationCount = this.modificationCount;
        }
      };

      diffs.patches().then(processPatches.bind(null, context));
    });
};

function processPatches(context, patches) {
  if (patches.length === 0) {
    return context.next();
  }

  context.pushPatches(patches);
  context.nextPatch();
}

function processHunks(context, hunks) {
  if (hunks.length === 0) {
    return context.next();
  }

  context.pushHunks(hunks);
  context.nextHunk();
}

function processLines(context, lines) {
  lines.forEach(function (line) {
    var diffString = String.fromCharCode(line.origin()) + line.content().trim();
    log(diffString);
    var originChar = String.fromCharCode(line.origin());

    var diffLine = {
      contents: line.content().trim(),
      isAddition: originChar === '+',
      isDeletion: originChar === '-',
      isContext: originChar === ' '
    };

    if (diffLine.isAddition || diffLine.isDeletion) {
      context.increaseModificationCount();
    }

    context.diffResult.push(diffLine);
  });

  context.nextHunk();
}

I would like a review of this because although it works, it feels too complex and procedural, so perhaps I'm missing a simpler or more proper solution here.

\$\endgroup\$
-1
\$\begingroup\$

It seems good to me. Even though a little breakdown into a smaller functions would help I guess.

\$\endgroup\$
  • \$\begingroup\$ If you could expand on that thought, or comment on other aspects of the code, this would be a better answer. \$\endgroup\$ – Edward Apr 3 '17 at 22:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.