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

51–60 of 115 posts

Re: Merge Pull Request Considered Harmful

#51

Post by ntalbott »

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.

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.

Re: Merge Pull Request Considered Harmful

#52

Post by agmcleod »

drunken_thor wrote: post #30

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…

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.

Re: Merge Pull Request Considered Harmful

#53

Post by ntalbott »

gleenn wrote: post #3

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.

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.

Re: Merge Pull Request Considered Harmful

#54

Post by jessaustin »

eevilspock wrote: post #49
ntalbott wrote: post #40

Earlier quoted context omitted.

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…

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?

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?

Re: Merge Pull Request Considered Harmful

#55

Post by aResponder51 »

rlpb wrote: post #44
rjbwork wrote: post #34

Earlier quoted context omitted.

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 seperate…

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.

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.

Re: Merge Pull Request Considered Harmful

#56

Post by jakub_g »

ntalbott wrote: post #41
jakub_g wrote: post #17

Earlier quoted context omitted.

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

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

It fetches all the PRs. Once you open a PR for a project (even from a fork), github creates a local branch for that PR in the upstream project.

Note though that doing a fetch will take some time the first time after you set up this - it will fetch all the historical PRs.

Re: Merge Pull Request Considered Harmful

#57

Post by eLobato »

andrewchilds wrote: post #26
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

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

Re: Merge Pull Request Considered Harmful

#58

Post by ForHackernews »

drunken_thor wrote: post #30

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…

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.

Re: Merge Pull Request Considered Harmful

#59

Post by lelf »

ntalbott wrote: post #36
drunken_thor wrote: post #30

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…

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…

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

Re: Merge Pull Request Considered Harmful

#60

Post by lmm »

ForHackernews wrote: post #58
drunken_thor wrote: post #30

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…

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.

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?
Post reply on HN