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

Re: [MirageOS-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" <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 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

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

 


Rackspace

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