IIO : Clarifying review tags.

IIO : Clarifying review tags.

[Jonathan Cameron]

Hi All,

The primary aim of this email is that I'd like to
improve the quality of credit that we are giving people
for the vital job or reviewing patches.

This is a quick(ish) email to try and clear up my interpretation
of when various review tags are appropriate.  This follows
on from a discussion as part of the kernel summit pre
discussion.  In particular, one issue that was raised there
was how to give credit where credit is due for reviews.

There are some proposals to add the option for patch
authors to explicitly credit anyone who pointed out bugs
in their earlier versions, but those may or may not come
to anything.  There may well be a formal statement from
the kernel summit (probably a clarification to
submittingpatches) but then there may be no clear opinion
and as with many kernel aspects it will be left to
individual maintainers.

Anyhow, another aspect was the uses various maintainers
are making of Acked-by vs Reviewed-by.  As such I thought
I would clarify my understanding (which has changed somewhat
as a result of those discussions).  Comments of course most
welcome!

Our standard tags;

Signed-off-by:
	This is part of the developer certificate of origin
stuff.  If you sign-off you are making an explicit statement
that either you wrote the code or it passed through your
hands and to the best of your knowledge you are not breaking
license terms etc (this includes me for example certifying
that I have made no changes that would invalidate earlier
sign offs) This is the one with some legal meaning
and I think we are all using it correctly (just here for
completeness).

Note that this also either implies that you have 'reviewed'
the code in detail, or that you are happy that the deep
review has already been done by someone you trust.
(and it's this last bit that lets the Linux development
process scale).  There is a pretty universal view that
you don't bother with both signed-off-by and any other tag
(though tested-by can be appropriate in my view).

Acked-by: 
	This is kind of a looks fine to me / I am happy with this
tag.  Has a couple of common uses:

1) Driver author / maintainer is saying they are happy with
someone else's change.  Note this includes me as IIO maintainer
saying I am happy for another maintainer to apply stuff to the
IIO tree (where cross subsystem series make this sensible).

2) For small changes where 'review' is not really relevant.
   e.g. Typo fixes.

Must be provided by the person who wishes to express this
opinion (note the difference from reviewed by below).

Tested-by:
	Does what is says on the tin.  If you have the hardware
and run a new patch to check it works then send a tested-by.
Note that it is absolutely encouraged to send a tested-by
and an ack/reviewed by.  I always want more tested-by:s as
they give me a warm fuzzy feeling about a patch. If someone
informally mentions they tested something I'll normally poke
them to give the tag as well.

Reviewed-by:
	Now this is the one that caused most discussion in the
previously mentioned thread.  When is it appropriate?

1) When substantial work has been applied as part of a review.
   This might be indicated by a series of suggested changes
   or by detailed comments on what they think is good / bad
   might be done differently.   In these cases the reviewer
   may 'suggest' the tag themselves.

2) Where earlier versions of a patch have received substantial
   review but the reviewer has not commented on the latest
   version.  In this case some maintainers have been giving
   'Reviewed-by' tags to people who haven't supplied them
   on their own.  This is the change I would like to make to
   how I have done things previously (where when I had time
   I pestered people to give a reviewed-by).

   A few of you delight in early stage reviews and I feel often
   get little or no permanent credit for your hard work.
   In particular there are some here who spend a lot of time
   helping new submitters get large patches in order.  A job
   that can take a huge amount of work on occasion!

   So does anyone object if I sometimes add Reviewed-by tags
   for them?  Obviously this is all going to be a little
   arbitrary so do keep sending them directly (and authors
   keep adding them to later versions of your patches!)
 
Anyhow, the thread that lead to this has yet again reminded
me of how lucky I am and we all are in IIO wrt to the quality
and quantity of reviewers who put huge amounts of time into
improving all our code!

I certainly know I would have burnt out long ago if it was
just me against the mountain.

I'm going to be on holiday week after next. Should get back
to normal in early August.

Thanks,

Jonathan

Re: IIO : Clarifying review tags.

[Hartmut Knaack]

