Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Hi Lars, On 18/12/2019 17:09, Lars Kurth wrote: On 18/12/2019, 14:29, "Julien Grall" 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 you can always assume this. There are case where I don't give a reviewed-by yet because I want to understand the follow-up patches first. I think what Ian described correspond the best to my view here. And I agree that we probably want to be more explicit in the review to avoid confusion. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Ian Jackson writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> > 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
...
> IOW: if you do not get an A-b or R-b, then the person writing is not
> necessarily making any of the statements you posit which start "I
> reviewed".
The other point it occurs to me to make here is that whether someone
reviewed something is not of formal importance to the process,
although it is of course very relevant information.
With my maintainer hat on I frequently approve patches which I have
not reviewed.
Likewise I sometimes read and even review in detail patches where I
have no formal authority. My comments are then intended (in a formal
sense) as input to the maintainers' decisionmaking.
(In practice, since we are all working together to make the best
software, maintainers generally expect a submitter to address issues
raised by a review from a non-maintainer, and strong objections from
non-maintainer stakeholders usually lead to a discussion about how to
resolve matters. This is all cooperation and common courtesy I
think.)
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> 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
There are only three possibilities:
Acked-by:
I hereby bless this with my maintainer powers.
I may or may not have reviewed it. The body text may contain
more information about that.
Reviewed-by:
I have reviewed this to my own satisfaction and this mail contains
all the comments I have on it. If I am a maintainer, I hereby
bless it with my maintainer powers. (This is a weird use of the
word "Reviewed" since in usual usage one can review something and
end up disapproving of it; nevertheless this is the convention.)
In both of the above cases, additionally
If my approval is conditional (eg on changes) this will be
stated explicitly in the body text of my message.
Neither of the above:
I have read at least some of this and felt motivated to make some
observations. If I have reviewed it properly this would usually be
stated in the body text. If I am taking enough of an interest in
this patch, the body text may also say what I think the current
state and/or next steps should be. In any case, I do *not* bless
this patch (in its current form) with any maintainer powers I may
have.
IOW: if you do not get an A-b or R-b, then the person writing is not
necessarily making any of the statements you posit which start "I
reviewed".
Having said that it is a good idea for people commenting on patches to
make clear what they have and haven't done. I often start a message
with "Thanks for this patch which I have reviewed. I have some
comments" or words to that effect.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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 wrote: >> >> On 18.12.2019 18:09, Lars Kurth wrote: >>> >>> >>> On 18/12/2019, 14:29, "Julien Grall" 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
> On 19 Dec 2019, at 09:56, Jan Beulich wrote: > > On 18.12.2019 18:09, Lars Kurth wrote: >> >> >> On 18/12/2019, 14:29, "Julien Grall" 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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
On 18.12.2019 18:09, Lars Kurth wrote: > > > On 18/12/2019, 14:29, "Julien Grall" 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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
On 18/12/2019, 14:29, "Julien Grall" 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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
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, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
