Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Eric Sandeen <sandeen@sandeen.net>]

[stable@vger.kernel.org stripped from this fascinating thread]

On 9/25/13 1:38 PM, Ben Myers wrote:
> Hey Dave,
> 
> On Wed, Sep 25, 2013 at 09:33:31AM +1000, Dave Chinner wrote:

<snip>

>> If a reviewer doesn't speak up about something, then that implies
>> the reviewer considers it acceptible.
> 
> I disagree with that statement.  A reviewer might not speak up about a
> flaw in a work for for other reasons.  There are more than two
> alternatives in this context.  For example...

<snip>

<apologies if I snipped too much context, not my intention>

>> If the
>> reviewer does not ask for improvements or you chose not to review
>> the proposed patches, then you have no grounds to complain about the
>> contents of patches that were committed on your watch...
> 
> I disagree.  The act of committing a patch does not necessarily imply an
> agreement regarding it's contents.  I am often in a position where I
> have to commit patches that I don't fully agree with.  This doesn't
> imply that I've waived my right to whine about bad commits later.  ;)

If you don't like it, don't add reviewed-by.

If you don't like the reviews, don't sign off, don't merge it.
As maintainer you have that right, but to be a good maintainer,
you need a strong reason.  And if you have a strong technical concern,
then it's the patch author's duty to take it seriously, or risk not
getting the patch merged.  And the author might argue back.  And other
people might argue back.  And in the end, if we can all keep our cool,
the code will be better for it.  If you apply clear and consistent merge
criteria then people will know what to expect.

When you send a Reviewed-by: that's a pretty strong indication of agreement.
There shouldn't be a lot of misunderstanding about what it means;
the kernel doc (Documentation/SubmittingPatches) is very clear:

===
        Reviewer's statement of oversight

        By offering my Reviewed-by: tag, I state that:

         (a) I have carried out a technical review of this patch to
             evaluate its appropriateness and readiness for inclusion into
             the mainline kernel.

         (b) Any problems, concerns, or questions relating to the patch
             have been communicated back to the submitter.  I am satisfied
             with the submitter's response to my comments.

         (c) While there may be things that could be improved with this
             submission, I believe that it is, at this time, (1) a
             worthwhile modification to the kernel, and (2) free of known
             issues which would argue against its inclusion.

         (d) While I have reviewed the patch and believe it to be sound, I
             do not (unless explicitly stated elsewhere) make any
             warranties or guarantees that it will achieve its stated
             purpose or function properly in any given situation.
===

If you can't get behind that, don't add your Reviewed-by: and continue to
work it out until you can.  Reviewers should be sure to pay attention to
point (c) as well, something I sometimes forget myself.

Reviews don't have to be heeded, either.  They don't have to be gauntlets
thrown down or lines in the sand.  "You've corrupted memory here" is
different from "you could use an args struct instead of 8 arguments"
or "your commit log isn't very descriptive."  Discretion abounds.

As a reviewer once said on another list, "I'm free to share
what occurs to me and you're free to tell me to go jump in a lake."
Especially for cosmetic issues, that's a pretty good POV.  Sometimes
it's trickier.

As maintainer I guess you get to decide if a review concern has enough
merit to hold up merging.  Ignoring a compelling technical concern
could be risky.

Maybe one reviewer is dissatisfied, and another is satisfied.  Then
you get to play Solomon again.  Life goes on, I hope.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs