On Monday October 8, corbet@lwn.net wrote:

I find it is always good to know *why* we have the tags.  That
information is a useful complement to what they mean, and can guide
people in adding them.

So below I present some "Purposes", YetAnotherTag, and a comment on
the RSO.

(And I'd like to add a vote for "Blame-Shared-By:" rather than

   From:        The Author, Primary Author, or Authors of the patch.
                Authors should also provide a Signed-off-by: tag.


                Purpose: to allow subsequent review of the originality of 

                Purpose:  to inform upstream aggregators that
		consensus was achieved for the change.  This is
		particularly relevant for changes that affect multiple

		Purpose: to inform upstream aggregators that due
		diligence has been performed to ensure correctness of

		Purpose: to ensure that interested parties are


This seems more detailed that necessary.  The process (communicated
back / responded) is not really relevant.  I would go for something
like:

    (b) I have no outstanding problems, concerns, or questions about
        this patch (except as noted in the above comments).

and in fact, given (c2), (b) might not be needed at all.

-

[ message continues ]

Hmm...I was just going to go with the "because I told you so" approach
that I use with my kids.  It works so well with them after all.  

<pauses to go scream at his kids who have never understood why playing
"Dance Dance Revolution" directly above the office is hard on
productivity> 

I agree with just about everything you've said, and am tweaking things

Instead, it seems to me that the process is crucially important.
Reviewed-by shouldn't be a rubber stamp that somebody applies to a
patch; I think it should really imply that issues of interest have been
communicated to the developers.  If we are setting expectations for what
Reviewed-by means, I would prefer to leave an explicit mention of
communication in there.  If I'm in the minority here, though, it can
certainly come out.

Thanks,

jon

-

[ message continues ]

> > > + (b) Any problems, concerns, or questions relating to the patch have been
 > > > +     communicated back to the submitter.  I am satisfied with how the
 > > > +     submitter has responded to my comments.
 > > 
 > > This seems more detailed that necessary.  The process (communicated
 > > back / responded) is not really relevant.
 > 
 > Instead, it seems to me that the process is crucially important.

I agree with you Jon.  In fact my first reaction to your initial post
was that this section (b) was the most succinct distillation of the
most important part of reviewing that I've seen.

 - R.
-

[ message continues ]

I couldn't agree more, Jon.

If we are to have a meaningful reviewed-by tag, it has to be clearly
documented as to what responsibilities it places on the reviewer. If
someone doesn't want to perform a well conducted review, then they
haven't earned the right to issue a Reviewed-by tag - they can use
the Acked-by rubber stamp instead.

FWIW, w.r.t. XFS patches, we already follow both the letter and
intent of your proposed reviewed-by tag for all changes to XFS code
and reviewers are currently listed as Signed-off-by in git-commits
(our internal SCM records the reviewer(s) and the git export script
converts that to s-o-b).  It would be much more meaningful if they
were exported as Reviewed-by under your definition....

IOWs, I fully support your definition of the Reviewed-by tag.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group
-

[ message continues ]

The SCM should include this info and we should not duplicate this
in the changelog's.

We often use s-o-b to docuemnt the path a patch took from origin (the
top-most s-o-b) to tree apply (lowest s-o-b).
This is IIUC part of the intended behaviour of s-o-b but it is not
consensus seems too strong a wording here. consensus imply more than one
that agree on the patch where I often see people give their "Acked-by:" by
simple changelog reading.
So Acked-by: is not used like documented today.

	Sam
-

[ message continues ]