[bug#61894] [PATCH RFC] Team approval for patches

[bug#61894] [PATCH RFC] Team approval for patches

[Ludovic Courtès]

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

Hello Guix!

The project has been steadily gaining new contributors, which is great,
and I think we need to adjust our processes accordingly.

Currently teams are described mostly as pools of people who can mentor
contributors in a particular area and who can review patches in that
area.  My proposal is to give teams formal approval power over changes
to code in their area.

This is sorta happening already, but informally: if a non-committer
sends a patch, someone from the team eventually “approves” it by pushing
it.  Within a team, the situation is different: people usually discuss
changes, and the submitter (also committer) eventually pushes them;
sometimes, the submitter pushes changes without getting approval (or
feedback) from others on the team.

With the proposed policy, members of a team would also have to review
and approve each other’s work.  Formal approval means getting an
explicit “LGTM” (or similar) from at least one other team member.

This is similar to the review thresholds found on GitLab & co., where
project admins can specify a minimum number of approvals required before
a change is marked as ready.  I think it avoids the unavoidable
misunderstandings that can arise in a growing group and help pacify
day-to-day collaboration.

Below is a suggested change.

What do people think?

Ludo’.


[-- Attachment #2: Type: text/x-patch, Size: 2042 bytes --]

diff --git a/doc/contributing.texi b/doc/contributing.texi
index c436bc4a31..d8ca6802c4 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -1486,7 +1486,7 @@ reply to incoming bugs and patches, which contains the bug number.
 @anchor{Notifying Teams}
 @cindex teams
 The @file{etc/teams.scm} script may be used to notify all those who
-may be interested in your patch of its existence (@pxref{Teams}).
+may be interested in your patch and may approve it (@pxref{Teams}).
 Use @command{etc/teams.scm list-teams} to display all the teams,
 decide which team(s) your patch relates to, and use
 @command{etc/teams.scm cc} to output various @command{git send-email}
@@ -1557,6 +1557,9 @@ these changes are necessary.
 @subsection Teams
 @cindex teams
 
+The project is structured as @dfn{teams}, which play two related roles:
+mentoring people who contribute code in their area of expertise, and
+reviewing and approving changes to that code.
 There are several teams mentoring different parts of the Guix source
 code.  To list all those teams, you can run from a Guix checkout:
 
@@ -1840,8 +1843,14 @@ Patches}).  It also allows patches to be picked up and tested by the
 quality assurance tooling; the result of that testing eventually shows
 up on the dashboard at
 @indicateurl{https://qa.guix.gnu.org/issue/@var{ISSUE_NUMBER}}, where
-@var{ISSUE_NUMBER} is the number assigned by the issue tracker.  Leave
-time for a review, without committing anything (@pxref{Submitting
+@var{ISSUE_NUMBER} is the number assigned by the issue tracker.
+
+When your patch falls under the area of expertise of a team
+(@pxref{Teams}), you need the explicit approval of at least one team
+member before committing (another team member if you are on the team).
+
+In other cases,
+leave time for a review, without committing anything (@pxref{Submitting
 Patches}).  If you didn’t receive any reply after one week (two weeks
 for more significant changes), and if you're confident, it's OK to
 commit.

[bug#61894] [PATCH RFC] Team approval for patches

[Felix Lechner via Guix-patches via]

Hi,

On Wed, Mar 1, 2023 at 9:31 AM Christopher Baines <mail@cbaines.net> wrote:
>
> I'm unclear how it would
> impact the things I push for others. I pushed some patches today, would
> this mean that I'd have to look at what team/teams are involved
> (according to /etc/teams.scm.in) for each commit/series, and then either
> continue if I'm a member of that team, or skip it if I'm not?

Perhaps a compromise would be to ask committers to get a second
opinion from another committer whenever they feel it is necessary.

A committer who is confident enough, however, would be encouraged to
sidestep the restriction.

This guidance would gently bump the perceived penalty for a misstep,
because ignorance was then part of the mix when an error occurred.

The second person will often be from an affected team, but sometimes
they won't. That would only need to be revisited when there was a
problem. Otherwise, it was water under the bridge.

A softer guidance would also allow the project to experiment gradually
with greater checks and balances.

After some time, the committers would be able to weigh—both
individually as well as collectively—whether the additional rules
actually provided the benefits they were designed to produce.

Kind regards
Felix Lechner

[bug#61894] [PATCH RFC] Team approval for patches

[Ludovic Courtès]

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> I guess I'm still a team sceptic, I think the idea is interesting and I
> have added myself as a member of some teams. But the main impact on me
> so far is that I've just been getting some unwanted personal email,
> messages that previously wouldn't have landed in my inbox have been
> doing so.

Same for me (took me a while to understand why I was suddenly Cc’d on
some many patches.  :-))  I’m not sure how to improve on that.

> Regarding this change specifically though, I'm unclear how it would
> impact the things I push for others. I pushed some patches today, would
> this mean that I'd have to look at what team/teams are involved
> (according to /etc/teams.scm.in) for each commit/series, and then either
> continue if I'm a member of that team, or skip it if I'm not?
>
> If I'm going to not be pushing stuff I would have previously pushed
> because I'm not in the relevant teams, maybe I should just add myself to
> every team? I guess this is not a serious question, but I'm more making
> the point that if teams become a formal part of patch review, then some
> formalities over membership of a team is probably a prerequsite.
>
> As a point of clarification, if a patch or series touches files that
> fall within the scope of several teams, am I correct in saying that this
> change would require approval from all teams?

Good questions.

For teams like ‘core’ or ‘home’, there should be no overlap, so it’s
quite easy to see who’s in charge.

Teams related to packages are more likely to overlap, and it’s also an
area where we generally want more flexibility.  The example you
give—pushing patches even though you’re not on the corresponding
team(s)—is something we’d still want to allow most of the time.

There seems to be different requirements depending on teams.  I’d like
more coordination and clearer responsibilities for subsystems (guix/*,
gnu/{services,system,build}/*, etc.) and key packages/tools (Python,
ocaml-build-system, etc.).  For “random packages”, I’m fine with the
status quo.

WDYT?

Thanks,
Ludo’.

[bug#61894] [PATCH RFC] Team approval for patches

[Andreas Enge]

Hello,

in the current situation I think the suggestion is putting the horse before
the cart. In a first step before adding policy, we should make the teams
functional. While working on core-updates, I have been realising we are
already spread too thin: Some important languages have teams with one or
two members, who would effectively become bottlenecks. Other software has
no team (Qt/KDE). All in all, I also think we have too few committers.
Adding policy might completely stall the project...

If for every trivial update of a Python package we need not only submit a
patch to the bugtracker, wait for QA, get back to the patch, resign it,
push it and close the bug, but additionally wait for one of the two Python
team members to have a look at it (or let an additional week pass),
incentives to participate will tend to zero.

Your suggested policy can help against commits of too bad quality; but I
do not think this is our problem, our problem is rather a lack of fast
progress.

So I think we need to add committers, add committers to teams, encourage
teams to engage in work, and if everything works smoothly, maybe add policy.

Andreas

[bug#61894] [PATCH RFC] Team approval for patches

[宋文武 via Guix-patches via]

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Chris,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> Regarding this change specifically though, I'm unclear how it would
>> impact the things I push for others. I pushed some patches today, would
>> this mean that I'd have to look at what team/teams are involved
>> (according to /etc/teams.scm.in) for each commit/series, and then either
>> continue if I'm a member of that team, or skip it if I'm not?
>>
>> If I'm going to not be pushing stuff I would have previously pushed
>> because I'm not in the relevant teams, maybe I should just add myself to
>> every team? I guess this is not a serious question, but I'm more making
>> the point that if teams become a formal part of patch review, then some
>> formalities over membership of a team is probably a prerequsite.
>>
> [...]
>
> Good questions.
>
> For teams like ‘core’ or ‘home’, there should be no overlap, so it’s
> quite easy to see who’s in charge.
> [...]
> For “random packages”, I’m fine with the status quo.

Hello, I'd like to know if I'm working fine according to the status
quo..

I usually push patches for others who don't have commit access, while
most packages don't have a team at all, and some with me as the only
team member.

Should I wait for another commiter's approvol under this new policy or
can I push "random packages" (eg: jwm) solo under the status quo?  For
packages I as the only team member (eg: fcitx), should I looking for
another commiter for other's patches and my patches?

Thank you!

[bug#61894] [PATCH RFC] Team approval for patches

[Maxim Cournoyer]

Hi Simon,

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

> Hi,
>
> On Tue, 07 Mar 2023 at 11:36, Andreas Enge <andreas@enge.fr> wrote:
>
>> 1) Every current and potential new package is covered by a team.
>> 2) Every team has at least 3 members, better yet 4 or 5.
>>    3 members would make it possible that even if one of them is on vacation
>>    or otherwise busy a patch could be pushed without this additional one
>>    week if the other 2 agree.
>
> It would help if being committer implies appearing at least in one team,
> no?
>
> Currently in etc/teams.scm.in, I count 26 members and 20 are committers
> over the 48 ones.  No blame. :-)

If most committers end up being team members, aren't we back to where we
currently stand?  It seems the original motivation here is to add some
extra control/guards against undesirable commits landing in the core of
Guix.  If a committer that previously landed such commits joined the
core team (e.g., myself), it seems to me the situation would be little
changed:

1. Our pool of reviewers would likely continue to be spread too thin.

2. The 2 weeks time window would quickly slip, even with a team looking
at a more focused backlog, or the reviews would only be of the kind "I
think that's not what we want" without more time or energy to offer the
kind of concrete insights that can be turned into action for the
submitter.

3. The team member might be tempted to take their chance and merge their
change with little to no feedback, or feedback they perceived
insufficient or not actionable enough to justify keeping their
submission in limbo for longer.

I think the main problem we have is social, not organizational.  There's
little incentive to jump into the laborious review process compared to
hack on something we like in our free time.  We need to promote and
value review work more, without making it feel like a compulsory chore.
That's a great challenge to solve for a project that's driven by
volunteers.

I'll venture a suggestion to explore: adding enticements to review (some
playful guidelines such as "while waiting for your 2 weeks review
period, please try to review twice as many other submissions that have
been patiently waiting on the patches tracker :-)", or some stats
crunched and advertised periodically to guix-devel or even our to our
blog about our top reviewers, etc.).

-- 
Maxim

[bug#61894] [PATCH RFC] Team approval for patches

[Leo Famulari]

I don't have a strong opinion one way or the other about whether we
should formalize the review process. The status quo isn't working well,
so I'm in favor of trying something.

On Tue, Mar 07, 2023 at 01:29:51PM -0500, Maxim Cournoyer wrote:
> I think the main problem we have is social, not organizational.  There's
> little incentive to jump into the laborious review process compared to
> hack on something we like in our free time.  We need to promote and
> value review work more, without making it feel like a compulsory chore.
> That's a great challenge to solve for a project that's driven by
> volunteers.

However, I agree with this point wholeheartedly. We really need to ask
ourselves, why would anyone review patches? It's a lot of work, often
thankless, and unfortunately sometimes unpleasant.

> I'll venture a suggestion to explore: adding enticements to review (some
> playful guidelines such as "while waiting for your 2 weeks review
> period, please try to review twice as many other submissions that have
> been patiently waiting on the patches tracker :-)", or some stats
> crunched and advertised periodically to guix-devel or even our to our
> blog about our top reviewers, etc.).

In release announcements, alongside to the the normal `git shortlog`
list of authors, I suggest also publicizing the list of committers:

`git shortlog --numbered --summary --committer v1.4.0..HEAD`

A small thing, but hopefully one of many incentives to review and
commit.

[bug#61894] [PATCH RFC] Team approval for patches

[Simon Tournier]

Hi,

On Wed, 08 Mar 2023 at 13:58, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> number of their R package updates (thanks!)).  It seems starting to use
> the 'Reviewed-by' git message tag would make this easy to account for.

Quoting from thread [1]:

        I agree that Reviewed-by would be helpful.

        Once reviewed, the author or the reviewer could roll the count and
        re-send the patch (or series), applying the line Reviewed-by.

        It would reward the reviewer for their work.  And it would help the
        committer work.

Subject: Thoughts on committers pushing simple changes from other committers?
1: <https://yhetil.org/guix/86k00avhpv.fsf@gmail.com>

Cheers,
simon

[bug#61894] [PATCH RFC] Team approval for patches

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

> Hello!
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
> [...]
>
>>> “Pacify” in the sense that, by being explicit, we avoid
>>> misunderstandings that could turn into unpleasant experiences.
>>>
>>> Like you I’m glad collaboration is nice and friendly; yet, over the past
>>> few months I’ve experienced misunderstandings that seemingly broke the
>>> consensus-based process that has always prevailed.
>>
>> I'm sorry that you feel that way.  I don't think consensus was willfully
>> broken,
>
> That’s my point: by being explicit about approval, we would avoid such
> misunderstandings.
>
>> and perhaps by studying some actual examples of these occurrences we
>> can better understand what went wrong and how the new suggested policy
>> would have helped or could be modified to help avoid such problems in
>> the future.
>
> I don’t want to rehash past occurrences of this problem.  It boils down
> to: changes where pushed despite consensus evidently not being met, at
> least not in the mind of every involved party.
>
> To some extent, that’s bound to happen due to an increase of the number
> of contributors, scope of the project, and diversity of backgrounds.  By
> making it clear that lack of “LGTM” from another team member equates
> with lack of consensus, we would avoid those misunderstandings.

I agree that firm conventions here would make things smoother.  It's
common that someone offers an incomplete review or just forget the LGTM,
and the author is left to ask if it's OK to push or not or assume it is.

> A good reference on consensus-based decision making is
> <https://www.seedsforchange.org.uk/consensus>.
>
>> It's also worth noting that this consensus-based process has always
>> been implicit; for example, it is not defined/mentioned anywhere in
>> our documentation.  Perhaps it should?
>
> Those who’ve followed the project long enough, such as part of the
> current maintainer collective, are certainly aware of that; it’s also
> spelled out in
> <https://guix.gnu.org/blog/2019/gnu-guix-maintainer-collective-expands/>.
>
> That said, again in the spirit of improving legibility, writing it down
> would be much welcome.

Yes, I've heard 'consensus' for years, and I think I have a good enough
understanding of it, but I think there are subtleties many (including
myself) fail to appreciate that the above link explains well, so I think
there's value in mentioning it somewhere.  I'll send a patch for
everyone to review.

>>> In a way, that’s probably bound to happen as the group grows, and I
>>> think that’s why we must be explicit about what the process is and about
>>> whether one is expressing consent or dissent.
>>>
>>> With so many things happening in Guix (yay!), it’s also easy to overlook
>>> a change and realize when it’s too late.  By having a rule that at least
>>> one other person on the team must approve (consent to) a change, we
>>> reduce that risk.
>>>
>>> Being on a team, then, is a way to express interest on a topic and to be
>>> “in the loop”.
>>
>> That's already what teams can do!
>
> Yes and no.  With the amount of activity going on, it’s easy to overlook
> something.  The explicit synchronization point could mitigate that.
>
>> I'd argue giving them the extra powers that would be conferred to
>> teams in this is not needed/desirable.  Some committer not a regular
>> member of X team may still be confident enough to push a patch sitting
>> on the tracker, and I think they should be able to.
>
> Self-assessment becomes tricky that this scale; I might be confident and
> yet someone will point out a problem (that literally happened to me two
> days ago in <https://issues.guix.gnu.org/62062>).  That’s when review
> really helps.
>
> For “core” work, I insist that explicit approval (and thus peer review)
> is necessary.  I doubt anyone would seriously challenge that.
>
> Now, I agree, as I wrote before, that this may be overkill for “random
> packages”.
>
> Thus we need to find the right balance.
>
> What about team/scope-specific rules?  As in: “Changes covered by teams
> X, Y, and Z need to be explicitly approved by at least one other member
> of the team.”

To me, it seems challenging to partition the code correctly and avoid
overloading the core team with spurious changes, which would slow things
down more (as the core team would be a fraction of the committers
currently enabled to push such changes), but I agree in principle that
it makes sense.

>>> It is not about asserting power or building a hierarchy;
>>> it’s about formalizing existing relations and processes.
>>
>> OK; I think in practice it would amount to that though (building a
>> hierarchy which has some form power).
>
> I disagree: just because power relations are not spelled out doesn’t
> mean they don’t exist.  I don’t know where you’re talking from; one
> thing that to me shed light on these matters is “The Tyranny of
> Structurelessness” (I’m sure I mentioned it before, I certainly did
> during Q&A on this very topic at the Ten Years event; apologies if I
> sound like a broken record!).

As with anything there's probably a middle ground to reach, where
there's some structure, but not too much, that maximizes our collective
happiness.

-- 
Thanks,
Maxim