[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.