Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd failing Revwalk test for fileHistoryWalk(*) #1184
Conversation
Revwalk's fileHistoryWalk(*) function does not find merge commits.
|
Okay. After looking at the code, it seems like the if (parents > 1) {
git_commit_free(nextCommit);
continue; |
walker.push(commitA);
// only check the A commit
walker.fileHistoryWalk("package.json", 1);So what should happen when the user runs this code where
Scenarios 2 and 3 are very close but it's an issue of which branch is the So, if you're merging in a 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.
The following scenarios have no common ancestors. This unique kind of merge commit does exist in the NodeGit repository.
|
|
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.
|
|
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. |
|
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. |
|
Talked to @implausible a little about this on Slack. Many moons ago I implemented
If you perform the following, you will actually notice that the merge commit is not included in |

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

rcjsuen commentedJan 8, 2017
It seems that
Revwalk'sfileHistoryWalk(*)function does not find merge commits.If you run
git logontest/repos/workdir/package.json(that's what's done in the test,./package.jsonwill work too), you can see that commit8076e2d8e8c2e5de4383d65abcbf4a5b4ea9254dis a merge commit that has affected thepackage.jsonfile. 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?