Jonathan Cameron schrieb am 19.07.2015 um 17:19:
> Hi All,
> 
<...> 
> Our standard tags;
> 
> Signed-off-by:
> 	This is part of the developer certificate of origin
> stuff.  If you sign-off you are making an explicit statement
> that either you wrote the code or it passed through your
> hands and to the best of your knowledge you are not breaking
> license terms etc (this includes me for example certifying
> that I have made no changes that would invalidate earlier
> sign offs) This is the one with some legal meaning
> and I think we are all using it correctly (just here for
> completeness).
> 
> Note that this also either implies that you have 'reviewed'
> the code in detail, or that you are happy that the deep
> review has already been done by someone you trust.

Hi,
call me paranoid, but I would feel better if you would still
review in all cases. To load off some weight from your
shoulders I would recommend to change the workflow a bit in the
sense that you hold back on reviews until a patch has either
received an acked/reviewed-by tag or an appropriate amount of
time has passed (maybe a week). Then you should still do a very
suspicious review before taking in that change.
Thus, you could focus more on the details and would not have to
deal with early respins.

> (and it's this last bit that lets the Linux development
> process scale).  There is a pretty universal view that
> you don't bother with both signed-off-by and any other tag
> (though tested-by can be appropriate in my view).
> 
<...>

I'm fine with the Acked-by and Tested-by stuff.

> Reviewed-by:
> 	Now this is the one that caused most discussion in the
> previously mentioned thread.  When is it appropriate?
> 
> 1) When substantial work has been applied as part of a review.
>    This might be indicated by a series of suggested changes
>    or by detailed comments on what they think is good / bad
>    might be done differently.   In these cases the reviewer
>    may 'suggest' the tag themselves.
> 
> 2) Where earlier versions of a patch have received substantial
>    review but the reviewer has not commented on the latest
>    version.  In this case some maintainers have been giving
>    'Reviewed-by' tags to people who haven't supplied them
>    on their own.  This is the change I would like to make to
>    how I have done things previously (where when I had time
>    I pestered people to give a reviewed-by).
> 
>    A few of you delight in early stage reviews and I feel often
>    get little or no permanent credit for your hard work.
>    In particular there are some here who spend a lot of time
>    helping new submitters get large patches in order.  A job
>    that can take a huge amount of work on occasion!
> 
>    So does anyone object if I sometimes add Reviewed-by tags
>    for them?  Obviously this is all going to be a little
>    arbitrary so do keep sending them directly (and authors
>    keep adding them to later versions of your patches!)
>  

I'm in favor of using the Reviewed-by tag rather restrictive.
IMHO it expresses that the reviewer was able to understand the
change in its full extend. This may include to check all
device specific parameters used against a data sheet (if the
data sheet is not available due to NDA, then I would not want
to give this tag), check each line of code, check fixes against
the source code block it applies to (or even parent level
blocks if required). As a result, it should indicate that the
patch has successfully passed certain quality checks of the
reviewer. This in turn gives appreciaction to the patch author.
The reviewers reputation however can suffer, if significant
bugs slipped through the review. That's why I don't feel too
comfortable if anyone else gives reviewed-by tags in my name.
Clearly, there is a gap in between the acked-by and reviewed-by
tags, something that would express your contribution in the
development process.

> Anyhow, the thread that lead to this has yet again reminded
> me of how lucky I am and we all are in IIO wrt to the quality
> and quantity of reviewers who put huge amounts of time into
> improving all our code!
> 
> I certainly know I would have burnt out long ago if it was
> just me against the mountain.
> 
> I'm going to be on holiday week after next. Should get back
> to normal in early August.

Enjoy!

Hartmut

> 
> Thanks,
> 
> Jonathan
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: IIO : Clarifying review tags.

[Jonathan Cameron]

On 19 July 2015 20:54:33 BST, Hartmut Knaack <knaack.h@gmx.de> wrote:
>Jonathan Cameron schrieb am 19.07.2015 um 17:19:
>> Hi All,
>> 
><...> 
>> Our standard tags;
>> 
>> Signed-off-by:
>> 	This is part of the developer certificate of origin
>> stuff.  If you sign-off you are making an explicit statement
>> that either you wrote the code or it passed through your
>> hands and to the best of your knowledge you are not breaking
>> license terms etc (this includes me for example certifying
>> that I have made no changes that would invalidate earlier
>> sign offs) This is the one with some legal meaning
>> and I think we are all using it correctly (just here for
>> completeness).
>> 
>> Note that this also either implies that you have 'reviewed'
>> the code in detail, or that you are happy that the deep
>> review has already been done by someone you trust.
>
>Hi,
>call me paranoid, but I would feel better if you would still
>review in all cases. To load off some weight from your
>shoulders I would recommend to change the workflow a bit in the
>sense that you hold back on reviews until a patch has either
>received an acked/reviewed-by tag or an appropriate amount of
>time has passed (maybe a week). Then you should still do a very
>suspicious review before taking in that change.

I do still look at everything but will gloss over things like register maps if I know
 someone else has waded through them.

Also don't necessarily check things like
 whether all instances possible of a cleanup have been applied.

I also like others like to do an early review from time to time. They can be very rewarding!

My focus in early reviews is all structure and ABI (more or less).. Stuff that can
 save a lot of wasted time if we get it right early.
>Thus, you could focus more on the details and would not have to
>deal with early respins.

I do that a fair bit already if low on time :)

>
>> (and it's this last bit that lets the Linux development
>> process scale).  There is a pretty universal view that
>> you don't bother with both signed-off-by and any other tag
>> (though tested-by can be appropriate in my view).
>> 
><...>
>
>I'm fine with the Acked-by and Tested-by stuff.
>
>> Reviewed-by:
>> 	Now this is the one that caused most discussion in the
>> previously mentioned thread.  When is it appropriate?
>> 
>> 1) When substantial work has been applied as part of a review.
>>    This might be indicated by a series of suggested changes
>>    or by detailed comments on what they think is good / bad
>>    might be done differently.   In these cases the reviewer
>>    may 'suggest' the tag themselves.
>> 
>> 2) Where earlier versions of a patch have received substantial
>>    review but the reviewer has not commented on the latest
>>    version.  In this case some maintainers have been giving
>>    'Reviewed-by' tags to people who haven't supplied them
>>    on their own.  This is the change I would like to make to
>>    how I have done things previously (where when I had time
>>    I pestered people to give a reviewed-by).
>> 
>>    A few of you delight in early stage reviews and I feel often
>>    get little or no permanent credit for your hard work.
>>    In particular there are some here who spend a lot of time
>>    helping new submitters get large patches in order.  A job
>>    that can take a huge amount of work on occasion!
>> 
>>    So does anyone object if I sometimes add Reviewed-by tags
>>    for them?  Obviously this is all going to be a little
>>    arbitrary so do keep sending them directly (and authors
>>    keep adding them to later versions of your patches!)
>>  
>
>I'm in favor of using the Reviewed-by tag rather restrictive.
>IMHO it expresses that the reviewer was able to understand the
>change in its full extend. This may include to check all
>device specific parameters used against a data sheet (if the
>data sheet is not available due to NDA, then I would not want
>to give this tag), check each line of code, check fixes against
>the source code block it applies to (or even parent level
>blocks if required). As a result, it should indicate that the
>patch has successfully passed certain quality checks of the
>reviewer. This in turn gives appreciaction to the patch author.
>The reviewers reputation however can suffer, if significant
>bugs slipped through the review. That's why I don't feel too
>comfortable if anyone else gives reviewed-by tags in my name.
>Clearly, there is a gap in between the acked-by and reviewed-by
>tags, something that would express your contribution in the
>development process.

Interesting to hear. I will refrain from adding such tags given both your
 response and Dan's.  Dan suggested in the mega thread an explicit
 acknowledgement of a bug sported in a review. This would be applies as a kind of
 'thanks' from the author to later versions without carrying any weight wrt to the
 rest of the patch being good.

I was kind of fishing for opinions with this email. Good to get some!
>
>> Anyhow, the thread that lead to this has yet again reminded
>> me of how lucky I am and we all are in IIO wrt to the quality
>> and quantity of reviewers who put huge amounts of time into
>> improving all our code!
>> 
>> I certainly know I would have burnt out long ago if it was
>> just me against the mountain.
>> 
>> I'm going to be on holiday week after next. Should get back
>> to normal in early August.
>
>Enjoy!
Thanks
>
>Hartmut
>
>> 
>> Thanks,
>> 
>> Jonathan
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: IIO : Clarifying review tags.

[Daniel Baluta]

On Sun, Jul 19, 2015 at 6:19 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> Hi All,
>
> The primary aim of this email is that I'd like to
> improve the quality of credit that we are giving people
> for the vital job or reviewing patches.
>
> This is a quick(ish) email to try and clear up my interpretation
> of when various review tags are appropriate.  This follows
> on from a discussion as part of the kernel summit pre
> discussion.  In particular, one issue that was raised there
> was how to give credit where credit is due for reviews.
>
> There are some proposals to add the option for patch
> authors to explicitly credit anyone who pointed out bugs
> in their earlier versions, but those may or may not come
> to anything.  There may well be a formal statement from
> the kernel summit (probably a clarification to
> submittingpatches) but then there may be no clear opinion
> and as with many kernel aspects it will be left to
> individual maintainers.
>
> Anyhow, another aspect was the uses various maintainers
> are making of Acked-by vs Reviewed-by.  As such I thought
> I would clarify my understanding (which has changed somewhat
> as a result of those discussions).  Comments of course most
> welcome!
>
> Our standard tags;
>
> Signed-off-by:
>         This is part of the developer certificate of origin
> stuff.  If you sign-off you are making an explicit statement
> that either you wrote the code or it passed through your
> hands and to the best of your knowledge you are not breaking
> license terms etc (this includes me for example certifying
> that I have made no changes that would invalidate earlier
> sign offs) This is the one with some legal meaning
> and I think we are all using it correctly (just here for
> completeness).
>
> Note that this also either implies that you have 'reviewed'
> the code in detail, or that you are happy that the deep
> review has already been done by someone you trust.
> (and it's this last bit that lets the Linux development
> process scale).  There is a pretty universal view that
> you don't bother with both signed-off-by and any other tag
> (though tested-by can be appropriate in my view).
>
> Acked-by:
>         This is kind of a looks fine to me / I am happy with this
> tag.  Has a couple of common uses:
>
> 1) Driver author / maintainer is saying they are happy with
> someone else's change.  Note this includes me as IIO maintainer
> saying I am happy for another maintainer to apply stuff to the
> IIO tree (where cross subsystem series make this sensible).
>
> 2) For small changes where 'review' is not really relevant.
>    e.g. Typo fixes.

Agree with this.  I was under the impression that the Acked-by tag should
only be used by maintainers or original authors of that code.

>
> Must be provided by the person who wishes to express this
> opinion (note the difference from reviewed by below).
>
> Tested-by:
>         Does what is says on the tin.  If you have the hardware
> and run a new patch to check it works then send a tested-by.
> Note that it is absolutely encouraged to send a tested-by
> and an ack/reviewed by.  I always want more tested-by:s as
> they give me a warm fuzzy feeling about a patch. If someone
> informally mentions they tested something I'll normally poke
> them to give the tag as well.
>
> Reviewed-by:
>         Now this is the one that caused most discussion in the
> previously mentioned thread.  When is it appropriate?
>
> 1) When substantial work has been applied as part of a review.
>    This might be indicated by a series of suggested changes
>    or by detailed comments on what they think is good / bad
>    might be done differently.   In these cases the reviewer
>    may 'suggest' the tag themselves.
>
> 2) Where earlier versions of a patch have received substantial
>    review but the reviewer has not commented on the latest
>    version.  In this case some maintainers have been giving
>    'Reviewed-by' tags to people who haven't supplied them
>    on their own.  This is the change I would like to make to
>    how I have done things previously (where when I had time
>    I pestered people to give a reviewed-by).
>
>    A few of you delight in early stage reviews and I feel often
>    get little or no permanent credit for your hard work.
>    In particular there are some here who spend a lot of time
>    helping new submitters get large patches in order.  A job
>    that can take a huge amount of work on occasion!
>
>    So does anyone object if I sometimes add Reviewed-by tags
>    for them?  Obviously this is all going to be a little
>    arbitrary so do keep sending them directly (and authors
>    keep adding them to later versions of your patches!)

I am not sure if this is a good idea. Better would be to encourage
people to explicitly add Reviewed-by tags.

Patch authors could, however, explicitly add lines like this:

Cc: Little Penguin <little.penguin@linux.com>

starting with the second version of the patch, thus there is
some sort of credit given to people participating in the review.

