Thoughts on committers pushing simple changes from other committers?

Thoughts on committers pushing simple changes from other committers?

[Christopher Baines]

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

Hey!

So probably in part due to the recent changes to the commit policy [1] I
think there are more "simple" changes being sent to guix-patches by
committers, which I think is good, but that's got me thinking about the
process for these changes.

1: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=9aa2b7419854306b7ae78d4c4f7684316b834b1d

Generally, I don't push changes for other committers, but I wonder if
that would be helpful now.

Take #61416 [2] as an example, I replied saying the change looked good
to me, but maybe it would be easier if I'd just pushed it along with the
changes I was pushing for non-committers?

2: https://issues.guix.gnu.org/61416

Obviously I think this wouldn't apply to all changes, just the simpler
ones (for some vauge definition of simpler).

Thoughts?

Thanks,

Chris

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

Re: Thoughts on committers pushing simple changes from other committers?

[Andreas Enge]

Am Tue, Feb 21, 2023 at 12:54:32PM +0000 schrieb Christopher Baines:
> Generally, I don't push changes for other committers, but I wonder if
> that would be helpful now.

Personally I prefer to push my own changes. And let other people push
theirs if they can. Actually I think we are responsible of the patches
we push, and less so of the patches we author :)  This is consistent with
the signatures as well.

Concerning other people's patches, what makes this policy a bit complicated
is that I think we do not have a list of committers on the web. So I might
accidentally push another committer's patch, or conversely not push
someone's patch who I erroneously assume to be a committer.

Andreas

Re: Thoughts on committers pushing simple changes from other committers?

[Leo Famulari]

On Tue, Feb 21, 2023 at 03:57:10PM +0100, Andreas Enge wrote:
> Concerning other people's patches, what makes this policy a bit complicated
> is that I think we do not have a list of committers on the web.

Commit access is controlled via membership of the Savannah project:

https://savannah.gnu.org/project/memberlist.php?group=guix

Thus, that is the list.

Re: Thoughts on committers pushing simple changes from other committers?

[Andreas Enge]

Am Tue, Feb 21, 2023 at 01:00:07PM -0500 schrieb Leo Famulari:
> Commit access is controlled via membership of the Savannah project:
> https://savannah.gnu.org/project/memberlist.php?group=guix
> Thus, that is the list.

Thanks, very helpful!

Andreas

Re: Thoughts on committers pushing simple changes from other committers?

[Leo Famulari]

On Tue, Feb 21, 2023 at 12:54:32PM +0000, Christopher Baines wrote:
> Thoughts?

I think it's fine to push other people's patches. Already, we push the
patches of non-committers. It's no big deal to push committers' patches.

Re: Thoughts on committers pushing simple changes from other committers?

[Vagrant Cascadian]

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

On 2023-02-21, Christopher Baines wrote:
> So probably in part due to the recent changes to the commit policy [1] I
> think there are more "simple" changes being sent to guix-patches by
> committers, which I think is good, but that's got me thinking about the
> process for these changes.
>
> 1: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=9aa2b7419854306b7ae78d4c4f7684316b834b1d
>
> Generally, I don't push changes for other committers, but I wonder if
> that would be helpful now.

As someone who submitted a patch a few weeks ago:

  https://issues.guix.gnu.org/60940

...which was very recently ACKed, I would definitely welcome someone
pushing it!

There is also no sign like "this has been reviewed by N people" better
than one person authoring it and another person pushing and signing it,
with some Reviewed-by or other relevent tags in the commit.

For some patches (60940 took a few months of on-again, off-again work,
testing, and poking at it), by the time it lands as a submitted patch, I
might be a little tired of staring at it. :)

Even with a quick (e.g. a couple days) response to a submitted patch, I
likely have moved onto other things, it may not be fresh in my mind, the
right things may not be in my /gnu/store anymore to quickly re-test it,
it may take a bit to apply the patch on the right branch...

In this particular case, not only has my attention shifted to other
things for a while (e.g. Debian freeze cycle and things entirely outside
of computer realms) ... to top it off the machine I tested the changes
on ceased to exist!

A lot can happen in a few days, or weeks, etc.

Obviously, for someone else to push a "my" patch, they may have to go
through a lot of the same sorts of issues... but if they have freshly
reviewed it, maybe they are more in a state of "working on guix" and it
might be less of a context switch? Or maybe not...

I guess one way might be to be more explicit about intentions and access
when submitting or reviewing patches:

"Reviews definitely appreciated, but please let me push it myself when
it is ready."

"On reviewing this patch, it looks good to me, but I cannot push it
myself right now. Push when you can or maybe another committer can do
it."

"Thanks for the patch, I will push in X days unless you beat me to it!"

"Reviewers, if you like this patch, go ahead and push it, as I do not
have commit access."

"I am so *done* with this patch, any reviewers please consider pushing
directly if you think it is ready!"

Just a few ideas, could maybe formalize and document it a bit with some
specific recommended tags or keywords or whatever...


Maybe QA could even look up the committers by email and compare against
the submitter to determine if they have commit access or not, and
display that information on the patch review pages? No need to be
perfect, just more right than not. :)


live well,
  vagrant

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

Re: Thoughts on committers pushing simple changes from other committers?

[Simon Tournier]

Hi,

On Tue, 21 Feb 2023 at 10:52, Vagrant Cascadian <vagrant@debian.org> wrote:

> There is also no sign like "this has been reviewed by N people" better
> than one person authoring it and another person pushing and signing it,
> with some Reviewed-by or other relevent tags in the commit.

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.

Cheers,
simon

Re: Thoughts on committers pushing simple changes from other committers?

[Ludovic Courtès]

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

> On Tue, 21 Feb 2023 at 10:52, Vagrant Cascadian <vagrant@debian.org> wrote:
>
>> There is also no sign like "this has been reviewed by N people" better
>> than one person authoring it and another person pushing and signing it,
>> with some Reviewed-by or other relevent tags in the commit.
>
> 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.

I like this idea!

Ludo’.