Merge Pull Request Considered Harmful

http://blog.spreedly.com/2014/06/24/merge-pull-request-considered-harmful/

by watson • 12 years ago

271 111 12 years ago

blog.spreedly.com

shadowmint• 12 years ago

Hm... this seems like a very complicated way of saying that github should have a way to merge pull requests into a new branch.

...but it doesn't so you have to:

    - checkout a local copy
    - add a remote to the PR
    - checkout a new branch
    - merge the PR into your local branch
    - fix code, merge to master

Which is entirely true; it is annoying.

The simple solution, though, is to require pull requests to come in a feature branch, and flat out reject any that target master. /shrug

dr4g0n• 12 years ago

You can create a new branch containing the pull request fairly easily.[1]

It's just: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME

Edit: I see that MaikuMori[2] posted the same information just before me; ah well.

[1] https://help.github.com/articles/checking-out-pull-requests-...

[2] https://news.ycombinator.com/item?id=7949107

fidlefodl• 12 years ago

I appreciate it! I had no idea that this was possible.

eric_bullington• 12 years ago

The simple solution, though, is to require pull requests to come in a feature branch, and flat out reject any that target master. /shrug

Agreed, and in my experience, every major open source project I'm familiar with requires pull requests to feature branches. In fact, most small projects use the same workflow.

Is this not the case with most open source projects?

snicker• 12 years ago

...and I believe the key word here is 'major'.

There are a lot of small, one-off, often very useful utilities that people are now sharing with each other via Github (I'm guilty/a participant in this phenomenon), and many noble users of these utilities want to help, contribute, and send PRs... a non-negligible number of them new to Git.

So, is it more of a PITA to set up a feature branch for a single python script and instruct users in your CONTRIBUTING file to 'make sure they submit PRs to branch XYZ!' or just deal with the odd occasional PR to master? Folks new to Git will probably just send a PR to master anyway (I believe OP addresses the 'new user' issue as well, having to explain Git commands to users in comments on a PR)

That all being said, I still typically follow the workflow shadowmint outlines above.

ntalbott14• 12 years ago

Using git remotes and distinct branches per contribution is totally legit, and I still do it sometimes. But, I'm often/usually dealing with contributions that have already been reviewed and don't need a whole feature branch/review within the root repository before inclusion. And for that setup - where it's almost ready - it's so much easier to just `git am` it into master, make necessary tweaks, and push.

In general I'd just encourage maintainers to try the `git am` flow, especially on small to medium complexity contributions where it looks mostly ready to go and you don't want to do another week of ping-pong with the contributor just to get a variable renamed or some whitespace fixed.

As I tell my kids, "Just try one bite of <food I find delicious>. If you don't like it, that's cool, then it's more for me!" :-)

eevilspock2• 12 years ago

What about submitting a PR with your cleanup to the original PR's branch? That person reviews and accepts, then submits a new PR back to you.

I haven't learned git yet so maybe my assumption that the original PR is on a branch/repo clone to which you can submit a PR is incorrect?

russelluresti• 12 years ago

Agreed. Having a pull request go to a feature branch (or bug branch, or whatever your pull request is aiming to do) is the best way to go about it.

The issue described by the author is definitely more of a workflow problem than a technology or service problem.

gleenn• 12 years ago

Interesting problem for committers. Since Github made the hub tool, maybe they could make this person's flow better right from the web UI somehow.

ntalbott14• 12 years ago

Maybe, but... probably not. The thing is, I want to work with the changes locally before they get included. So it can't be a 100% web workflow. That said, there might be ways the web UI could make the transition to the CLI easier, and it would be great if Github promoted the `git am` flow on the PR pages as well.

watson5• 12 years ago

Just a note to people who want to try this: According to the post you should use the command "git am -3 <url>", this seems to be a typo and should be: "hub am -3 <url>"

ytjohn• 12 years ago

Author didn't mention it, but in the hub documents they instruct you to `alias git=hub`. So when author is running git, he's really running hub.

watson5• 12 years ago

Ah, good point. I've been running hub for some time (love "hub pull-request"), but yes I never did set it up as an alias for some reason

MaikuMori• 12 years ago

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.

watson5• 12 years ago

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?

johnkeeping• 12 years ago

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.

aaronharnly• 12 years ago

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.

jakub_g2• 12 years ago

It's possible to configure it to have all PRs automatically fetched when you use `git fetch origin`:

