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

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

[Thomas Schwinge]

Hi!

Sending this to the "main" GNU tools mailing lists.  Of course, that's
not meant to exclude other tools.


Amongst other things ;-) at the GNU Tools Cauldron 2017 we discussed
whether having a "Reviewed-by" tag in the commit log might provide an
incentive for more people to invest time in patch review, and to get that
investment recognized more prominently/formally.  For the scheme used by
the Linux kernel, search for "Reviewed-by" on
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst>.
(Similar concepts to acknowledge time spent with PR triaging and other
things have also been discussed briefly, but I'll focus on this one
first.)

Generally, if adopting such a scheme (and I'm all for it!), I suggest
that we do not needlessly deviate from established processes, such as the
Linux kernel's, because remembering/looking up which project is doing
something in exactly which way is always confusing.


As David Edelsohn (as far I remember?) eloquently has put it, a lot of
things in the GNU tools community happened/improved just because somebody
started doing something.  ;-)

Now, with "Reviewed-by" it's actually two people who have to start doing
something: according to the Linux kernel's process, putting "Reviewed-by:
NAME <EMAIL>" into his email, the reviewer "indicates that the patch has
been reviewed and found acceptable according to the Reviewer's
Statement".  Then see "Reviewer's statement of oversight" on
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst>:

| 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.
| 
| A Reviewed-by tag is a statement of opinion that the patch is an
| appropriate modification of the kernel 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 into the kernel.

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?

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"?

If yes, then that will make it easier to start using this scheme,
avoiding the need for additional roundtrips à la: 'Does your patch
review's "OK" answer correspond to the "Reviewer's statement of
oversight" [...]'.

Maybe in the future, reviewers will then switch over to explicitly
stating "Reviewed-by: NAME <EMAIL>" -- or maybe not, because "OK" is just
so much easier to type...


Grüße
 Thomas

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

[Carlos O'Donell]

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.

> Maybe in the future, reviewers will then switch over to explicitly
> stating "Reviewed-by: NAME <EMAIL>" -- or maybe not, because "OK" is just
> so much easier to type...

All of this is nothing compared to the work of doing the review.

It will be your own personal comments, your reminder, your leading by 
example, that will change behaviours.

-- 
Cheers,
Carlos.

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

[Richard Biener]

On September 21, 2017 7:38:29 PM GMT+02:00, 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... 

>> Maybe in the future, reviewers will then switch over to explicitly
>> stating "Reviewed-by: NAME <EMAIL>" -- or maybe not, because "OK" is
>just
>> so much easier to type...
>All of this is nothing compared to the work of doing the review.

Depends on the complexity of the patch... 

Richard. 

>It will be your own personal comments, your reminder, your leading by 
>example, that will change behaviours.

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

[Carlos O'Donell]

On 09/21/2017 11:56 AM, Richard Biener wrote:
>> 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...

Isn't it better to be explicit about this; rather than assuming?

>> All of this is nothing compared to the work of doing the review.
> 
> Depends on the complexity of the patch...  

... depends on your ability to create quick paste hot keys :}

-- 
Cheers,
Carlos.

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

[Richard Biener]

On September 21, 2017 8:18:39 PM GMT+02:00, Carlos O'Donell <carlos@redhat.com> wrote:
>On 09/21/2017 11:56 AM, Richard Biener wrote:
>>> 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...
>
>Isn't it better to be explicit about this; rather than assuming?
>
>>> All of this is nothing compared to the work of doing the review.
>> 
>> Depends on the complexity of the patch...  
>
>... depends on your ability to create quick paste hot keys :}

Indeed. Any idea how to do that with K9 / SwiftKey? 

Richard. 

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

[Carlos O'Donell]

On 09/21/2017 12:38 PM, Richard Biener wrote:
> On September 21, 2017 8:18:39 PM GMT+02:00, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 09/21/2017 11:56 AM, Richard Biener wrote:
>>>> 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...
>>
>> Isn't it better to be explicit about this; rather than assuming?
>>
>>>> All of this is nothing compared to the work of doing the review.
>>>
>>> Depends on the complexity of the patch...  
>>
>> ... depends on your ability to create quick paste hot keys :}
> 
> Indeed. Any idea how to do that with K9 / SwiftKey? 

