Hey, On Wed, Sep 25, 2013 at 04:03:06PM -0500, Eric Sandeen wrote: > [stable@xxxxxxxxxxxxxxx stripped from this fascinating thread] Good idea. I should have done, in retrospect. > 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. Gah. Yeah, I overstated. Maybe 'does not necessarily imply an agreement' should read something more like 'does not necessarily imply it is beyond criticism', that's a bit more reasonable. > 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. I haven't read that in awhile. Nice to have a brush up, thanks. I think it probably comes short of '...you have no grounds to complain about the contents of patches that were committed on your watch...', which is the idea I disagree with. Maybe others don't think so, you could argue that I failed on point b in my review by not choosing to hound Dave for a commit message. ;) > 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. I agree that ignoring a compelling technical concern can be risky. > Maybe one reviewer is dissatisfied, and another is satisfied. Then > you get to play Solomon again. Life goes on, I hope. Yeah. Life goes on. Thanks Eric, Ben _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
