harmful?
It's a meme, sort of: http://en.wikipedia.org/wiki/Considered_harmful.
Merge Pull Request Considered Harmful
31–40 of 115 posts
Re: Merge Pull Request Considered Harmful
#32Hm... 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 br…
>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?
...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.
Re: Merge Pull Request Considered Harmful
#33Earlier quoted context omitted.
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…
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.
Re: Merge Pull Request Considered Harmful
#34harmful?
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".
Re: Merge Pull Request Considered Harmful
#35harmful?
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…
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?
Re: Merge Pull Request Considered Harmful
#36I 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 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.
Re: Merge Pull Request Considered Harmful
#37Earlier 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…
Agreed, editing contributions does add a bit of extra work for contributors post-inclusion, especially if they've been using their own branch in production. That said, since this is a PR flow, the contributor will get an email from Github that their PR has been closed, and they can track directly from that PR to the closing commit.
While I've often wished that Github had a proper mailing list per repository that could be used to discuss changes, I have to say that one awesome thing about PR's is how nice they make reviewing a patch. The diff view is just awesome, so now that I have a `git am` style flow with them it would be hard for me to switch to just passing patches around on a mailing list. That said, a mailing list would still be a welcome adjunct to the PR flow if done right.
Re: Merge Pull Request Considered Harmful
#38harmful?
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…
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.
Re: Merge Pull Request Considered Harmful
#39Earlier quoted context omitted.
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…
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.
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.
Re: Merge Pull Request Considered Harmful
#40Hm... 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 br…
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 . If you don't like it, that's cool, then it's more for me!" :-)