Mailing List Archive

Re: [PATCH v3 5/7] Add Code Review Guide
?On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> 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 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

Regards
Lars

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v3 5/7] Add Code Review Guide [ In reply to ]
On 18.12.2019 18:09, Lars Kurth wrote:
>
>
> ?On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> 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 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".

Jan

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v3 5/7] Add Code Review Guide [ In reply to ]
> On 19 Dec 2019, at 09:56, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.12.2019 18:09, Lars Kurth wrote:
>>
>>
>> ?On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> 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




_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v3 5/7] Add Code Review Guide [ In reply to ]
On 19.12.2019 11:03, Lars Kurth wrote:
>
>
>> On 19 Dec 2019, at 09:56, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 18.12.2019 18:09, Lars Kurth wrote:
>>>
>>>
>>> ?On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> 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-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api