Pushing patches

Matt Wagner matt.wagner at redhat.com
Wed May 16 17:43:58 UTC 2012


On Wed, May 16, 2012 at 12:20:01PM -0400, 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.

Sorry, I didn't mean to suggest that you didn't know. I was just
mentioning it for the benefit of those that might not be familiar with
it.

> >>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).

Ah, this makes sense. Perhaps the multi-branch thing was a red herring.
I think we're going to need to handle more branches soon, as we start
having multiple shipped versions coexisting. However, if the
responsibility for pushing to the branches isn't put on the reviewer, I
withdraw my opposition. :)

> >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.  

Apologies if I erroneously implied that some people were not pulling
their weight. What I intended is that patch review is a comparatively
unpleasant task, and that I wouldn't want to make it any more
unpleasant. However, it sounds like it was really a moot point on my
part, since it was all based on the assumption that reviewers were going
to have to deal with pushing things onto maintenance branches.

-- Matt



More information about the aeolus-devel mailing list