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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.12.2019 11:03, Lars Kurth wrote:
> 
> 
>> 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

Well, I though the convention is to give A-b / R-b explicitly. In
a few overly ambiguous cases we tend to simply ask back whether a
given reply can be transformed into a tag. I don't see any need
for further formalization here.

Jan

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