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

Should techniques for managing merges be added to the docs? #111

Open
Ichimonji10 opened this issue Feb 12, 2016 · 7 comments
Open

Should techniques for managing merges be added to the docs? #111

Ichimonji10 opened this issue Feb 12, 2016 · 7 comments

Comments

@Ichimonji10
Copy link
Contributor

When a pull request comes in, most checks are done automatically by Travis. It's great! However, there's a couple things I do that either cannot be automated or are not automated. Three come to mind:

  • For one, I never use the green "merge pull request" button. When you click that button, GitHub performs a non-fast-forward merge, even if the pull request is rebasd onto HEAD. That's silly, and it makes quite a few things unnecessarily hard. It makes reverting commits hard, it makes git bisect hard, it makes browsing the commit history hard - yeesh.
  • I always rebase commits before merging them. Not everyone has the know-how to do this, and it's nice that I can do this for people.
  • I check commit messages, both for formatting and content. Regarding formatting, I strongly like having a short headline, and a body with hard-wrapped text. This makes browsing the commit history with git log, git show and other such tools much easier. Regarding content, I strongly like having a descriptive message that states what's being done, why, and what the effect is on test suite results.
@Ichimonji10
Copy link
Contributor Author

Oh, one more thing: I almost always git checkout the code in pull requests and personally inspect it.

@Ichimonji10
Copy link
Contributor Author

For posterity, here's the procedure for getting, inspecting and merging a pull request. First, edit .git/config. Find the [origin] section (or whatever section points at this repository - for me it's [upstream]) and add the following before the existing fetch line:

fetch = +refs/pull/*/head:refs/remotes/origin/pr/*

Then do a git fetch. You'll end up with this!

screenshot_2016-02-12_18-16-34

Neat, right? Here's a typical workflow for inspecting, testing and merging a pull request:

git checkout upstream/pr/106
# inspect changes and test them out
git rebase master
# test them out again to ensure nothing broke
git branch -f master && git checkout master
git push origin master && git push upstream master

There's plenty of other similar workflows. See kahmali/meteor-restivus#89 (comment) for another testimonial.

@Ichimonji10
Copy link
Contributor Author

Given the above, I hope it's more apparent why I find good commit messages so important. In my book, it's a failure if I have to go to github just to figure out what a commit does. I should be able to execute git show abc1234 and find out plenty. Some examples:

screenshot_2016-02-12_18-31-37

@Ichimonji10
Copy link
Contributor Author

@dkliban FYI

@omaciel
Copy link
Contributor

omaciel commented Feb 14, 2016

@Ichimonji10 thank you for the write up. I shall implement the same behavior from now on and avoid the temptation of clicking the merge button :)

@Ichimonji10
Copy link
Contributor Author

@omaciel I'm glad you find this useful! I'll admit, my approach definitely has a higher up-front workload for the committer(s). But I hope the benefits are apparent.

@Ichimonji10 Ichimonji10 changed the title Update docs re: committing Should techniques for managing merges be added to the docs? Jul 8, 2016
@nixocio
Copy link

nixocio commented Jan 18, 2018

@Ichimonji10, thank you.

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

3 participants