8 ms·

Merge Pull Request Considered Harmful

271 points by watson 12y ago

shadowmint 12y 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 12y 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-locally#modifying-an-inactive-pull-request-locally https://help.github.com/articles/checking-out-pull-requests-... [2]: https://news.ycombinator.com/item?id=7949107 https://news.ycombinator.com/item?id=7949107
  - fidlefodl 12y ago
    I appreciate it! I had no idea that this was possible.
- eric_bullington 12y 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 12y 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.
- ntalbott 12y 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!" :-)
  - eevilspock 12y 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?
    - jessaustin 12y ago
      You totally could do that, but it doesn't really help with the lack of interest problem he described. If the contributor can't be arsed to fix their patch, can we expect them to be arsed to merge a patch to their patch and then re-PR it?
      - gknoy 12y ago
        Since PRs on Github auto-update when the branch they are based on is updated, they would not need to create a new pull request: Bob requests that Alice merge bob/bob-feature into alice/master. Alice requests that Bob merge alice/fix-bob-to-conform-to-pep8 into bob/bob-feature. Bob merges that into bob/bob-feature, and Bob's PR into Alice's repo is now auto-updated to reflect the changes that he merged into his branch. Alice is now satisfied, and merges bob/bob-feature with alice/master.
- russelluresti 12y 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 12y 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.
- ntalbott 12y 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.
watson 12y 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 12y 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.
  - watson 12y 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 12y ago
Is this exactly situation that https://help.github.com/articles/checking-out-pull-requests-locally 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.
- watson 12y 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 12y 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 12y 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_g 12y 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 https://gist.github.com/piscisaureus/3342247 and then you'll have each PR available under sth like git checkout pr/123
  - 12y ago
watson 12y 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 12y 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?
  - watson 12y 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 12y ago
    hub is just a bunch of shortcuts for operating on github repositories, rather than do it all by hand operation by operation.
  - ntalbott 12y 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.
- ntalbott 12y 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.
thegeomaster 12y 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-5654674 https://github.com/torvalds/linux/pull/17#issuecomment-56546...
- watwut 12y 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.
  - tytso 12y 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.
    - ulisesrmzroche 12y ago
      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.
SimeVidas 12y ago
I love the approach but hub seems to be Mac only :(
- warfangle 12y ago
  It doesn't build properly on Linux?
- nfm 12y ago
  It's just a ruby script, and it works on Linux/Mac/Windows.
thinkt4nk 12y ago
harmful?
- unwind 12y ago
  It's a meme, sort of: http://en.wikipedia.org/wiki/Considered_harmful http://en.wikipedia.org/wiki/Considered_harmful.
  - McGuffin 12y ago
    A more precise term would probably be snowclone.
  - rrss1122 12y ago
    Like all memes, it is mindlessly implemented to look "cool" or "clever".
- rjbwork 12y 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 12y 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?
drunken_thor 12y 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.
- ntalbott 12y 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 12y 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+).
    - ntalbott 12y ago
      Most of the contributions I'm merging in are 10 lines, but a lot of them (half?) take two or more (sometimes many more) commits to converge on those 10 lines, and none of those intermediate commits has useful information in them. The other half of contributions are only a single commit already. If someone contributes a true 1000+ line contribution that I'm even willing to take (yikes!) then you better believe I want that in multiple, logical commits, and it may even be me that ends up doing the splitting. I'm sure a lot of it just has to do with what types of contributions a given OSS project receives, so YMMV.
nirvdrum 12y 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.
- eevilspock 12y ago
  see ntalbott's comment: https://news.ycombinator.com/item?id=7949456 https://news.ycombinator.com/item?id=7949456
- ntalbott 12y 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.
  - nirvdrum 12y 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-between-author-and-committer-in-git 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.
- jessaustin 12y 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/contributors https://github.com/Shopify/active_merchant/graphs/contributo...
nirvdrum 12y 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.
ricket 12y 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 12y 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 12y 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 12y 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 12y ago
Considered Harmful blog posts Considered Harmful
- CtrlAltDel 12y ago
  We used to be elitist about our choice of language. We've evolved to become elitist about how we use source control.
- ntalbott 12y ago
  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 :-)
