devhub-tud / devhub

DevHub is a software system designed to give students a simple practical introduction into modern software development.
15 stars 8 forks source link

Merging in pull request may inadvertently inherit from last merge commit rather than last commit #409

Closed jamy015 closed 7 years ago

jamy015 commented 8 years ago

If master is at a non-merge commit A, and a pull request is opened for a branch originating from master, and this branch is at commit B, merging this pull request should create a merge commit with A and B as parents. However, what actually happens is that a merge commit with parents B and the last merge commit on master is created.

Example: capture Before merging pull request #7, master was at 'Update pom.xml'. Pull request #7 contained only 'Add state machine tests for game.Game'. Expected result: 'Merge pull request #7' has as parents 'Update pom.xml' and 'Add state machine tests for game.Game' Actual result: 'Merge pull request #7' has as parents 'Merge pull request #6' and 'Add state machine tests for game.Game'

Fastjur commented 8 years ago

Does master loose changes from Update pom.xml(1)?

I am uncertain what the issue is. As Add state machine tests for game.Game(2) is a commit later than (1), it is expected that (2) is now the parent of Merge pull request... right?

If you do git log on master after merging the pull request and then pulling master, does the commit Update pom.xml still appear on master after pulling?

jwgmeligmeyling commented 8 years ago

So I forked your your repository

git clone ssh://git@devhub.ewi.tudelft.nl/jgmeligmeyling/test-repo
cd test-repo
git remote add jamy ssh://git@devhub.ewi.tudelft.nl/courses/ti1706/1516/group-14
git fetch jamy

And pulled your master in at Update pom.xml (54ee8b7).

git pull jamy 54ee8b73647152c440ac753ea4c936f04a28a87c
git push

Then I created your branch:

git checkout -b game-state-machine-tests
git pull jamy 974abdc1acf3835eaefd77dcc04249615fa1527a
git push origin game-state-machine-tests

Now my repository should be roughly in the same state as yours. As we can also see from the screenshot of the pull request, the branch is one commit ahead:

screen shot 2016-06-16 at 10 15 54

When I now pull master and perform a git log, the output is:

commit 5f0ac3f8dc9dae12d2f995fc1153622c34fc4a2d
Merge: 54ee8b7 974abdc
Author: Jan-Willem Gmelig Meyling <J.GmeligMeyling@student.tudelft.nl>
Date:   Thu Jun 16 10:17:11 2016 +0200

    Merge pull request #1 from game-state-machine-tests

This means that the PR has the correct parents. This also means that this problem needs more context to be reproduced. My intuition is that the work directory on the git server was still at a97a2e6 from the previous merge, and failed to checkout the 54ee8b7 from the Git server, and therefore merged 974abdc onto a97a2e6 instead. This gives us some new ideas to continue looking into https://github.com/devhub-tud/git-server/pull/89 - previous non-reproducable problems related to merging. Thanks for the detailled report!

jwgmeligmeyling commented 8 years ago

So I have added one more commit to the master

echo "MASTER AHEAD YOU BE" >> README.md 
git add README.md
git commit -m "Master is ahead of merge"
git push

And then to a branch as well:

git checkout -b ahead-branch
echo "DO OR DO NOT, THERE IS NO TRY" >> README.md 
git add README.md 
git commit -m "Yoda's quotes"
git push origin ahead

Creating a PR and merging that PR on Devhub results in the following tree:

*   2686c6a Merge pull request #2 from ahead-branch
|\  
| * 8cf11ec Yoda's quotes
| * 53a10e5 Master is ahead of merge
|/  
*   5f0ac3f Merge pull request #1 from game-state-machine-tests

So it seems that the work folder does not correctly pull the branch changes correctly.