>
> Anyhow, the thread that lead to this has yet again reminded
> me of how lucky I am and we all are in IIO wrt to the quality
> and quantity of reviewers who put huge amounts of time into
> improving all our code!
>
> I certainly know I would have burnt out long ago if it was
> just me against the mountain.
>
> I'm going to be on holiday week after next. Should get back
> to normal in early August.

Have fun! :)

thanks,
Daniel.

Re: IIO : Clarifying review tags.

[Lars-Peter Clausen]

On 07/19/2015 11:23 PM, Jonathan Cameron wrote:
[...]
>>> (and it's this last bit that lets the Linux development
>>> process scale).  There is a pretty universal view that
>>> you don't bother with both signed-off-by and any other tag
>>> (though tested-by can be appropriate in my view).
>>>
>> <...>
>>
>> I'm fine with the Acked-by and Tested-by stuff.
>>
>>> Reviewed-by:
>>> 	Now this is the one that caused most discussion in the
>>> previously mentioned thread.  When is it appropriate?
>>>
>>> 1) When substantial work has been applied as part of a review.
>>>     This might be indicated by a series of suggested changes
>>>     or by detailed comments on what they think is good / bad
>>>     might be done differently.   In these cases the reviewer
>>>     may 'suggest' the tag themselves.
>>>
>>> 2) Where earlier versions of a patch have received substantial
>>>     review but the reviewer has not commented on the latest
>>>     version.  In this case some maintainers have been giving
>>>     'Reviewed-by' tags to people who haven't supplied them
>>>     on their own.  This is the change I would like to make to
>>>     how I have done things previously (where when I had time
>>>     I pestered people to give a reviewed-by).
>>>
>>>     A few of you delight in early stage reviews and I feel often
>>>     get little or no permanent credit for your hard work.
>>>     In particular there are some here who spend a lot of time
>>>     helping new submitters get large patches in order.  A job
>>>     that can take a huge amount of work on occasion!
>>>
>>>     So does anyone object if I sometimes add Reviewed-by tags
>>>     for them?  Obviously this is all going to be a little
>>>     arbitrary so do keep sending them directly (and authors
>>>     keep adding them to later versions of your patches!)
>>>
>>
>> I'm in favor of using the Reviewed-by tag rather restrictive.
>> IMHO it expresses that the reviewer was able to understand the
>> change in its full extend. This may include to check all
>> device specific parameters used against a data sheet (if the
>> data sheet is not available due to NDA, then I would not want
>> to give this tag), check each line of code, check fixes against
>> the source code block it applies to (or even parent level
>> blocks if required). As a result, it should indicate that the
>> patch has successfully passed certain quality checks of the
>> reviewer. This in turn gives appreciaction to the patch author.
>> The reviewers reputation however can suffer, if significant
>> bugs slipped through the review. That's why I don't feel too
>> comfortable if anyone else gives reviewed-by tags in my name.
>> Clearly, there is a gap in between the acked-by and reviewed-by
>> tags, something that would express your contribution in the
>> development process.
>
> Interesting to hear. I will refrain from adding such tags given both your
>   response and Dan's.  Dan suggested in the mega thread an explicit
>   acknowledgement of a bug sported in a review. This would be applies as a kind of
>   'thanks' from the author to later versions without carrying any weight wrt to the
>   rest of the patch being good.

Yeah, I understand your reasoning, but I too feel a bit uncomfortable having 
other people add tags for me in my name. It's a bit like telling somebody to 
sign something in your name and forge your signature.

I'm a bit more easy when it comes to giving Reviewed-by tags than Hartmut 
though. If the driver looks sane from a API/ABI point I'll be ok with it. It 
is the driver author's task of making sure that the driver writes the right 
bits to the right registers.

>
> I was kind of fishing for opinions with this email. Good to get some!
>>
>>> Anyhow, the thread that lead to this has yet again reminded
>>> me of how lucky I am and we all are in IIO wrt to the quality
>>> and quantity of reviewers who put huge amounts of time into
>>> improving all our code!
>>>
>>> I certainly know I would have burnt out long ago if it was
>>> just me against the mountain.
>>>
>>> I'm going to be on holiday week after next. Should get back
>>> to normal in early August.

Enjoy your well deserved vacation and don't read/write so many emails ;)

- Lars