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

Clone PR with merge instead rebase #2

Closed
wants to merge 3 commits into from

Conversation

ploh
Copy link

@ploh ploh commented Nov 15, 2018

No description provided.

@ploh
Copy link
Author

ploh commented Nov 15, 2018

@bradrydzewski Here my PR in the new repo, as suggested by you in drone-plugins/drone-git#76 (comment)

@bradrydzewski
Copy link
Member

bradrydzewski commented Nov 16, 2018

thanks, do we need to modify the unit tests here? If I remember correctly the goal was to avoid creating a new merge commit. If this pull request is able to checkout the correct commit sha, we should not need to modify the unit tests.

@ploh
Copy link
Author

ploh commented Nov 16, 2018

We do not need to modify the unit tests for the new behaviour (i.e. doing a git merge), but... the changed unit tests reveal a problem with the old behaviour (i.e. doing a git rebase):

The new unit test checks the drone git plugin behaviour in this situation:
screenshot from 2018-11-16 09-40-48

For a pull request that wants to merge squashed into master here, we cannot avoid creating a new commit at all. The old rebase code does the following here:

git checkout master
git rebase ff4dc6e3  # which is the head of branch squashed

Git tries to put the commits which are in master but not in squashed on top of ff4dc6e3.

The new merge code instead does:

git checkout master
git merge ff4dc6e3  # which is the head of branch squashed

Here, git tries to create a merge commit by merging ff4dc6e3 into the current head of master. (This is also what the previous technique of using the github pull ref did - only then we let github do the merging for us.)

Either way we can have merge conflicts if master and squashed have changed the same files since their last common commit. But with the rebase approach there can also be unnecessary conflicts:
In the new unittest the commit "This is a combination of 2 commits." contains exactly the same changes as the two commits on branch master. Hence, when simply merging the branch heads there is no conflict. But when rebasing each commit from master on top of squashed, the first commit will have a conflict and then the second will have a conflict again.

Btw: On github this can only happen when the "Require branches to be up to date before merging" mark is not checked, otherwise github will automatically require that squashed needs to be built on top of master already.
screenshot from 2018-11-16 09-19-27

@ploh
Copy link
Author

ploh commented Nov 16, 2018

Oh, yeah, forgot to add:
In case of squashed being "up-to-date", i.e. built on top of master (as can be required via github branch protection settings), rebase and merge are both fine and do not create an extra commit - this is still checked by the old unittest which I did not semantically modify.

@ploh
Copy link
Author

ploh commented Dec 13, 2018

@bradrydzewski Anything I can do to speed up this topic / PR?

@bradrydzewski
Copy link
Member

hey there, this change was manually merged and applied to master, so this can be closed :) thanks again for all your research, much appreciated.

@ploh
Copy link
Author

ploh commented Dec 15, 2018

@bradrydzewski Glad to see the change from rebase to merge being made 👍

I was surprised to not see my additional unit tests being adopted - those really show why the new behaviour is better... and guard against future regressions in this area. I invested quite some time into that and am convinced they are a substantial improvement.

I must admit that the deeper concepts of how git branching, merging etc work can be quite complicated if you are unfamiliar with it. If you would like to invest the time to learn about it, I would be happy to help. Since Drone is closely tied to version control, I am sure you would not regret diving deeper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants