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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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

Carlos O'Donell-6
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.
Reply | Threaded
Open this post in threaded view
|

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

Thomas Schwinge-8
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 <[hidden email]> wrote:
> > On 09/21/2017 11:56 AM, Richard Biener wrote:
> > > On Thu, 21 Sep 2017 11:38:29 -0600, Carlos O'Donell <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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
[hidden email]
Reply | Threaded
Open this post in threaded view
|

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

Carlos O'Donell-6
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.