https://gist.github.com/piscisaureus/3342247

and then you'll have each PR available under sth like

  git checkout pr/123

ntalbott14• 12 years ago

Cool, I'll have to try that! Does it also pull PR's that live on feature branches in forks of the root repo?

ntalbott14• 12 years ago

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, but then just do what works best for you.

peff• 12 years ago

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.

lotyrin• 12 years ago

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.

andrewchilds• 12 years ago

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

eLobato• 12 years ago

You can always send a PR to the original sender PR branch.

watson5• 12 years ago

I just tried this on an open pull request we had. Pretty ok experience. For some reason though, there where a merge conflict when running the "hub am -3 <url>" command. In my case was easy to fix, but Github reported the PR to be mergeable, so maybe Github uses a different merge strategy for PR's than "hub am" out of the box?

davedx• 12 years ago

Yeah, I don't get this hub thing. When I want to manually merge on a Github project I just follow the GH instructions on the PR page to manually checkout the fork, do my work there, and merge it locally then push. Why do you need another tool to do this?

watson5• 12 years ago

As far as I know hub is more than just a command line tool for pull requests. It extends git with a lot of Github nice-ness. But in the end some people just like CLI's better - and in this case it exposes some features that's not directly available in the UI.

masklinn• 12 years ago

hub is just a bunch of shortcuts for operating on github repositories, rather than do it all by hand operation by operation.

ntalbott14• 12 years ago

FWIW hub is even handy for the "add a remote for the fork, check out the fork branch" flow, since it lets you just `git remote add ntalbott` instead of having to find or derive the full url for the remote.

ntalbott14• 12 years ago

As I understand it `git am` is actually replaying the series of commits on top of the branch you run it in, whereas a merge commit flattens the commits out and applies them in one go (while keeping full history of the commits begin merged). So it's definitely a different strategy, and you can get conflicts where you wouldn't with a merge commit. As you said, though, they're super easy to fix when they happen. Often when I see a conflict like this it's because there are 52 (OK, exaggerating, a little) commits in the contribution and using the `git am` flow is a huge win since I'm now in a good spot to squash the down to one or two commits.

thegeomaster2• 12 years ago

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...

watwut• 12 years ago

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.

tytso3• 12 years ago

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.

ntalbott14• 12 years ago

These quality issues become a problem sooner than I would have thought, and Linux is big enough that they can just demand contributors know their stuff. But I found that in between "hobby project of my own" and "huge project where people beg me to contribute", there's a big space of "big enough that it gets contributions, but small enough that it's not worth turning contributors away for small quality issues". And I love that hub + `git am` lets me take those contributions but still maintain the workflow I want on the project.

SimeVidas• 12 years ago

I love the approach but hub seems to be Mac only :(

warfangle3• 12 years ago

It doesn't build properly on Linux?

nfm• 12 years ago

It's just a ruby script, and it works on Linux/Mac/Windows.

thinkt4nk• 12 years ago

harmful?

unwind• 12 years ago

It's a meme, sort of: http://en.wikipedia.org/wiki/Considered_harmful.

McGuffin• 12 years ago

A more precise term would probably be snowclone.

rrss1122• 12 years ago

Like all memes, it is mindlessly implemented to look "cool" or "clever".

rjbwork3• 12 years ago

It's a play on the classic Dijkstra letter, "Goto Considered Harmful." Also yes, I think it's harmful. A co-worker and I wanted to use a new up and coming FOSS project that's hosted on GitHub, but we needed a certain killer feature. Said co-worker worked for about 4 weekends in a row, and fully implemented it, with nice abstraction and separation of concerns. The code was then turned down because it was "too seperated, and unnessecarily abstracted" which we disagreed with. Thus the pull request, with an amazing feature, sits languishing because he nor I is going to spend another few weekends working on a project that won't integrate new features unless they're perfect and conform to the original author's idea of "perfect code".

al2o3cr• 12 years ago

Translation: "My co-worker wrote a clusterfuck of indirection spaghetti and the open-source volunteer refused to maintain it!"

Every PR that adds a feature adds future support and maintenance effort. If you and your buddy are unwilling to spend the time to get it right, why are you expecting the maintainer to spend the time to support it?

calinet6• 12 years ago

So what you have is a fork, with a fully implemented feature, that didn't make it back upstream.

This happens all the time, and is the nature of open source. No project is obligated to take your contribution, and that happens for a myriad of reasons that you are not obligated to understand.

It's fundamentally a human problem, not a software one. There's nothing wrong with the software here—it does everything required and more to be able to manage the codebase.

Take your fork, explain the situation in the README and let it be. It'll be in the fork graph and the list on Github. People can find it. If they like your feature, maybe they'll use yours instead. If enough people get behind it and ask for it, or say "hey this was in a PR, why wasn't it merged?", the pressure could be enough for the original author to just accept the PR.

In any case, this is not a problem with the pull request feature. It's a simple collaboration problem, and a PR is only one method of communication you can use to solve it. If you give up after throwing a PR into the void (not that I'm saying you did), you shouldn't expect instant success.

