Pushing patches

Scott Seago sseago at redhat.com
Thu May 17 13:19:44 UTC 2012


On 05/17/2012 03:59 AM, Jan Provaznik wrote:
> On 05/16/2012 07:44 PM, Scott Seago wrote:
>> 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
>
> Hm, maybe I missed some important mail, but I thought that running 
> tests is _not_ part of review (no matter of patch complexity) - it's 
> author's responsibility to make sure that the patch doesn't break 
> tests (reviewer is really not needed for this step). I know we did run 
> tests before when doing a review, but I believe this has changed. Let 
> me know if I'm wrong about this - I don't run tests when doing reviews 
> for last X months.
>
Yes, this part isn't so clear right now. In some cases it seems 
necessary, in some cases not. However, if we change to a process where 
the reviewer is pushing, then we would need to run tests, since running 
tests is needed prior to pushing to handle the case where something 
rebased after the initial patch was sent is in conflict with something 
in the patch. If the "who pushes the patch" process change results in 
tests not being run immediately prior to push, we will have broken 
something in our process with this change.
>> 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
>>
>
> So the new process of doing a review (in simplest common situation) is:
> 1) apply and read the patch, test functionality
> 2) run rspec/cucumber tests
> 3) update BZ/Redmine instead of author -  if this is not done when the 
> patch is pushed, there will be mess and inconsistency
>
> It think it's better to make life easier for reviewer and steps 2 and 
> 3 should be done by author of the patch. I would prefer to stick with 
> current process instead of hand over more work to reviewer.
>
There are two places that tests must be run (and all pass):
1) prior to generating/sending the patch
2) prior to pushing the patch

With our current process, the patch author can do both. If we were to 
change to the proposed "reviewer pushes" process, then the reviewer 
would have to do the second round of testing. In some cases, that's 
happening already though, but not always.
> Jan




More information about the aeolus-devel mailing list