No idea. Right now I use Thunderbird+Clippings, and hotkey my responses
via ctrl+alt+v,r and ctrl+alt+v,s for Reviewed-by/Signed-off-by. Then
with mutt+vim I use recorded macros.

-- 
Cheers,
Carlos.

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

[Thomas Schwinge]

Hi!

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:</p>
 <li><a href="#testing">Testing Patches</a></li>
 <li><a href="#docchanges">Documentation Changes</a></li>
 <li><a href="#webchanges">Web Site Changes</a></li>
-<li><a href="#patches">Submitting Patches</a></li>
+<li><a href="#patches">Preparing Patches</a></li>
 <li><a href="#announce">Announcing Changes (to our Users)</a></li>
 </ul>
 
@@ -164,7 +164,7 @@ file" mode of the validator.</p>
 <p>More <a href="about.html#cvs">about our web pages</a>.</p>
 
 
-<h2><a name="patches">Submitting Patches</a></h2>
+<h2><a name="patches">Preparing Patches</a></h2>
 
 <p>Every patch must have several pieces of information, <em>before</em> we
 can properly evaluate it:</p>
@@ -257,6 +257,71 @@ bzip2ed and uuencoded or encoded as a <c
 acceptable, as long as the ChangeLog is still posted as plain text.
 </p>
 
