Process for reviewing patches as someone without commit access

Process for reviewing patches as someone without commit access

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

Hey!

I've been reviewing the list of ideas on and around QA ([1]) recently,
and got thinking again about how to support people without commit access
reviewing patches. Obviously you don't need commit access to review
patches, but where I think we need some process is how to expedite
someone with commit access taking a look at the patches that have been
reviewed, and merging them if appropriate.

1: https://qa.guix.gnu.org/README

Maybe we can use debbugs tags for this? It looks like this has already
been done in the past, I found some issues tagged with the usertag
"reviewed" for example [1]. Some were also tagged with
"reviewed-looks-good". I guess my primary concern is to have a tag (or
combination of tags) which indicate that a committer should have a look
at the issue as it's been reviewed by someone and should be ready to
merge. I don't really use debbugs much, but does anyone have any
opinions on appropriate tags?

1: https://debbugs.gnu.org/cgi-bin/pkgreport.cgi?tag=reviewed&users=guix

Once we know what tags to use, I can have the QA frontpage do something
similar to the "Mark as moreinfo" links, so it's easy to just click a
button then send the email to change the state of a issue.

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

Re: Process for reviewing patches as someone without commit access

[Felix Lechner via Development of GNU Guix and the GNU System distribution.]

Hi Chris,

On Wed, Sep 6, 2023 at 9:47 AM Christopher Baines <mail@cbaines.net> wrote:
>
> Maybe we can use debbugs tags for this?

Instead of pushing people into reviews and then again making the same
committers a bottleneck, I would offer some entry-level contributors
commit rights but require that they obtain approval for some steps. It
can be done on a trust basis.

That way, you can train a new generation of committers while getting
the work done.

Advancing to a higher level requires a majority approval of the
maintainer collective. Same for revocations.

*** Contributor levels ***

1. Add new packages or fix documentation. This simple activity focuses
on fitness of software for Guix, including licensing. Finding a good
place in the file tree is also part of this job. Sign off required.

Novice levels, all require review by someone with a higher level

2. Update existing packages. Here, a contributor must be aware of how
updates affect consuming packages. Ideally, this activity would
include knowledge of CI, including the pre-compilation of all
consumers in order to avoid build failures before the change is
committed. No new inputs or phases. Hash updates only.
3. Modify inputs and build phases. Requires the mastery of
G-Expressions and a detailed knowledge of the particular build system
involved.
4. Rename package variables or change inheritance. This is for experts
in tool chains like Golang, or a technology like Emacs.

Intermediate levels:

5. Add new services
6. Edit existing services (after review)
7. Change Guix records outside of services, for example for
operating-system (after review).
https://lepiller.eu/en/a-deep-dive-into-guix-records.html

Advanced levels

8. Modify command-line behavior of the Guix executable (after review)
9. Change the Guix daemon (after review)

Black belt

10. No restrictions, and no reviews or approvals needed.

Kind regards
Felix

Re: Process for reviewing patches as someone without commit access

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]


Felix Lechner <felix.lechner@lease-up.com> writes:

> On Wed, Sep 6, 2023 at 9:47 AM Christopher Baines <mail@cbaines.net> wrote:
>>
>> Maybe we can use debbugs tags for this?
>
> Instead of pushing people into reviews and then again making the same
> committers a bottleneck, I would offer some entry-level contributors
> commit rights but require that they obtain approval for some steps. It
> can be done on a trust basis.

It's an idea, although one I'd discount based on how many breaking
changes (including ones with wide impact like breaking guix pull) happen
with the current criteria for granting commit access.

I don't want to make reviewing changes more difficult, and I think
setting up more people with commit access and continuing the trend that
it's mostly people with commit access that review changes would increase
the difficulty, compared to what I'm proposing here, which is trying to
empower people who just do review whilst avoiding any of the complexity
of merging and pushing the changes without breaking things.

Now of course you could argue that it being easy to break things is a
problem, and maybe it is, but often it's not strictly someone breaking
something but simply a commit not being signed, or not being signed in a
way that guix accepts. Here I think pushing changes is complicated for
good reason.

> That way, you can train a new generation of committers while getting
> the work done.

In my opinion, I want to see review and committing things become more
separated, because the valuable thing is having good review. Committing
and pushing someone elses changes isn't adding much value to Guix in and
of itself, but good review of those changes does.