rlpb• 12 years ago

The right way to do this is to talk to the maintainer about how he wants the feature implemented first. Especially if it involves four weekends of work.

MichaelGG• 12 years ago

I was once on the other side of this. A nice guy wrote a whole ton of code for a module, refactored things to the max, made it "testable" and a bunch of other things. He was very nice, very enthusiastic, and really wanted to help.

But I had to say no because it didn't fit well within the overall project, and didn't really add any value but a lot of complexity. I felt very bad to turn him down but otherwise I'd be now responsible for code I have no interest in and code I don't feel helps users.

The right way is to talk to the maintainer and agree on what to do first. Or be prepared to run a fork.

drunken_thor• 12 years ago

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.

ntalbott14• 12 years ago

So to be clear, every commit in the ActiveMerchant history also points back at the initiating PR via the commit message including the PR number (which gets linked up). So there's no real loss of information in that sense

And I hear your RE adding another tool, as I'm very cautious about that myself. That said, `git am` is "git cannon", and all hub is really adding is the ability for it to slurp Github urls as well as Maildirs.

Neat, related trick: add ".patch" to the end of any PR url on Github and you'll see a formatted patch for the PR. All hub is really doing is slurping that and passing it to git as though it came from a mailing list.

lelf• 12 years ago

So there's no real loss of information in that sense

