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

[git] Fixed duplicated entries for git history on merge operations #7188

Merged

Conversation

federicobozzini
Copy link
Contributor

It fixes #7186.

What it does

It merges together entries for a commit, when their sha is the same. An alternative approach would be to not get duplicated entries in the first place by changing the git log options used to load the git history.

How to test

Import https://github.com/ARMmbed/mbed-os-example-blinky in Theia and check its history.

Review checklist

Reminder for reviewers

@federicobozzini federicobozzini force-pushed the git-merge-double-entries branch from 04c9fd2 to 187f34f Compare February 20, 2020 15:57
@akosyakov akosyakov added the git issues related to git label Feb 21, 2020
@westbury
Copy link
Contributor

To test this out, I create a test repository. This has 2 branches with:
file A: modified in both branches (non-conflicting changes)
file B: changed in one branch, deleted in the other branch
file C: deleted in both branches
file D: created with different content in each branch
The branches were then merged. The conflicts being file B which was resolved by deleting the file and D which was resolved with a third version of the content.

I've pushed it to https://github.com/westbury/theia-git-test-repo

First viewed without the PR (current master):

image

Then viewed with the PR:

image

The problem is that there is a reason why Git decided to return two entries for the same merge commit. Each entry shows the changes for that appropriate parent. The same file could have changes in both parents. You are picking the file change from an arbitrary parent, showing just one.

Now we actually have another issue here because we are not keeping enough information to distinguish the two parents, so, on the master branch, one sees the same changes for a file in both the entries for the merge commit. This results in us seeing the same diff for files A and D in both the merge commit entries. I believe we should be seeing in each the diff appropriate for that parent.

I think we need to think about what this history really is. By showing the merge commits the same change will appear multiple times. One could argue that we should not show merge commits as those changes have already appeared in other commits.

A better solution would be, IMO, to simply set the firstParent flag. That would result in a linear list from master, with stuff in other branches showing as a single commit. That is also consistent with the 'amend' button in the source control view. The amend button would then go down the same list of commits as one sees in history.

@federicobozzini federicobozzini force-pushed the git-merge-double-entries branch 2 times, most recently from fc98cb6 to c46435a Compare February 25, 2020 11:33
Signed-off-by: Federico Bozzini <federico.bozzini@gmail.com>
@federicobozzini federicobozzini force-pushed the git-merge-double-entries branch from c46435a to 84f1e4a Compare February 25, 2020 11:34
@federicobozzini
Copy link
Contributor Author

You are right. The best approach is probably to just use the first-parent flag to return only the changes introduced with the merge operation. This is also consistent with the commit history you get on Github.

I've also completely removed the firstParent flag from the log options since it's not really used anywhere else.

@westbury
Copy link
Contributor

I'll test this after the history refactoring has merged.

Note that this does change the history view substantially. Currently the history view is is a list of commits and merge-commits in chronological order, jumbled up across branches. After this change just the master branch will be listed with every commit being the child of the commit below it. Individual commits on other branches won't be shown, just a single commit will be shown with all the changes from the branch.

I think this is the right thing to do because:

  1. Users seem to be assuming that each commit is based on the one below it.
  2. It fixes this issue and we don't have to fix the issue with the wrong diffs on the second parent.
  3. It is consistent with 'amend'.

In any case this does not affect anyone using the Git vscode builtin. It will also be changed anyway when the graph is added to this history view to show the branches.

Copy link
Contributor

@westbury westbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this PR with the current master branch. Using the repo at https://github.com/westbury/theia-git-test-repo, the history now correctly shows just the master branch and the correct file changes are shown. I have also checked the scm view (last commit and amend commit) because the log command is also used there. The last commit and amend commit now exactly track the history view.

@westbury westbury merged commit 924e6a2 into eclipse-theia:master Mar 10, 2020
@federicobozzini federicobozzini deleted the git-merge-double-entries branch March 10, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git issues related to git
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[git] git history panel might contain double entries for merge operations
3 participants