If we end up with a big backlog of changes that are reviewed and ready
to merge, then I'm all for training and helping more people do the
committing and pushing, but I have a suspicion that it's the review bit
that takes the time.

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

Re: Process for reviewing patches as someone without commit access

[Felix Lechner via Development of GNU Guix and the GNU System distribution.]

Hi Chris,

On Wed, Sep 6, 2023 at 11:39 AM Christopher Baines <mail@cbaines.net> wrote:
>
> I don't want to make reviewing changes more difficult, and I think
> setting up more people with commit access and continuing the trend that
> it's mostly people with commit access that review changes would increase
> the difficulty, compared to what I'm proposing here, which is trying to
> empower people who just do review whilst avoiding any of the complexity
> of merging and pushing the changes without breaking things.

Usually, helpful reviews come from people with more experience. In
your system they come from folks with less. That seems upside down to
me.

More significantly, work gets done faster when people are motivated. A
junior committer who is about to push a change will be much more eager
to find an experienced reviewer. It's a way to demonstrate knowledge.
The reviewer's testimony will eventually help the contributor attain a
higher level.

In other words, there is no empowerment in "just do[ing] a review".

Kind regards
Felix

Re: Process for reviewing patches as someone without commit access

[Simon Tournier]

Hi Felix,

On Wed, 06 Sep 2023 at 12:01, Felix Lechner via "Development of GNU Guix and the GNU System distribution." <guix-devel@gnu.org> wrote:

> Usually, helpful reviews come from people with more experience. In
> your system they come from folks with less. That seems upside down to
> me.

Well, I think we are not there.  Maybe I am missing the proposal about
levels.  Somehow, I think there is not enough regular contributors for
having kind of levels.  Well, that’s not Debian. ;-)

Aside that considering how Savannah is configured today, commit access
are all or nothing.

Cheers,
simon

Re: Process for reviewing patches as someone without commit access

[Simon Tournier]

Hi Chris, all,

On Wed, 06 Sep 2023 at 16:55, Christopher Baines <mail@cbaines.net> wrote:

> Once we know what tags to use, I can have the QA frontpage do something
> similar to the "Mark as moreinfo" links, so it's easy to just click a
> button then send the email to change the state of a issue.

That’s cool!

Well, using emacs-debbugs and then

    C-u M-x debbugs-gnu-usertags guix-patches RET

the list of usertags is:

        guix-patches  for-core-updates
        guix-patches  reviewed-looks-good

And if instead of guix-patches we consider guix then it reads,

        guix  build-system
        guix  cross-compilation
        guix  for-core-updates
        guix  looks-good
        guix  patch
        guix  plz-work
        guix  powerpc64le-linux
        guix  ready-to-review
        guix  reproducibility
        guix  reviewed
        guix  reviewed-looks-good
        guix  test-tag
        guix  v1.3.0

However, I do not know how to list all the bugs for the package
guix-patches that matches the usertag reviewed-looks-good.  Anyway!

I think that the usertag ’reviewed’ is a good idea.  That would be a
very good start.  Then if it helps, we could add other usertags as
reviewed-julia for patches that the Julia team can merge.

Discussing about idea, would it be possible that the QA infrastructure
automatically send a message to Debbugs for tagging?  For example, the
usertag ’qa-ok’ or whatever other meaningful name. :-)

Well, we can start with one usertag for only the architecture x86_64.
If this one is green, then it is worth to start the review.  It would
help for filtering.

The slippery slope is to have too much usertags.  I think we should
start with one or two usertags and see if it helps.

WDYT?

Cheers,
simon

Re: Process for reviewing patches as someone without commit access

[Maxim Cournoyer]

Hi Simon, Chris,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Chris, all,
>
> On Wed, 06 Sep 2023 at 16:55, Christopher Baines <mail@cbaines.net> wrote:
>
>> Once we know what tags to use, I can have the QA frontpage do something
>> similar to the "Mark as moreinfo" links, so it's easy to just click a
>> button then send the email to change the state of a issue.
>
> That’s cool!
>
> Well, using emacs-debbugs and then
>
>     C-u M-x debbugs-gnu-usertags guix-patches RET
>
> the list of usertags is:
>
>         guix-patches  for-core-updates
>         guix-patches  reviewed-looks-good
>
> And if instead of guix-patches we consider guix then it reads,
>
>         guix  build-system
>         guix  cross-compilation
>         guix  for-core-updates
>         guix  looks-good
>         guix  patch
>         guix  plz-work
>         guix  powerpc64le-linux
>         guix  ready-to-review
>         guix  reproducibility
>         guix  reviewed
>         guix  reviewed-looks-good
>         guix  test-tag
>         guix  v1.3.0
>
> However, I do not know how to list all the bugs for the package
> guix-patches that matches the usertag reviewed-looks-good.  Anyway!