+<!-- (Eventually) referenced from many places.  -->
+<h3><a name="patches-review">Acknowledge Patch Review</a></h3>
+
+<p>Patch review often is a time-consuming effort.  It is appreciated to
+  acknowledge this in the commit log.  We are adapting
+  the <code>Reviewed-by:</code> tag as established by the Linux kernel patch
+  review process.</p>
+
+<p>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:</p>
+
+<blockquote>
+  <p>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;.
+  </p>
+</blockquote>
+
+<p>For reference, reproduced from
+  the <a href="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">Linux
+  kernel 4.13's <code>Documentation/process/submitting-patches.rst</code></a>:
+</p>
+
+<blockquote cite="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">
+  <p><em>Reviewed-by:</em> [...] indicates that the patch has been reviewed
+    and found acceptable according to the Reviewer's Statement:<br>
+<br>
+<strong>Reviewer's statement of oversight</strong><br>
+<br>
+By offering my <em>Reviewed-by:</em> tag, I state that:<br>
+<br>
+	 (a) I have carried out a technical review of this patch to
+	     evaluate its appropriateness and readiness for inclusion [...].
+<br>
+<br>
+	 (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.
+<br>
+<br>
+	 (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.
+<br>
+<br>
+	 (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.
+<br>
+<br>
+A <em>Reviewed-by:</em> 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 <em>Reviewed-by:</em> 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.  <em>Reviewed-by:</em> 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].
+</p></blockquote>
+
+<h3>Submitting Patches</a></h3>
+
 <p>When you have all these pieces, bundle them up in a mail message and
 send it to <a href="lists.html">the appropriate mailing list(s)</a>.
 (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

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

[Thomas Schwinge]

Hi!

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:</p>
>  <li><a href="#testing">Testing Patches</a></li>
>  <li><a href="#docchanges">Documentation Changes</a></li>
>  <li><a href="#webchanges">Web Site Changes</a></li>
> -<li><a href="#patches">Submitting Patches</a></li>
> +<li><a href="#patches">Preparing Patches</a></li>
>  <li><a href="#announce">Announcing Changes (to our Users)</a></li>
>  </ul>
>  
> @@ -164,7 +164,7 @@ file" mode of the validator.</p>
>  <p>More <a href="about.html#cvs">about our web pages</a>.</p>
>  
>  
> -<h2><a name="patches">Submitting Patches</a></h2>
> +<h2><a name="patches">Preparing Patches</a></h2>
>  
>  <p>Every patch must have several pieces of information, <em>before</em> we
>  can properly evaluate it:</p>
> @@ -257,6 +257,71 @@ bzip2ed and uuencoded or encoded as a <c
>  acceptable, as long as the ChangeLog is still posted as plain text.
>  </p>
>  
> +<!-- (Eventually) referenced from many places.  -->
> +<h3><a name="patches-review">Acknowledge Patch Review</a></h3>
> +
> +<p>Patch review often is a time-consuming effort.  It is appreciated to
> +  acknowledge this in the commit log.  We are adapting
> +  the <code>Reviewed-by:</code> tag as established by the Linux kernel patch
> +  review process.</p>
> +
> +<p>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:</p>
> +
> +<blockquote>
> +  <p>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;.
> +  </p>
> +</blockquote>
> +
> +<p>For reference, reproduced from
> +  the <a href="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">Linux
> +  kernel 4.13's <code>Documentation/process/submitting-patches.rst</code></a>:
> +</p>
> +
> +<blockquote cite="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">
> +  <p><em>Reviewed-by:</em> [...] indicates that the patch has been reviewed
> +    and found acceptable according to the Reviewer's Statement:<br>
> +<br>
> +<strong>Reviewer's statement of oversight</strong><br>
> +<br>
> +By offering my <em>Reviewed-by:</em> tag, I state that:<br>
> +<br>
> +	 (a) I have carried out a technical review of this patch to
> +	     evaluate its appropriateness and readiness for inclusion [...].
> +<br>
> +<br>
> +	 (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.
> +<br>
> +<br>
> +	 (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.
> +<br>
> +<br>
> +	 (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.
> +<br>
> +<br>
> +A <em>Reviewed-by:</em> 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 <em>Reviewed-by:</em> 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.  <em>Reviewed-by:</em> 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].
> +</p></blockquote>
> +
> +<h3>Submitting Patches</a></h3>
> +
>  <p>When you have all these pieces, bundle them up in a mail message and
>  send it to <a href="lists.html">the appropriate mailing list(s)</a>.
>  (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

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

[Thomas Schwinge]

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:</p>
> >  <li><a href="#testing">Testing Patches</a></li>
> >  <li><a href="#docchanges">Documentation Changes</a></li>
> >  <li><a href="#webchanges">Web Site Changes</a></li>
> > -<li><a href="#patches">Submitting Patches</a></li>
> > +<li><a href="#patches">Preparing Patches</a></li>
> >  <li><a href="#announce">Announcing Changes (to our Users)</a></li>
> >  </ul>
> >  
> > @@ -164,7 +164,7 @@ file" mode of the validator.</p>
> >  <p>More <a href="about.html#cvs">about our web pages</a>.</p>
> >  
> >  
> > -<h2><a name="patches">Submitting Patches</a></h2>
> > +<h2><a name="patches">Preparing Patches</a></h2>
> >  
> >  <p>Every patch must have several pieces of information, <em>before</em> we
> >  can properly evaluate it:</p>
> > @@ -257,6 +257,71 @@ bzip2ed and uuencoded or encoded as a <c
> >  acceptable, as long as the ChangeLog is still posted as plain text.
> >  </p>
> >  
> > +<!-- (Eventually) referenced from many places.  -->
> > +<h3><a name="patches-review">Acknowledge Patch Review</a></h3>
> > +
> > +<p>Patch review often is a time-consuming effort.  It is appreciated to
> > +  acknowledge this in the commit log.  We are adapting
> > +  the <code>Reviewed-by:</code> tag as established by the Linux kernel patch
> > +  review process.</p>
> > +
> > +<p>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:</p>
> > +
> > +<blockquote>
> > +  <p>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;.
> > +  </p>
> > +</blockquote>
> > +
> > +<p>For reference, reproduced from
> > +  the <a href="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">Linux
> > +  kernel 4.13's <code>Documentation/process/submitting-patches.rst</code></a>:
> > +</p>
> > +
> > +<blockquote cite="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">
> > +  <p><em>Reviewed-by:</em> [...] indicates that the patch has been reviewed
> > +    and found acceptable according to the Reviewer's Statement:<br>
> > +<br>
> > +<strong>Reviewer's statement of oversight</strong><br>
> > +<br>
> > +By offering my <em>Reviewed-by:</em> tag, I state that:<br>
> > +<br>
> > +	 (a) I have carried out a technical review of this patch to
> > +	     evaluate its appropriateness and readiness for inclusion [...].
> > +<br>
> > +<br>
> > +	 (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.
> > +<br>
> > +<br>
> > +	 (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.
> > +<br>
> > +<br>
> > +	 (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.
> > +<br>
> > +<br>
> > +A <em>Reviewed-by:</em> 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 <em>Reviewed-by:</em> 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.  <em>Reviewed-by:</em> 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].
> > +</p></blockquote>
> > +
> > +<h3>Submitting Patches</a></h3>
> > +
> >  <p>When you have all these pieces, bundle them up in a mail message and
> >  send it to <a href="lists.html">the appropriate mailing list(s)</a>.
> >  (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

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

[Carlos O'Donell]

On 10/19/2017 08:57 AM, Thomas Schwinge wrote:
> 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>.

I've started using Reviewed-by: Carlos O'Donell <carlos@redhat.com>
and Signed-off-by: Carlos O'Donell <carlos@redhat.com> in all my
glibc reviews.

Since then I've seen 5 such items go into the git commit messages.

Progress? :-)

-- 
Cheers,
Carlos.

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

[Martin Sebor]

On 10/19/2017 09:57 AM, Thomas Schwinge wrote:
> 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>.

Quoting from the Wiki:

    If approving this patch, please respond with "Reviewed-by:...

Often several people provide helpful feedback on patches that
only one person ultimately approves.  As per the GCC process,
the approver is also one of the maintainers for the area
affected by the patch, and so had to demonstrate the value of
their own contribution to the area by committing many high
quality changes of their own.  Their sustained and valuable
effort has already been recognized (they are prominently
mentioned in the MAINTAINERs file).  So without in any way
diminishing their continued contribution by reviewing and
approving other people's work in addition to making valuable
improvements of their own, I think by focusing on approvals,
the Reviewed-by proposal neglects to acknowledge the hard
work of all the others who contribute to the project.

Martin

>
>
> 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:</p>
>>>  <li><a href="#testing">Testing Patches</a></li>
>>>  <li><a href="#docchanges">Documentation Changes</a></li>
>>>  <li><a href="#webchanges">Web Site Changes</a></li>
>>> -<li><a href="#patches">Submitting Patches</a></li>
>>> +<li><a href="#patches">Preparing Patches</a></li>
>>>  <li><a href="#announce">Announcing Changes (to our Users)</a></li>
>>>  </ul>
>>>
>>> @@ -164,7 +164,7 @@ file" mode of the validator.</p>
>>>  <p>More <a href="about.html#cvs">about our web pages</a>.</p>
>>>
>>>
>>> -<h2><a name="patches">Submitting Patches</a></h2>
>>> +<h2><a name="patches">Preparing Patches</a></h2>
>>>
>>>  <p>Every patch must have several pieces of information, <em>before</em> we
>>>  can properly evaluate it:</p>
>>> @@ -257,6 +257,71 @@ bzip2ed and uuencoded or encoded as a <c
>>>  acceptable, as long as the ChangeLog is still posted as plain text.
>>>  </p>
>>>
>>> +<!-- (Eventually) referenced from many places.  -->
>>> +<h3><a name="patches-review">Acknowledge Patch Review</a></h3>
>>> +
>>> +<p>Patch review often is a time-consuming effort.  It is appreciated to
>>> +  acknowledge this in the commit log.  We are adapting
>>> +  the <code>Reviewed-by:</code> tag as established by the Linux kernel patch
>>> +  review process.</p>
>>> +
>>> +<p>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:</p>
>>> +
>>> +<blockquote>
>>> +  <p>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;.
>>> +  </p>
>>> +</blockquote>
>>> +
>>> +<p>For reference, reproduced from
>>> +  the <a href="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">Linux
>>> +  kernel 4.13's <code>Documentation/process/submitting-patches.rst</code></a>:
>>> +</p>
>>> +
>>> +<blockquote cite="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.13#n560">
>>> +  <p><em>Reviewed-by:</em> [...] indicates that the patch has been reviewed
>>> +    and found acceptable according to the Reviewer's Statement:<br>
>>> +<br>
>>> +<strong>Reviewer's statement of oversight</strong><br>
>>> +<br>
>>> +By offering my <em>Reviewed-by:</em> tag, I state that:<br>
>>> +<br>
>>> +	 (a) I have carried out a technical review of this patch to
>>> +	     evaluate its appropriateness and readiness for inclusion [...].
>>> +<br>
>>> +<br>
>>> +	 (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.
>>> +<br>
>>> +<br>
>>> +	 (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.
>>> +<br>
>>> +<br>
>>> +	 (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.
>>> +<br>
>>> +<br>
>>> +A <em>Reviewed-by:</em> 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 <em>Reviewed-by:</em> 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.  <em>Reviewed-by:</em> 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].
>>> +</p></blockquote>
>>> +
>>> +<h3>Submitting Patches</a></h3>
>>> +
>>>  <p>When you have all these pieces, bundle them up in a mail message and
>>>  send it to <a href="lists.html">the appropriate mailing list(s)</a>.
>>>  (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
>

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

[Joseph Myers]

On Thu, 19 Oct 2017, Thomas Schwinge wrote:

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

That wiki page refers to Reviewed-by as being about crediting reviewers.  
But the specification appears to be oriented to something else entirely 
(i.e. convincing a committer - in a Linux-kernel-like context with a very 
limited set of committers to a particular tree, much smaller than the set 
of reviewers - that a patch is worthy of commit).  It doesn't cover 
reviews that request changes, or only relate to part of a patch, or relate 
to a previous version of a patch - only the limited special case of a 
review approving the entirety of a patch as posted.  If the aim is credit, 
a substantially different specification is needed.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Carlos O'Donell]

On 10/19/2017 09:45 AM, Joseph Myers wrote:
> On Thu, 19 Oct 2017, Thomas Schwinge wrote:
> 
>> 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>.
> 
> That wiki page refers to Reviewed-by as being about crediting reviewers.  
> But the specification appears to be oriented to something else entirely 
> (i.e. convincing a committer - in a Linux-kernel-like context with a very 
> limited set of committers to a particular tree, much smaller than the set 
> of reviewers - that a patch is worthy of commit).  It doesn't cover 
> reviews that request changes, or only relate to part of a patch, or relate 
> to a previous version of a patch - only the limited special case of a 
> review approving the entirety of a patch as posted.  If the aim is credit, 
> a substantially different specification is needed.
 
This is the purpose of Acked-by: ...

Which we could also include.

linux/Documentation/process/submitting-patches.rst
...

Acked-by: does not necessarily indicate acknowledgement of the entire patch.
For example, if a patch affects multiple subsystems and has an Acked-by: from
one subsystem maintainer then this usually indicates acknowledgement of just
the part which affects that maintainer's code.  Judgement should be used here.
When in doubt people should refer to the original discussion in the mailing
list archives.

...

-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

On 10/19/2017 09:45 AM, Joseph Myers wrote:
> On Thu, 19 Oct 2017, Thomas Schwinge wrote:
> 
>> 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>.
> 
> That wiki page refers to Reviewed-by as being about crediting reviewers.  
> But the specification appears to be oriented to something else entirely 
> (i.e. convincing a committer - in a Linux-kernel-like context with a very 
> limited set of committers to a particular tree, much smaller than the set 
> of reviewers - that a patch is worthy of commit).  It doesn't cover 
> reviews that request changes, or only relate to part of a patch, or relate 
> to a previous version of a patch - only the limited special case of a 
> review approving the entirety of a patch as posted.  If the aim is credit, 
> a substantially different specification is needed.
 
If a person is requesting changes, they should after accepting the changes,
submit a 'Reviewed-by:' tag or 'Acked-by:' tag to indicate they are happy
with the results?

-- 
Cheers,
Carlos.