It is in reality. Try to git bisect a bug. (Yes, I read the part about the bisect in the post. But thanks, I'd rather search the bug in several 10-line patches than in one 1000+).

agmcleod• 12 years ago

Yeah i feel the same way. Never been one to squash my commits, even when i commit something stupid and have to revert it. We have three owners and one collaborator on a javascript project right now, we've been good on keeping up with pull requests, and closing off bugs. I can see where a bigger project like ActiveMerchant could get nuts if only one or two people are handling it.

ForHackernews2• 12 years ago

Yeah, I found the notion of a commit being "history worthy" kind of silly. If that's how it happened, then it's history! It's not a value judgement.

lmm2• 12 years ago

There's an argument that bisect is only useful if every commit is broken. If, as is often the case with my history, most commits don't compile, maybe there's an argument for squashing?

ntalbott14• 12 years ago

I think a lot of it depends on context. In a private repo I'm right there with you. But when I look at a public repo with a lot of contributors, I want to know what the "logical" history is a lot more than I want to know about the three steps it took to get a feature ready for public consumption.

npsimons5• 12 years ago

There's an issue here where people are conflating two definitions of "history". Sure, what happened in the real world can't be changed, but why should we be constrained by that in the worlds we construct in software? Just because someone calls a record of development "history" doesn't make it inviolate, and quite frankly, as a maintainer, I don't care about every little sneeze that a developer had on a project. I want each commit to compile and pass tests at a minimum, so that I can do automated git bisects. I would prefer to have a coherent history of well thought out conceptual chunks, so that I can figure out how things are supposed to be, not what some sleep-deprived programmer was thinking at 3AM on a Sunday.

livingparadox2• 12 years ago

Since the author of the article decided to invoke Linus, I thought I'd see what Linus thought about the merge vs rebase debate.

http://www.mail-archive.com/dri-devel@lists.sourceforge.net/...

Turns out Linus also agrees with drunken_thor, that merge messages are useful. Suppose that's why Linus added merges in the first place. ;)

chrismorgan• 12 years ago

His answer there on this topic boils down to “tell the patch author that they made a mistake, and steadfastly refuse to do anything with it yourself until the patch author does fix it”. This is the whole point of this article—that is simply not pragmatic; you can have changes made that aren’t quite perfect but where the original user is not willing to make the changes you require. It being abandoned in imperfect state, Linus’s answer is actually ignore it.

ntalbott14• 12 years ago

Well, quoting from that message, "If it's not ready, you send patches around". I think the big difference is that I'm willing to take patches that aren't ready and get them ready, whereas I'm sure Linus usually pushes back on the contributor to clean up their stuff. And I do think merges are super useful in a lot of cases, but Linus added `git am` too, and I like using all the tools at my disposal when they fit.

nirvdrum2• 12 years ago

I'm not sure how I feel about someone writing something and making me the author. I guess attribution makes sense in the form of paraphrasing. But I tend to think of commits as literal quotations.

eevilspock2• 12 years ago

see ntalbott's comment: https://news.ycombinator.com/item?id=7949456

ntalbott14• 12 years ago

You may think of commits as literal quotations of the "Author", but it's important to realize that git doesn't. If you want to know who's ultimately responsible for a commit, you always want to look at the "Committer" since that's who actually signed on the dotted line so to speak.

nirvdrum2• 12 years ago

I'm not concerned with who's ultimately responsible for the commit. I'm concerned with being called the author on something I didn't write. These distinctions are not made or enforced by git. At the end of the day, it's how humans interpret two pieces of metadata.

The first hit on Google and Bing for "git committer vs author" brings up this SO entry:

http://stackoverflow.com/questions/18750808/difference-betwe...

So, I don't think I'm taking some fringe view here. For my part, I don't think I want someone else writing code and putting my name on it.

jessaustin3• 12 years ago

I see what you mean, and it almost seems like the "sin" isn't the additional editing, it's the subsequent rebase. It's one thing to require contributors to rebase, but it seems like another to rebase for them. Maybe I'm missing something, but based on the "History Is Written By the Victors" section of TFA I would have been surprised to see anyone other than @ntalbott at the top of https://github.com/Shopify/active_merchant/graphs/contributo...

ricket• 12 years ago

"And as a hypothetical story-telling construct I created in my mind, Jane agrees 100%!"

That's a very strange way to end an article. Could have just stopped before adding that last line.

reledi• 12 years ago

What works best for me is to use a combination of the Pull Request button and to manually merge or rebase changes when necessary.

I used to strictly stay away from the Pull Request button, because it made my history "messy". Now I care less about that and more about the convenience (when it's appropriate).

leccine• 12 years ago

"If the contribution is a single commit, I’ll do a git commit --amend and just smoosh my changes right into the original commit."

that is right, editing history is the way to go, or is it? :)

fra• 12 years ago

I wish github would either detect when a branch is merged back into master and automatically close the PR, or allow for some more complex merge operation. Squash merge would be particularly helpful.

grogenaut• 12 years ago

Considered Harmful blog posts Considered Harmful

CtrlAltDel• 12 years ago

We used to be elitist about our choice of language. We've evolved to become elitist about how we use source control.

ntalbott14• 12 years ago

You forgot to reference the great recursive essay Eric Meyer wrote on exactly this topic:

http://meyerweb.com/eric/comment/chech.html

Which essay I already knew about and chose to willfully ignore when I titled the OP. We could chat over beers sometime as to whether I'm a bad person for doing so :-)

owenversteeg• 12 years ago

You know, when I started reading this I thought that I would disagree with you and write a grumpy comment to that effect. ("Considered Harmful" usually makes me grumpy.) However, you convinced me otherwise. Congratulations on a good "Considered Harmful" piece!

Here's another example: I recently made a PR to a project to fix a broken URL. I changed a wrong file and forgot about the PR. The maintainer had to close the PR, make the change himself, and explain why/what he did. This whole process would've been a lot simpler if Github allowed people to merge to another branch.

IanCal8• 12 years ago

This whole process would've been a lot simpler if Github allowed people to merge to another branch.

Is this a problem that comes from forking? I raise PRs on my projects onto various different branches all the time. Or maybe I've misunderstood the issue.

Siecje• 12 years ago

You should be able to update your fork to be the same as upstream, with one click on your fork.

aut0mat0n1c• 12 years ago

Using the term 'considered harmful' in a blog post title considered harmful.

joshribakoff• 12 years ago

I've always just pulled in the contributor's branch, made additional commits to fix things up, and squashed all the commits before pushing up to GitHub. Not very difficult

Merge Pull Request Considered Harmful

Related Stories