
Merge Pull Request Considered Harmful - watson
http://blog.spreedly.com/2014/06/24/merge-pull-request-considered-harmful/
======
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.

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

~~~
lmm
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?

~~~
warfangle
It might make sense to squash so you only have compile-ready commits in your
log.

In a sense I work this way, without the squashing. When I write a test, the
build fails and nothing has been done on the application codebase so I don't
commit. When I fix the build by writing new code, I commit.

Maybe I should be doing small intermediate commits and squashing the commits
into one commit.

~~~
lomnakkus
> Maybe I should be doing small intermediate commits and squashing the commits
> into one commit.

I would recommend trying that, or at least something similar: Try committing
regularly (this is useful if you ever want to go back to a prior state while
working or e.g. pick up where you left off on a different machine, etc), but
reorganizing and cleaning up your topic branch (via rebase -i, etc.) into a
_few_ commits before merging (fast forward). What is "a few commits"? Well,
it's a bit of a judgement call, but I just group stuff into logicially
coherent units which make sense as single units of functionality. Of course
you must make sure each individual commit at least builds sensibly and
preferably that all tests run too.

~~~
warfangle
Yeah, I've been using 'have I added things, and do my tests pass' as a metric
for when I should commit - I'm pretty strictly TDD these days.

------
MaikuMori
Is this exactly situation that [https://help.github.com/articles/checking-out-
pull-requests-...](https://help.github.com/articles/checking-out-pull-
requests-locally) 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.

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

------
shadowmint
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
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-...](https://help.github.com/articles/checking-out-pull-requests-
locally#modifying-an-inactive-pull-request-locally)

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

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

------
thegeomaster
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...](https://github.com/torvalds/linux/pull/17#issuecomment-5654674)

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

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

~~~
ulisesrmzroche
That's cuz Linus Torvalds doesn't have much of a bedside manner. I agree with
ya'll on the quality commit message stuff, but that last third was all
conjecture from your part, homey.

~~~
tytso
The last third is based on my extensive opinion having worked on open source
software for my entire career, including being the ext4 subsystem maintainer
and the e2fsprogs author and maintainer.

~~~
thegeomaster
pwnd

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

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

------
watson
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
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?

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

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

~~~
watson
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

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

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

~~~
nirvdrum
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...](http://stackoverflow.com/questions/18750808/difference-between-
author-and-committer-in-git)

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.

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

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

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

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

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

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

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

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

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

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

------
leccine
"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? :)

------
grogenaut
Considered Harmful blog posts Considered Harmful

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

[http://meyerweb.com/eric/comment/chech.html](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 :-)

------
thinkt4nk
harmful?

~~~
rjbwork
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".

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

~~~
aResponder51
Depends on the size of the feature, and the maintainer, but you're right about
"talk to the maintainter."

Some projects, you'll want to do a weekend of really rough work, contact the
maintainer, get their thoughts. Others, you'll want to do 8 days work, get it
far enough along to show, get their thoughts.

Often though, before you write a line of code, talk to them.

