Posted Jun 7, 2012 10:06 UTC (Thu) by aliguori (subscriber, #30636)
[Link]
I think github has really ruined people's workflows with git. If the first time your code is being reviewed is a PULL request, you're doing it wrong.
Still emailing patches?
Posted Jun 7, 2012 13:04 UTC (Thu) by jengelh (subscriber, #33263)
[Link]
>If the first time your code is being reviewed is a PULL request, you're doing it wrong.
I do not think so. A pull request, in properly-organized circles, always involves an implicit request for review — which is why you can see "this is not ready yet"-type of warnings when the pull is not really meant as a pull, but as a review only that just happens to look like a pull request; git-request-pull(1) just makes for a short statistical overview.
Still emailing patches?
Posted Jun 7, 2012 16:03 UTC (Thu) by iabervon (subscriber, #722)
[Link]
The problem with pull requests for review is that they don't provide a useful structure for feedback on the code level; you really want the patch as the context for the discussion, and you want it to be one level quoted (so your comments read as a response to the code). A pull request is a good point for a bit of review, but it's really only great for, well, one bit: yes/no on taking the change in its current form.
Still emailing patches?
Posted Jun 7, 2012 17:01 UTC (Thu) by epa (subscriber, #39769)
[Link]
I imagined there would be some git tool which showed you the diff associated with a pull request and allowed you to comment on it (perhaps by simply forwarding it to yourself as electronic mail). Email is great for natural language communication but it seems strange to use it as the primary means to send copies of source code.
A pull request makes more sense, in a way, since if you want to view the code differences in a different format you can do so easily.
Still emailing patches?
Posted Jun 7, 2012 17:33 UTC (Thu) by iabervon (subscriber, #722)
[Link]
Oh, pull requests are definitely the way to go for shipping code around. But sending patches is generally a better way of starting a conversation, because it puts what the reviews will want to respond to already in the message that they're replying to. You can certainly generate the message to reply to, and tweak the reply to go back to the author, and you could arrange to have the reply look like a reply to the pull request (so that threading works) and you could arrange to follow project policies on cc:ing other potential reviewers so they can skim your review and know what doesn't need to be said again, but it all just happens automatically if the patch is the original message.
Of course, this is only relevant if the project communication is mailing-list-based, and particularly if it's got policies which imply that participants must have heavy-duty email-handling techniques; that's not all (or even most) projects, but it is the kernel.
Still emailing patches?
Posted Jun 9, 2012 20:29 UTC (Sat) by marcH (subscriber, #57642)
[Link]
A combination of a bug/feature request tracker + email typically solve all these problems nicely. Granted, every tracker requires some amount of customization but, well within the abilities of this community.
Is there here an aversion to databases generally speaking? Or maybe it's just because many maintainers have become too attached to their hard-earned email scripts and shortcuts.
Still emailing patches?
Posted Jun 7, 2012 18:44 UTC (Thu) by jengelh (subscriber, #33263)
[Link]
>pull requests [...] don't provide a useful structure for feedback on the code level; you really want the patch as the context for the discussion
Pull requests without patches may work in some circles. However, where trees grow by the gardening rules of David Miller — and the networking tree has been named as an organized citizen by LWN before, sort of —, pulls better had patches alongside.
Do `git send-email --compose ...`, and while in the editor, run-and-paste the output of `git request-pull ...`.
Still emailing patches?
Posted Jun 9, 2012 20:09 UTC (Sat) by marcH (subscriber, #57642)
[Link]
> > If the first time your code is being reviewed is a PULL request, you're doing it wrong.
> I do not think so. A pull request, in properly-organized circles, always involves an implicit request for review
Should be called a fetch request.
Still emailing patches?
Posted Jun 10, 2012 18:29 UTC (Sun) by mathstuf (subscriber, #69389)
[Link]
Eh, that doesn't mesh well with git's verbiage. I don't really care if all someone does is fetch my code from my repository. What I am actually asking for is someone to fetch and integrate my code into their tree. This is done with pull usually. Other terms I can accept: merge request, patch request, patch review, and branch review. I don't think 'fetch' really works here though.
Still emailing patches?
Posted Jun 11, 2012 6:56 UTC (Mon) by marcH (subscriber, #57642)
[Link]
> I don't really care if all someone does is fetch my code from my repository.
So, if I ask you to fetch my (not ready yet) code you will do it just to waste bandwidth?
Still emailing patches?
Posted Jun 11, 2012 5:51 UTC (Mon) by broonie (subscriber, #7078)
[Link]
Of course they are, e-mail is the main code review mechanism the kernel uses. Pretty much everything should be going through e-mail so people can provide review comments even if the patch ends up getting transferred via git.