owenversteeg 12y 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.
- IanCal 12y 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 12y ago
You should be able to update your fork to be the same as upstream, with one click on your fork.
aut0mat0n1c 12y ago
Using the term 'considered harmful' in a blog post title considered harmful.
joshribakoff 12y 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

271 points by watson 12y 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 12y 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-locally#modifying-an-inactive-pull-request-locally https://help.github.com/articles/checking-out-pull-requests-... [2]: https://news.ycombinator.com/item?id=7949107 https://news.ycombinator.com/item?id=7949107
- fidlefodl 12y ago
  I appreciate it! I had no idea that this was possible.
eric_bullington 12y 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 12y 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.
ntalbott 12y 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!" :-)
- eevilspock 12y 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?
  - jessaustin 12y ago
    You totally could do that, but it doesn't really help with the lack of interest problem he described. If the contributor can't be arsed to fix their patch, can we expect them to be arsed to merge a patch to their patch and then re-PR it?
    - gknoy 12y ago
      Since PRs on Github auto-update when the branch they are based on is updated, they would not need to create a new pull request: Bob requests that Alice merge bob/bob-feature into alice/master. Alice requests that Bob merge alice/fix-bob-to-conform-to-pep8 into bob/bob-feature. Bob merges that into bob/bob-feature, and Bob's PR into Alice's repo is now auto-updated to reflect the changes that he merged into his branch. Alice is now satisfied, and merges bob/bob-feature with alice/master.
russelluresti 12y 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 12y 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.

ntalbott 12y 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.

watson 12y 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 12y 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.
- watson 12y 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 12y ago

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

watson 12y 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 12y 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 12y 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_g 12y 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 https://gist.github.com/piscisaureus/3342247 and then you'll have each PR available under sth like git checkout pr/123
- 12y ago

watson 12y 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 12y 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?
- watson 12y 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 12y ago
  hub is just a bunch of shortcuts for operating on github repositories, rather than do it all by hand operation by operation.
- ntalbott 12y 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.
ntalbott 12y 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.

thegeomaster 12y 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-5654674 https://github.com/torvalds/linux/pull/17#issuecomment-56546...

watwut 12y 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.
- tytso 12y 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.
  - ulisesrmzroche 12y ago
    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.

SimeVidas 12y ago

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

warfangle 12y ago
It doesn't build properly on Linux?
nfm 12y ago
It's just a ruby script, and it works on Linux/Mac/Windows.

thinkt4nk 12y ago

harmful?

unwind 12y ago
It's a meme, sort of: http://en.wikipedia.org/wiki/Considered_harmful http://en.wikipedia.org/wiki/Considered_harmful.
- McGuffin 12y ago
  A more precise term would probably be snowclone.
- rrss1122 12y ago
  Like all memes, it is mindlessly implemented to look "cool" or "clever".
rjbwork 12y 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 12y 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?

drunken_thor 12y 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.

ntalbott 12y 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 12y 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+).
  - ntalbott 12y ago
    Most of the contributions I'm merging in are 10 lines, but a lot of them (half?) take two or more (sometimes many more) commits to converge on those 10 lines, and none of those intermediate commits has useful information in them. The other half of contributions are only a single commit already. If someone contributes a true 1000+ line contribution that I'm even willing to take (yikes!) then you better believe I want that in multiple, logical commits, and it may even be me that ends up doing the splitting. I'm sure a lot of it just has to do with what types of contributions a given OSS project receives, so YMMV.

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

eevilspock 12y ago
see ntalbott's comment: https://news.ycombinator.com/item?id=7949456 https://news.ycombinator.com/item?id=7949456
ntalbott 12y 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.
- nirvdrum 12y 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-between-author-and-committer-in-git 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.
jessaustin 12y 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/contributors https://github.com/Shopify/active_merchant/graphs/contributo...

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

ricket 12y 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 12y 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 12y 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 12y 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 12y ago

Considered Harmful blog posts Considered Harmful

CtrlAltDel 12y ago
We used to be elitist about our choice of language. We've evolved to become elitist about how we use source control.
ntalbott 12y ago
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 :-)

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

IanCal 12y 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 12y ago

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

aut0mat0n1c 12y ago

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

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