Clicking on an entry in the above list shows them; I'm sure we could
define a procedure to directly show the bugs associated with a usertag,
which would be useful.

> I think that the usertag ’reviewed’ is a good idea.  That would be a
> very good start.  Then if it helps, we could add other usertags as
> reviewed-julia for patches that the Julia team can merge.

+1 for the 'reviewed' usertag (using the 'guix' user).

> Discussing about idea, would it be possible that the QA infrastructure
> automatically send a message to Debbugs for tagging?  For example, the
> usertag ’qa-ok’ or whatever other meaningful name. :-)

+1 for that as well, maybe 'qa-passed'.

-- 
Thanks,
Maxim

Re: Process for reviewing patches as someone without commit access

[Simon Tournier]

Hi,

On Wed, 06 Sep 2023 at 22:47, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>> Well, using emacs-debbugs and then
>>
>>     C-u M-x debbugs-gnu-usertags guix-patches RET
>>
>> the list of usertags is:
>>
>>         guix-patches  for-core-updates
>>         guix-patches  reviewed-looks-good
>>
>> And if instead of guix-patches we consider guix then it reads,
>>
>>         guix  build-system
>>         guix  cross-compilation
>>         guix  for-core-updates
>>         guix  looks-good
>>         guix  patch
>>         guix  plz-work
>>         guix  powerpc64le-linux
>>         guix  ready-to-review
>>         guix  reproducibility
>>         guix  reviewed
>>         guix  reviewed-looks-good
>>         guix  test-tag
>>         guix  v1.3.0

> Clicking on an entry in the above list shows them; I'm sure we could
> define a procedure to directly show the bugs associated with a usertag,
> which would be useful.

Thanks.

For instance, clicking on

        guix          reviewed

leads to,

55997 normal,pat Tom Fitzhenry      [PATCH staging 0/4] Remove texlive's dependence on mariadb.
55769 normal,pat <derekchuank@outlo [PATCH] gnu: Add xwhite.
49672 normal,pat Thiago Jung Bauerm [PATCH 0/9] Target check fixes and cleanups

where #55997 and #55769 are marked as done.  How do you filter for
listing only the ones not marked as done?

Maybe, we could also automatically remove the usertag 'reviewed' when
the status is marked as 'done'.  Hum, I do not know, WDYT?


Cheers,
simon

Re: Process for reviewing patches as someone without commit access

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]

On 2023-09-06, Maxim Cournoyer wrote:
> Simon Tournier <zimon.toutoune@gmail.com> writes:
>> On Wed, 06 Sep 2023 at 16:55, Christopher Baines <mail@cbaines.net> wrote:
>>
>>> Once we know what tags to use, I can have the QA frontpage do something
>>> similar to the "Mark as moreinfo" links, so it's easy to just click a
>>> button then send the email to change the state of a issue.
>>
>> That’s cool!
>>
>> Well, using emacs-debbugs and then
>>
>>     C-u M-x debbugs-gnu-usertags guix-patches RET
>>
>> the list of usertags is:
>>
>>         guix-patches  for-core-updates
>>         guix-patches  reviewed-looks-good
>>
>> And if instead of guix-patches we consider guix then it reads,
>>
>>         guix  build-system
>>         guix  cross-compilation
>>         guix  for-core-updates
>>         guix  looks-good
>>         guix  patch
>>         guix  plz-work
>>         guix  powerpc64le-linux
>>         guix  ready-to-review
>>         guix  reproducibility
>>         guix  reviewed
>>         guix  reviewed-looks-good
>>         guix  test-tag
>>         guix  v1.3.0
>>
>> However, I do not know how to list all the bugs for the package
>> guix-patches that matches the usertag reviewed-looks-good.  Anyway!
>
> Clicking on an entry in the above list shows them; I'm sure we could
> define a procedure to directly show the bugs associated with a usertag,
> which would be useful.
>
>> I think that the usertag ’reviewed’ is a good idea.  That would be a
>> very good start.  Then if it helps, we could add other usertags as
>> reviewed-julia for patches that the Julia team can merge.
>
> +1 for the 'reviewed' usertag (using the 'guix' user).
>
>> Discussing about idea, would it be possible that the QA infrastructure
>> automatically send a message to Debbugs for tagging?  For example, the
>> usertag ’qa-ok’ or whatever other meaningful name. :-)
>
> +1 for that as well, maybe 'qa-passed'.

