Re: Coordinators for patch review session on Tuesday

[Steve George]

On  4 Apr, Christina O'Donnell wrote:
> Hi,
> 
> Thanks for your reply,
> 
> > 1. Changing the tag to reviewed-looks-good
> > 
> > It doesn't look like this worked. The way to do this is in the instructions 
> > are 4. 'Set a user tag' [0], probably the easiest way is to send an email 
> > (I do get funny results sometimes with my email client):
> > 
> > Subject: setting usertag on 65938
> > 
> > user guix
> > usertag 65938 + reviewed-looks-good
> > quit
> > 
> > The first line is important it has to be 'user guix' for it to appear on 
> > the patch review reports [1]. I think I messed up the instructions in the 
> > Wiki - you have to have a + in between the bug number and the tag you want 
> > to set (sorry about that). Please try again.
> 
> Ah I got it this time. I was missing the 'user guix'. I didn't read the wiki
> and tried to look it up from the debbugs documentation.
> 
> > This is really just a way of signalling that reviews are happening - so 
> > trying to keep us in sync. The usertags we're using are:
> > 
> > - patch-review-hackers-list
> > - under-review
> > - escalated-review-request
> > - waiting-on-contributor
> > - reviewed-looks-good
> If I change the patch quite a lot, should I mark it as
> 'escalated-review-request' instead of 'reviewed-looks-good'?

Since a maintainer will look at it anyway before it's committed, and you are 
going to 're-roll' it as a v2 (which they will see) I wouldn't bother. I've 
been using 'escalated-review-request' for (a) ones I see that are just too hard 
for me, (b) if I personally think there's some outstanding issue with the patch 
that I can't solve.

> 
> And should I remove them from the patch-review-hackers-list after I've
> responded

Yes please - if you remove it from that list, and you make yourself the owner 
then I hope we'll keep people co-ordinated!

> > The patch changes all look reasonable to me, you've already done a lot:
> Great, thanks! Good to know I'm doing things vaguely right!
> > 1. You should add a reviewed-by trailer:
> > Reviews are contributions from our community (and work!) so we should 
> > recognise them and add trailers. It also helps the maintainer know who did 
> > the review and therefore the level of confidence.
> > 
> > Basically just add 'Reviewed-by: A Person <a@email.com> - [2]
> Sure, do you want me resubmit these patches to add that?

No - I think you're all good, it looked they were both going to the build 
system last night.


> > It looks like your updated patch retriggered QA, so if you look here and 
> > the foolow the Data Service link on the right you can see it's building it:
> > 
> >    https://qa.guix.gnu.org/issue/65938
> > 
> > The last step will be for a maintainer to see that it's built correctly, 
> > see your review and to apply it - great job for a first patch review!
> Wonderful! The first of many, I'm hoping.
(...)

Keep an eye on them in QA, and see if a maintainer sees them. If nothing 
happens after 10 days consider pinging on IRC.

Thanks,

Steve