Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Merge Pull Request Considered Harmful

blog.spreedly.com

Post reply on HN
Print view View original

21–30 of 115 posts

Re: Merge Pull Request Considered Harmful

#21

Post by peff »

ntalbott wrote: post #19
MaikuMori wrote: post #5

Is this exactly situation that https://help.github.com/articles/checking-out-pull-requests-... documents? So basically: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME And now you can edit that pull request and/or make new pull request based on the previous one. Or just simply merge in master.

OP author here... The result is the same, but I've found the `git am` workflow to be much smoother vs. mucking around with remotes. Some of that could be due to the nature of the OSS project I work on - ActiveMerchant - since just in the last month we've had 30+ unique contributors, and most of them have contributed a single change. My general recommendation is to make sure you try out the `git am` flow for a bit, bu…

Keep in mind that `git am` is not identical to pulling the commits. You might be applying the commits on a different base, and certainly your committer field will be different (not to mention any substantive changes you make). As a result, the sha1 of the commits you create will be different than those of the original submitter.

This makes life harder for the submitter, because they cannot ask a simple question: were my commits merged into the upstream repository? Because no, they weren't; but commits that are the moral equivalent were. Usually `git log --cherry-pick` can correlate the two, but not always.

We do use `git am` upstream when working on git itself, for the reasons you indicate in the article (plus we like mailing-list based review). But it does come at a cost in managing the various versions of patches.

Re: Merge Pull Request Considered Harmful

#22

Post by johnkeeping »

watson wrote: post #9
MaikuMori wrote: post #5

Is this exactly situation that https://help.github.com/articles/checking-out-pull-requests-... documents? So basically: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME And now you can edit that pull request and/or make new pull request based on the previous one. Or just simply merge in master.

This also eliminates the use of the "Merge pull request" button which removes all the pull-request-merged-commits. I don't know about you guys, but I have not found them useful? Do you use the merge commits for anything?

I always use "--no-ff" when merging topic branches to master. The advantage is that you can keep individual commits on the topic branch readable and still get an overview of the changes between two releases with "git log --first-parent".

You can also see the set of changes in a topic merge easily with "git log ${merge}^2..${merge}^1" whereas if you use a fast-forward merge it's not at all obvious which sets of commits are related.

Re: Merge Pull Request Considered Harmful

#24

Post by lotyrin »

ntalbott wrote: post #19
MaikuMori wrote: post #5

Is this exactly situation that https://help.github.com/articles/checking-out-pull-requests-... documents? So basically: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME And now you can edit that pull request and/or make new pull request based on the previous one. Or just simply merge in master.

OP author here... The result is the same, but I've found the `git am` workflow to be much smoother vs. mucking around with remotes. Some of that could be due to the nature of the OSS project I work on - ActiveMerchant - since just in the last month we've had 30+ unique contributors, and most of them have contributed a single change. My general recommendation is to make sure you try out the `git am` flow for a bit, bu…

I think maybe you overlooked the grandparent's suggestion; you don't have to mess with adding contributors' remotes. Open pull request heads are available on your existing remote.

Re: Merge Pull Request Considered Harmful

#25

Post by aaronharnly »

watson wrote: post #9
MaikuMori wrote: post #5

Is this exactly situation that https://help.github.com/articles/checking-out-pull-requests-... documents? So basically: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME And now you can edit that pull request and/or make new pull request based on the previous one. Or just simply merge in master.

This also eliminates the use of the "Merge pull request" button which removes all the pull-request-merged-commits. I don't know about you guys, but I have not found them useful? Do you use the merge commits for anything?

We use the merge commits to trace back a given deploy or machine image to the pull request that caused it. So from a given deployed AMI, a Jenkins job can point to the pull request, and hence the full context of the feature or bug description, the back-and-forth of code review, etc.

Re: Merge Pull Request Considered Harmful

#26

Post by andrewchilds »

MaikuMori wrote: post #5

Is this exactly situation that https://help.github.com/articles/checking-out-pull-requests-... documents? So basically: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME And now you can edit that pull request and/or make new pull request based on the previous one. Or just simply merge in master.

The problem is that pull requests are one-way, read-only collaborations. If I want to make changes, I have to close the contributor's PR and open my own. With write access, I could do:

    git fetch origin pull/ID/head:BRANCHNAME
    git checkout BRANCHNAME
    // make changes
    git push origin pull/ID/head

Re: Merge Pull Request Considered Harmful

#27

Post by tytso »

watwut wrote: post #16
thegeomaster wrote: post #13

Torvalds himself doesn't like GitHub's pull request workflow for several reasons and doesn't accept pull requests on GH for the Linux kernel, see [1]. [1]: https://github.com/torvalds/linux/pull/17#issuecomment-56546...

He seems to be complaining primary about quality of commit messages, missing emails, commit sign offs an things like that. It is more about bureaucracy (which is arguably important on project if linux size) then about workflow itself.

It's about the patches not being fit for merging. It might be because of the commit sign-offs, or it might be because it was missing error checking, or any number of things. The real question is which incurs more overhead --- asking the contributor to fix it, and then needing to wonder when/if the contributor will fix it, or to just make the d*mned changes yourself, while preserving bisectability.

Another very common one isn't about bureaucracy, but making the commit messages readable --- including the stack trace of the faliure you are fixable (which again is important for non-toy projects that are being distributed in other products, and where people may cherry-pick fixes into a stable branch used for release purposes). Or if you have contributors from around the world whose native language is not English, and you want to make it easier for other people to understand what the heck is going on without having to read the contents of each and every diff.

This is fundamental to project health, which means that a proper workflow is fundamental to project health. Given that the vast majority of github repos are toy-sized, or end up being abandoned, maybe that's fine for github. But for any project where I have hopes that it will turn into something real (and if I don't have that hope, why would I waste time on it?), I'm not going to accept pull requests from git hub.

It is not just going to happen.

Re: Merge Pull Request Considered Harmful

#28

Post by tytso »

peff wrote: post #21
ntalbott wrote: post #19

Earlier quoted context omitted.

OP author here... The result is the same, but I've found the `git am` workflow to be much smoother vs. mucking around with remotes. Some of that could be due to the nature of the OSS project I work on - ActiveMerchant - since just in the last month we've had 30+ unique contributors, and most of them have contributed a single change. My general recommendation is to make sure you try out the `git am` flow for a bit, bu…

Keep in mind that `git am` is not identical to pulling the commits. You might be applying the commits on a different base, and certainly your committer field will be different (not to mention any substantive changes you make). As a result, the sha1 of the commits you create will be different than those of the original submitter. This makes life harder for the submitter, because they cannot ask a simple question: were…

This is why it is a good idea for proposed patches to be sent to a mailing list, where other people other than the repo owner can more easily review patches, and then to have those patches tracked via patchwork.

And when I accept a patch, even if I've had to edit the commit summary so it is parsable english (not all of my contributors speak english as a first language), I send a reply via e-mail saying "Thanks, applied".

Re: Merge Pull Request Considered Harmful

#30

Post by drunken_thor »

I really do not see the problem with the rails commit history. Having those merge messages that point back to a pull request leaves lots of documentation about what was done and why it was done. I really do not know why people are so particular about their git log either. It shows an accurate history of the repo, not a revised cleaned up history.

However being able to edit pull request before merging was a good thing to learn. However I think in the long run I would want to learn it with plain old git rather than tack on another tool to my workflow.

Post reply on HN