X Tutup
The Wayback Machine - https://web.archive.org/web/20200916181731/https://github.com/nodegit/nodegit/pull/1184
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing Revwalk test for fileHistoryWalk(*) #1184

Open
wants to merge 1 commit into
base: master
from

Conversation

@rcjsuen
Copy link
Member

rcjsuen commented Jan 8, 2017

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?

Revwalk's fileHistoryWalk(*) function does not find merge commits.
@rcjsuen
Copy link
Member Author

rcjsuen commented Jan 8, 2017

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
Copy link
Member Author

rcjsuen commented Jan 9, 2017

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
Copy link
Member

implausible commented Jan 26, 2017

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
Copy link
Member

implausible commented Jan 27, 2017

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
Copy link
Member

implausible commented Apr 28, 2017

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
Copy link
Member Author

rcjsuen commented May 10, 2019

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.

@implausible implausible force-pushed the nodegit:master branch from c345989 to c67b436 Aug 26, 2019
@implausible implausible force-pushed the nodegit:master branch from 2b7db46 to 69b010a Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.
X Tutup