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

OCD suggestion on merging PRs more neatly #89

Closed
dandv opened this issue Jun 19, 2015 · 3 comments
Closed

OCD suggestion on merging PRs more neatly #89

dandv opened this issue Jun 19, 2015 · 3 comments

Comments

@dandv
Copy link
Contributor

dandv commented Jun 19, 2015

image

I struggled myself with this one but managed to get it working :)

image

@kahmali
Copy link
Owner

kahmali commented Jun 20, 2015

Great suggestion! (Not that I expect anything less from you) I was actually discussing this issue with a friend recently. I hate that the merge button on GitHub creates an extra merge commit even when it's a fast-forward merge! That's just absurd. I'll practice this in a test repo so I don't destroy anything here, but I definitely plan to incorporate this into my workflow. I'll keep this issue open until I pull it off in here for the first time. Thanks for the information!

@kahmali
Copy link
Owner

kahmali commented Jul 1, 2015

Hey @dandv! I made an improvement on Meteor's suggested workflow for merging PRs! Wow. I literally have a tear of excitement in my eye right now. This is what a clean commit history does to me. :P I feel like I've achieved the ultimate level of commit history cleanliness.

All I did was make sure the PR was rebased onto the latest version of the branch I was merging it with, devel. Then I checked out the PR branch locally, and instead of cherry-picking the commit from the PR branch (as Meteor's workflow suggested), I just merged it into devel with

> git checkout pr/XXX
...test changes...
> git checkout devel
> git merge --ff-only pr/XXX
> git push

That should always be a fast-forward merge, since the branch was rebased on devel.

Check this history out. It comes from PR #97, which you can see is marked as "Merged" on GitHub, as if I had clicked the evil Merge button! What you won't see, however, is any extra merge commit, or even that unnecessary modification of the commit by the repo owner! The large picture on GitHub next to the commit log shows the author of the commit, and the small inset picture is the committer (if they're different -- otherwise only the one pic is shown). So now, the original author and committer can be preserved, without any useless information polluting the commit history. Even better, because the original commit is preserved, GitHub will now be able to detect when that commit is merged upstream, and will automatically mark the PR as "Merged" and close it as soon as the commit is pushed. Yay for git OCD!

The one piece of useful information that is now missing (since there's no more merge commit) is a reference to the PR that a commit comes from. I'll have to add that to my contributing guidelines, so PR numbers are included in commit messages (maybe at the end of the subject line). Thanks so much for this suggestion! Let me know what you think of my change to the workflow.

@dandv
Copy link
Contributor Author

dandv commented Aug 15, 2015

Such a nice application of this procedure as in #123 merits mention on StackOverflow :)

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

No branches or pull requests

2 participants