Overall, I like this!

The only concern is that it is maintaining QA information in multiple
places, the QA service itself, and trying to mirror it to the bug
tracker...

QA might fail with a later patch iterations, so it should also then go
and remove the qa-passed tag? Should it remove the tag when QA is
currently in an unknown state?

live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: Process for reviewing patches as someone without commit access

[Simon Tournier]

Hi,

There are only two hard things in Computer Science…

On Thu, 07 Sep 2023 at 09:19, Vagrant Cascadian <vagrant@debian.org> wrote:

>>> Discussing about idea, would it be possible that the QA infrastructure
>>> automatically send a message to Debbugs for tagging?  For example, the
>>> usertag ’qa-ok’ or whatever other meaningful name. :-)
>>
>> +1 for that as well, maybe 'qa-passed'.

…naming and…

> The only concern is that it is maintaining QA information in multiple
> places, the QA service itself, and trying to mirror it to the bug
> tracker...

…cache invalidation. :-)


> QA might fail with a later patch iterations, so it should also then go
> and remove the qa-passed tag? Should it remove the tag when QA is
> currently in an unknown state?

That’s good questions.  Yeah, maybe QA could check when starting to
build that there is no ’qa-passed’ or remove it if any.

Cheers,
simon

Re: Process for reviewing patches as someone without commit access

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 2478 bytes --]


Christopher Baines <mail@cbaines.net> writes:

> I've been reviewing the list of ideas on and around QA ([1]) recently,
> and got thinking again about how to support people without commit access
> reviewing patches. Obviously you don't need commit access to review
> patches, but where I think we need some process is how to expedite
> someone with commit access taking a look at the patches that have been
> reviewed, and merging them if appropriate.
>
> 1: https://qa.guix.gnu.org/README
>
> Maybe we can use debbugs tags for this? It looks like this has already
> been done in the past, I found some issues tagged with the usertag
> "reviewed" for example [1]. Some were also tagged with
> "reviewed-looks-good". I guess my primary concern is to have a tag (or
> combination of tags) which indicate that a committer should have a look
> at the issue as it's been reviewed by someone and should be ready to
> merge. I don't really use debbugs much, but does anyone have any
> opinions on appropriate tags?
>
> 1: https://debbugs.gnu.org/cgi-bin/pkgreport.cgi?tag=reviewed&users=guix
>
> Once we know what tags to use, I can have the QA frontpage do something
> similar to the "Mark as moreinfo" links, so it's easy to just click a
> button then send the email to change the state of a issue.

I've gone ahead and implemented an initial version of this. On the page
for an issue now there's a form to mark patches as reviewed. This
replaces the previous review checklist and notes field.

That form then takes you to a page to submit the review. This can be
done through the mailto link, or by following the manual
instructions. Of course people using other ways of interacting with
debbugs can also just add the reviewed-looks-good usertag for the guix
user.

Once QA has picked up that the reviewed-looks-good usertag is present,
it'll display a dark green status and the issue will appear at the top
of the /patches page. My hope here is that people with commit access can
prioritise merging patches that have been reviewed and should be ready
to merge.

The implementation is a bit iffy, Mumi doesn't have access to the
usertags as far as I can tell, so I'm getting them from the debbugs soap
interface (luckily Ricardo has already figured that out), but it gives
you every issue tagged for every usertag for that user, which probably
won't scale well if lots of people start reviewing patches. But if that
happens we can always look at how to address it.

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

Re: Process for reviewing patches as someone without commit access

[Ludovic Courtès]

Hello,

Christopher Baines <mail@cbaines.net> skribis:

> That form then takes you to a page to submit the review. This can be
> done through the mailto link, or by following the manual
> instructions. Of course people using other ways of interacting with
> debbugs can also just add the reviewed-looks-good usertag for the guix
> user.
>
> Once QA has picked up that the reviewed-looks-good usertag is present,
> it'll display a dark green status and the issue will appear at the top
> of the /patches page. My hope here is that people with commit access can
> prioritise merging patches that have been reviewed and should be ready
> to merge.

Very nice, thank you!

Ludo’.