Re: GNU Tools Cauldron 2017 follow up: "Reviewed-by" etc.

Thomas Schwinge
2017-10-19T15:57:52Z

Hi!

Still waiting for any kind of reaction -- general process-change inertia,
chicken-and-egg problem, I suppose. ;-/

I have now put the proposed text onto a wiki page, so that those
interested have a convenient handle to use,
<https://gcc.gnu.org/wiki/Reviewed-by>.

Ping.

On Wed, 4 Oct 2017 15:47:30 +0200, I wrote:
> Ping.
>
> On Fri, 22 Sep 2017 20:37:50 +0200, I wrote:
> > On Thu, 21 Sep 2017 12:18:39 -0600, Carlos O'Donell <carlos@redhat.com> wrote:
> > > On 09/21/2017 11:56 AM, Richard Biener wrote:
> > > > On Thu, 21 Sep 2017 11:38:29 -0600, Carlos O'Donell <carlos@redhat.com> wrote:
> > > > > On 09/21/2017 10:50 AM, Thomas Schwinge wrote:
> > > > > > So my question is, if I've gotten a patch reviewed by someone who is not
> > > > > > yet ;-) familiar with that new process, and I nevertheless want to
> > > > > > acknowledge their time invested in review by putting "Reviewed-by" into
> > > > > > the commit log, is it fine to do that if the reviewer just answered with
> > > > > > "OK" (or similar) instead of an explicit "Reviewed-by: NAME <EMAIL>"
> > > > > > statement?
> > > > > You should instead ask the author to give their "Reviewed-by:" and point
> > > > > out what the Reviewed-by statement means.
> > > > >
> > > > > > That is, is it fine to assume that our current patch review's standard
> > > > > > "OK" (or similar) answer matches the more formal "Reviewer's statement of
> > > > > > oversight"?
> > > > >
> > > > > Not yet.
> > > >
> > > > I think given an OK from an official reviewer entitles you to commit
> > > > it indeed IS matching the formal statement. It better does...
> >
> > I certainly understand your rationale, and do agree to that -- yet, I can
> > see how somebody might get offended if turning a casual "OK" into a
> > formal "Reviewed-by: NAME <EMAIL>", so...
> >
> > > Isn't it better to be explicit about this; rather than assuming?
> >
> > ..., yeah, that makes sense.
> >
> > Anyway: aside from starting to use them, we should also document such new
> > processes, so we might do it as follows, where I had the idea that the
> > *submitter* 'should encourage the reviewer to "earn" this
> > acknowledgement'.
> >
> > Gerald, OK to commit? If approving this patch, please respond with
> > "Reviewed-by: NAME <EMAIL>" so that your effort will be recorded. See
> > <https://gcc.gnu.org/contribute.html#patches-review>. There you go. ;-)
> >
> > Index: htdocs/contribute.html
> > ===================================================================
> > RCS file: /cvs/gcc/wwwdocs/htdocs/contribute.html,v
> > retrieving revision 1.87
> > diff -u -p -r1.87 contribute.html
> > --- htdocs/contribute.html 9 Apr 2015 21:49:31 -0000 1.87
> > +++ htdocs/contribute.html 22 Sep 2017 18:20:04 -0000
> > @@ -23,7 +23,7 @@ contributions must meet:
> > Testing Patches
> > Documentation Changes
> > Web Site Changes
> > - Submitting Patches
> > + Preparing Patches
> > Announcing Changes (to our Users)
> >
> >
> > @@ -164,7 +164,7 @@ file" mode of the validator.
> > More about our web pages.
> >
> >
> > - Submitting Patches
> > + Preparing Patches
> >
> > Every patch must have several pieces of information, before we
> > can properly evaluate it:
> > @@ -257,6 +257,71 @@ bzip2ed and uuencoded or encoded as a <c
> > acceptable, as long as the ChangeLog is still posted as plain text.
> >
> >
> > +
> > + Acknowledge Patch Review
> > +
> > + Patch review often is a time-consuming effort. It is appreciated to
> > + acknowledge this in the commit log. We are adapting
> > + the Reviewed-by: tag as established by the Linux kernel patch
> > + review process.
> > +
> > + As this is not yet an established process in GCC, you, as the submitter,
> > + should encourage the reviewer to "earn" this acknowledgement. For example,
> > + include the following in your patch submission:
> > +
> > +
> > + If approving this patch, please respond with "Reviewed-by: NAME
> > + &lt;EMAIL&gt;" so that your effort will be recorded. See
> > + &lt;https://gcc.gnu.org/contribute.html#patches-review&gt;.
> > +
> > +
> > +
> > + For reference, reproduced from
> > + the Linux
> > + kernel 4.13's Documentation/process/submitting-patches.rst:
> > +
> > +
> > +
> > + Reviewed-by: [...] indicates that the patch has been reviewed
> > + and found acceptable according to the Reviewer's Statement:
> > +
> > +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 [...].
> > +
> > +
> > + (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 [...], 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.
> > +
> > +
> > +A Reviewed-by: tag is a statement of opinion that the patch is an
> > +appropriate modification [...] without any remaining serious
> > +technical issues. Any interested reviewer (who has done the work) can
> > +offer a Reviewed-by: tag for a patch. This tag serves to give credit to
> > +reviewers and to inform maintainers of the degree of review which has been
> > +done on the patch. Reviewed-by: tags, when supplied by reviewers known to
> > +understand the subject area and to perform thorough reviews, will normally
> > +increase the likelihood of your patch getting [...] [approved].
> > +
> > +
> > + Submitting Patches
> > +
> > When you have all these pieces, bundle them up in a mail message and
> > send it to the appropriate mailing list(s).
> > (Patches will go to one or more lists depending on what you are
> >
> > (I have not yet spent much time on verifying the HTML, or formatting
> > tweaks.)

Grüße
Thomas