Add failing Revwalk test for fileHistoryWalk(*)

[rcjsuen]

It seems that Revwalk's fileHistoryWalk(*) function does not find merge commits.

If you run git log on test/repos/workdir/package.json (that's what's done in the test, ./package.json will work too), you can see that commit 8076e2d8e8c2e5de4383d65abcbf4a5b4ea9254d is a merge commit that has affected the package.json file. However, fileHistoryWalk(*) will not find this commit as shown by the attached failing test.

This bug makes fileHistoryWalk(*) unsuitable for creating a graphical log of a file. Do you have any ideas, @implausible?

[rcjsuen]

Okay. After looking at the code, it seems like the fileHistoryWalk(*) function will just ignore any commit that has more than one parent. I'm not sure what the reasoning behind this is though.

if (parents > 1) {
  git_commit_free(nextCommit);
  continue;

[rcjsuen]

walker.push(commitA);
// only check the A commit
walker.fileHistoryWalk("package.json", 1);

So what should happen when the user runs this code where A is a merge commit? Here are a couple of scenarios I have thought of. The following all assume a common ancestor exists for the merge commit A.

Scenario 1 - YES

A   Merged content from both branches
|\
B | Modified
\ C Modified
 \|
  D Unmodified

Scenario 2 - NO

A   Merged in branch containing commit C
|\
B | Modified
\ C Unmodified
 \|
  D Unmodified

Scenario 3 - YES

A   Merged in branch containing commit C
|\
B | Unmodified
\ C Modified
 \|
  D Unmodified

Scenarios 2 and 3 are very close but it's an issue of which branch is the master and which branch is the feature. Let's assume commit B is in the master branch and commit C is in the feature branch.

So, if you're merging in a feature branch and that branch has modified package.json (scenario 3) then I feel like the merge commit should be flagged as you're bringing in changes to the file into your current branch. On the other hand, if the change is in feature and not in the master then I'm inclined to say the merge commit shouldn't be flagged as the merge commit is not bringing in any new changes to the file.

Of course, the same logic would apply if the merge commit contains three or more parents. If any of the branches that are being merged in have modified the file, then the merge commit should be flagged.

Scenario 4 - YES

A   Merged content from both branches
|\
B | Unmodified
| C Unmodified
D | Modified
| E Modified
 \|
  F Unmodified

Scenario 5 - NO

A   Merged content from both branches
|\
B | Unmodified
| C Modified again, but reverts the changes from E
D | Unmodified
| E Modified
 \|
  F Unmodified

Scenario 6 - YES

A   Merged and further modified in A, such as by amending the commit with further changes
|\
| B Modified
|/
C   Unmodified

Scenario 7 - NO

A   Fast-forwarded merge commit with no further content change
|\
| B Modified
|/
C   Unmodified

Scenario 8 - NO

A   Fast-forwarded merge commit with no further content changes
|\
| B Modified
|/
C   Modified

The following scenarios have no common ancestors. This unique kind of merge commit does exist in the NodeGit repository.

Scenario 9 - YES

 A   Merged in branch containing commit C, creating the file with the content of C
/ \
B  | File did not exist
|  C Modified
|  |

Scenario 10 - NO

 A   Merged in branch containing commit C, file is of course unchanged
/ \
B  | Modified
|  C File did not exist
|  |

[implausible]

Yes. So when we developed this, we originally did show merge commits, but for some reason or another, that was shot down from one of the main NodeGit crowd so I ripped it out. Definitely is a bug now that we've used it and considered what the implications of that decision were. Thanks for adding the failing test. We'll need to revise the fileHistoryWalk to include merge commits.

I think it is as simple as removing the logic for discarding commits with more than 1 parent.

[implausible]

Took a little more time to digest what you're saying. I think your approach of diffing parents until we can prove there is no file change in a merge commit (octopus like commits) should suffice. We should move in the direction that tests merge commit parent diffs until all parents have been tested or a the file change was discovered. Thanks so much for breaking out this scenario for me.

[implausible]

Hmm, so I did some playing around with this today. I think maybe we should just try removing the skip merge commits statement all together. We will have the merge commit be treated like any other commit and diff against it's 0th parent. I think that's what git log does anyway.

[rcjsuen]

Talked to @implausible a little about this on Slack. Many moons ago I implemented git log myself and found that it wasn't always enough to just compare the merge commit its first (0-th) parent.

$ git init

$ touch a.txt

$ git add a.txt 

$ git commit -m "Add"
[master (root-commit) d38b8cb] Add
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 a.txt

$ git checkout -b branch
Switched to a new branch 'branch'

$ echo "a" > a.txt

$ git commit -a -m "Mod"
[branch fe5b100] branch #time Mod
 1 file changed, 1 insertion(+)

$ git checkout master
Switched to branch 'master'

$ git merge --no-ff branch

$ git log a.txt

If you perform the following, you will actually notice that the merge commit is not included in git log even though from a comparison point of view it has changed.