Re: IIO : Clarifying review tags.

[Lars-Peter Clausen <lars@metafoo.de>]

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