[famousactress]

The title is a much more cynical take on the data than I have looking at it. I have some familiarity and experience with Django, one of the projects that has a pretty low pull request acceptance rate from his sample. At 29% I was impressed that many pull requests were accepted, frankly! I think the signal to noise is pretty high on pulls to high profile projects.

It's easy, and common I think, to throw a small change into a pull request and fire it off without confirming it's desired or within the objectives of the maintainers. I've done this at least once w/ Django personally. My thought process was "This may be undesired, but if it is desirable the chances of it making it in without bikeshedding in a mailing list is way higher if I just lead with a pull request."

In my case it wasn't desired (I think it was a settings.py var that allowed users to configure auto-import of models in the shell, dropped cause it's implicit over explicit and adds to settings.py).

Add to that all of the pull requests that are effectively proof of concepts because the implementation fails to take some major stuff into account, and an acceptance rate of almost a third starts to sounds pretty good to me.

[tytso]

It's so much better to send these sorts of proposal via e-mail. It's much easier to review the patches via e-mail, and everyone on the development community can make suggestions. Even so, it's not uncommon for patches to require three or four versions, and not just by first-time contributors. There are patches written by folks who have been working on ext4 for many years which still require three or four or more revisions after being subjected to multiple rounds of peer review.

If you show me a git project where most of the pull requests are accepted, I'll show you a git project whose quality assurance is probably completely out of control.

[jjh42]

I think that depends a lot on the project. Projects that heavily use github tend to prefer a github pull request.

It makes it easy to comment on the diff and integrates with Travis CI (integration tool) and other tools.

It is easy to make multiple changes to the pull request before it is accepted so nothing about this system implies that pull request are always accepted without changes.

[jedbrown]

It's easy for the original author to make changes to the PR, but not easy for the maintainer or a third party to make changes. The micro-pedantry associated with spelling errors, tweaking commit messages, and trivial spacing/formatting issues can be more work for everyone when communicated through comments on PRs with the original author expected to apply and re-roll.

[bad_user]

That's only partially true.

Pull requests on GitHub have to have an associated branch or fork in another repo. Even if you're not the original author, if you have access to it, you can do "git checkout" or "git clone", then you make the required changes and then on push the commits are automatically included in the pull request.

The only problem is that the maintainers do not have write access to the forked repositories of new or casual contributors. So if you're in a rush to change something, you have to fork that branch, make changes and then merge it, while ignoring the original pull request (although if you place the issue number in the commit messages you push, they'll get referenced on the associated page).

However, the commits logs made by the wannabe contributor are still in the history, so proper credit/blame is given where it is due, unless you rebase of course. And this isn't something you can say about diffs sent by email.

Also, GitHub sends you emails on pull requests. And you can reply by email too.

What I like about GitHub Pull requests is that you have a link you can give to people for review or that you can post wherever, a link that gives you a descriptive diff of what happened. Plus, the discussions around that pull request are left there, included in that link, left for everybody to see, instead of turning into a private conversation that nobody else will know about (incidentally, it's also the reason why I think native apps will never beat web apps, as long as native apps don't have URLs for referring to content or state within the app). This to me is useful enough to prefer it over diffs by email.

EDIT: there is one thing about GitHub I don't like - you cannot give write-access to people, while preventing them from pushing to master. If this where possible, you could give everybody access to push, except for master, in which case the above mentioned issue with pull requests would be a non-issue!

[tytso]

The problems with keeping the original commit and then putting the fixes on top are, (a) it makes the history harder to follow, (b) it makes it easy for someone to screw up a cherry-pick of a bug fix to a maintenance brance, since now you have to cherry-pick the bug fix plus the fix(es) to the bug fix, and (c) if the original patch had a potential bug that might cause the system to crash or otherwise malfunction, even if you fix it in the subsequent commit, it makes "git bisects" more likely to yield false positives, or at least make things more confusing and more difficult than it has to be.

[lucian1900]

Even when sending patches via email, I've always been asked by maintainers to do the trivialities myself and re-submit. I suspect at least part of the reason was education.

[adanto6840]

Does this hold true for you on smaller projects too? Ruby gems, for instance...?

I personally strongly prefer to see a diff via Github, especially for smaller features on a small library / gem -- although on larger & more popular projects (lots of pull requests) it definitely makes some sense that you'd prefer to have a 'heads up' of sorts via email first to confirm that it's even a desirable feature.

Really just wondering here, I recently started semi-blindly submitting pull requests with feature additions or small bug fixes (with very verbose pull request messages + solid but concise commit messages) to various library-type projects so I'm legitimately curious as far as how far to take this logic...

[jedbrown]

I think email is a better forum if everyone involved is disciplined about factoring commits well, writing good messages, using an email client that formats properly, and is fluent with the tools. PRs are more intuitive, more tolerant of workflows, and more accessible to newcomers. At this point, nothing matches the archival quality of mailing lists. When PRs are rebased/refactored as a result of reviews, the original versions generally become orphaned, so the discussion vanishes.

When there are multiple independent discussions spawned from different parts of single patch, as is common in less disciplined communities, the granularity of email is too coarse, and threaded line-comments are much better. On the other hand, patches on mailing lists frequently spin off into higher level design/philosophy discussions, which isn't appropriate in line comments.

[zalew]

well, I guess adding anything to settings.py will get red flags instantly because everyone is sick of settings, that is common knowledge when you follow django-dev and con vids.

my almost first contribution to django was recently hijacked by a better one, bummer :)

[famousactress]

Yeah, it was a long shot. I thought maybe the fact that it was entirely optional and not on by default might help my case, but the reasons for it being turned down were totally reasonable and consistent with the project & python idioms.

Still, I ended up building our own shell command that included the change because fuck typing that much.