Re: [MirageOS-devel] [PATCH v3 5/7] Add Code Review Guide

[Lars Kurth <lars.kurth.xen@xxxxxxxxx>]

> On 19 Dec 2019, at 09:56, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 18.12.2019 18:09, Lars Kurth wrote:
>> 
>> 
>> On 18/12/2019, 14:29, "Julien Grall" <julien@xxxxxxx> wrote:
>> 
>>    Hi Lars,
>> 
>>    On 12/12/2019 21:14, Lars Kurth wrote:
>>> +### Workflow from an Author's Perspective
>>> +
>>> +When code authors receive feedback on their patches, they typically first 
>>> try
>>> +to clarify feedback they do not understand. For smaller patches or patch 
>>> series
>>> +it makes sense to wait until receiving feedback on the entire series before
>>> +sending out a new version addressing the changes. For larger series, it may
>>> +make sense to send out a new revision earlier.
>>> +
>>> +As a reviewer, you need some system that he;ps ensure that you address all
>> 
>>    Just a small typo: I think you meant "helps" rather than "he;ps".
>> 
>>    Cheers,
>> 
>> Thank you: fixed in my working copy.
>> 
>> One thing which occurred to me for reviews like these, where there is no 
>> ACK's or Reviewed-by's is that I don't actually know whether you as reviewer 
>> is otherwise happy with the remainder of the patch.
>> Normally the ACKed-by or Reviewed-by is a signal that it is
>> 
>> I am assuming it is, but I think it may be worthwhile pointing this out in 
>> the document, that unless stated otherwise, the reviewer is happy with the 
>> patch
> 
> I don't think there should ever be such an implication. Afaic there
> are patches I comment upon, but that I either don't feel qualified
> to give an ack/R-b on, or that I simply don't want to for whatever
> reason. At best, no other comment (as in the above example) may be
> taken as "I don't object".


If that is the case, would there be a reasonable convention to make this clear? 

In a nutshell, in such a review the possible interpretations are
* I reviewed, but didn't feel qualified to do the rest
* I reviewed, but did not get round to give it full attention
* I reviewed, but before I make a final decision want to look at the next 
version
...
* I reviewed and don't object the rest
* I reviewed and agreed with the rest 
The latter two are equivalent to Ack/R-b

That is quite a large range of possibilities and kind of leaves the author 
guessing what state the review is in

Regards
Lars




_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/mirageos-devel