Pushing patches

Scott Seago sseago at redhat.com
Wed May 16 17:44:35 UTC 2012


On 05/16/2012 12:20 PM, Jason Guiditta wrote:
> On 16/05/12 11:28 -0400, Matt Wagner wrote:
>> On Wed, May 16, 2012 at 10:03:17AM -0400, Jason Guiditta wrote:
>>> Hey, list, had a thought on pushing patches.  I have noticed some
>>> people (inconsistently) pushing patches others send after they review
>>> them, and adding a 'signed-off by' to the patch.  I kind of like this,
>>> as it adds a simple way to see who reviewed a patch.
>>
>> This is easily accomplished with adding "-s" to your git-am run.
>>
> Yes, I know, that is precisely what I was suggesting, I just omitted
> the command, making the (often erroneous) assumption people know how
> to use git, or can figure it out.
>
>>> This can make it
>>> easy to ask that person 'hey, I am hitting this problem after that
>>> patch, did you see this when you tested it?'.  Right now, it is a
>>> matter of combingthe list to find the ack, and if you delete your
>>> local copies of pushed patches (as I do), that means going to the
>>> archive, just t see who reviewed a patch so you can ask them a
>>> question.  I would personally much rather just do 'git log' and see
>>> who it was, so I can get hold of them as needed.  Thoughts, concerns,
>>> vehement opposition?  General apathy?
>>
>> Actually, I think I do have some opposition. :-\ Adding a signed-off-by
>> line is nice, but it implies that the reviewer is the one that pushes.
>> This is all well and good, except I think that we're going to have to
>> start juggling multiple branches soon, as we backport things to
>> maintenance branches.
>>
> I don't know what this is about pushing to multiple branches, I only
> ever push to master myself, and was expecting only that of the
> reviewer.  Any cherry-picking onto other branches is a separate step,
> imo, whoever might be the responsible party (certainly NOT the
> reviewer).
>
>> Compounding matters, I don't think many people really enjoy doing patch
>> reviews -- it's just something we begrudgingly do to help get patches
>> approved. (I can't say I speak for everyone, but I do know that given
>> the choice of writing code or reviewing someone's patch, I'm not the
>> only one that would choose to keep coding.) If reviewing someone's patch
>> suddenly saddles you with the responsibility of figuring out what
>> branches it needs to go onto, pushing it to all of them, and resolving
>> any conflicts that might arise, it's going to become considerably more
>> unappealing than it already is.
>>
>
> This is where I invoke Hugh.  Reviewing patches is part of everybody's
> job (at least those of us at RH, external community would be 
> different), and if people are not seeing that, I am sure a manager 
> will contact
> them shortly.  I think 'git am -s', and then 'git push' assuming the
> patch passes review is not a particularly onerous task.  Again, my
> suggestion is meaning reviewer pushes to master only, anything else is
> out of scope.
>
> -j
>
There will always be edge cases -- but, if we put certain parameters 
around it things shouldn't be so bad. Generally speaking when I review a 
patchset, unless there are explicit not-yet-acked patches it depends on 
(that should be relatively rare), I always 'git pull' master and then 
'git am' on a new topic branch from Master. If I'm able to ack the patch 
as-is, in most cases I should be able to push it without trouble. The 
only exception would be if someone checked in something which would 
result in merge conflicts _while I'm reviewing/testing the patch_. 
However, if conflicts come up that the reviewer can't resolve, this is 
really just a special case of the existing situation where the 
conflicting commits happen _before_ the reviewer applies the patch.

The other edge cases involve the "conditional ack" situations -- i.e. 
"ACK, but fix *above trivial issues* before pushing" -- in this case the 
patch submitter would need to submit as usual -- unless, as a separate 
question, we _also_ intend to disallow that bit of process shortcut and 
want to require re-sending and re-reviewing any patch, even with trivial 
fixes from the first review.

One other point here -- a while back we had explicitly agreed to lower 
the requirements for review in some cases -- that for particularly 
simple patches, a visual review/ACK would be sufficient -- i.e. not 
always requiring the reviewer to apply and run all tests for really 
simple stuff. If we stick with this process, these sorts of ACKs would 
still need the original submitter to push. Are we also proposing to 
reverse direction on this decision and go back to requiring 'git am' and 
running spec/cucumber on every patch review? In that case this goes away 
as a special case. Again, we're already doing the test/spec/etc for the 
majority of fixes, but at one point there was a lot of resistance to the 
notion that _every_ patch required the reviewer to do all of this, so we 
changed course for limited cases.

One final note -- currently the assumption is that the patch submitter 
will push a patch upon ACK receipt -- the exceptions are generally noted 
by reviewers replying with 'ACK and pushed' (often because they're 
waiting on the fix, or the submitter is offline either due to PTO, work 
time differences, etc). The new proposal will change the default 
assumption, but will make it more important for patch reviewers to be 
clear when they've pushed the patches and when they're relying on the 
submitter to push them.

Scott




More information about the aeolus-devel mailing list