[PATCH] doc: contributing: Tweak the Commit Policy.

Done

Details

6 participants

Liliana Marie Prikler
Ludovic Courtès
Christopher Baines
Maxim Cournoyer
Vagrant Cascadian
zimoun

Owner: unassigned

Submitted by: Christopher Baines

Severity: normal

Christopher Baines wrote on 23 Nov 2022 11:49

Recipients:(address . guix-patches@gnu.org)

Message-ID:20221123104946.29480-1-mail@cbaines.net

Add more examples of when it can be appropriate to push changes without

review, as I think this can be appropriate in the case of trivial changes (as

mentioned before), but also non-trivial fixes.

No longer suggest pushing simple new packages or package upgrades (that don't

cause lots of rebuilds) without sending to guix-patches. Now there's some

automation for testing changes sent to guix-patches, sending changes there

before pushing can mean that more rigerious testing takes place and help speed

up substitutes becoming available. This is true, even if no human review takes

place.

Only suggest waiting one week for review for simpler changes, wait two weeks

for more significant changes.

Also, reorder some of the information in this section so it's grouped together

better.

* doc/contributing.texi (Commit Policy): Tweak.

---

doc/contributing.texi | 38 ++++++++++++++++----------------------

1 file changed, 16 insertions(+), 22 deletions(-)

Toggle diff (61 lines)diff --git a/doc/contributing.texi b/doc/contributing.texi
index 40ae33ecac..6f772961ea 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -1819,23 +1819,25 @@ It additionally calls @code{make check-channel-news} to be sure
 
 @subsection Commit Policy
 
-If you get commit access, please make sure to follow
-the policy below (discussions of the policy can take place on
+If you get commit access, please make sure to follow the policy below
+(discussions of the policy can take place on
 @email{guix-devel@@gnu.org}).
 
-Non-trivial patches should always be posted to
-@email{guix-patches@@gnu.org} (trivial patches include fixing typos,
-etc.).  This mailing list fills the patch-tracking database
-(@pxref{Tracking Bugs and Patches}).
+For a minority of changes, it can be appropriate to push them directly
+without sending them for review.  This includes both trivial changes
+(e.g. fixing typos) but also reverting problomatic changes and
+addressing regressions.
 
-For patches that just add a new package, and a simple one, it's OK to
-commit, if you're confident (which means you successfully built it in a
-chroot setup, and have done a reasonable copyright and license
-auditing).  Likewise for package upgrades, except upgrades that trigger
-a lot of rebuilds (for example, upgrading GnuTLS or GLib).  We have a
-mailing list for commit notifications (@email{guix-commits@@gnu.org}),
-so people can notice.  Before pushing your changes, make sure to run
-@code{git pull --rebase}.
+In general though, all changes should be posted to
+@email{guix-patches@@gnu.org}.  This mailing list fills the
+patch-tracking database (@pxref{Tracking Bugs and Patches}).  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.
+
+That last part is subject to being adjusted, allowing individuals to
+commit directly on non-controversial changes on parts they’re familiar
+with.
 
 When pushing a commit on behalf of somebody else, please add a
 @code{Signed-off-by} line at the end of the commit log message---e.g.,
@@ -1850,14 +1852,6 @@ right before pushing:
 make check-channel-news
 @end example
 
-For anything else, please post to @email{guix-patches@@gnu.org} and
-leave time for a review, without committing anything (@pxref{Submitting
-Patches}).  If you didn’t receive any reply after two weeks, and if
-you're confident, it's OK to commit.
-
-That last part is subject to being adjusted, allowing individuals to commit
-directly on non-controversial changes on parts they’re familiar with.
-
 @subsection Addressing Issues
 
 Peer review (@pxref{Submitting Patches}) and tools such as
-- 
2.38.1

zimoun wrote on 23 Nov 2022 21:27

Recipients:

Message-ID:86tu2pfmbv.fsf@gmail.com

Hi Chris,

On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote:

Toggle quote (3 lines)

> +For a minority of changes, it can be appropriate to push them directly

> +without sending them for review. This includes both trivial changes

> +(e.g. fixing typos) but also reverting problomatic changes and

Toggle quote (18 lines)

> +addressing regressions.

> -For patches that just add a new package, and a simple one, it's OK to

> -commit, if you're confident (which means you successfully built it in a

> -chroot setup, and have done a reasonable copyright and license

> -auditing). Likewise for package upgrades, except upgrades that trigger

> -a lot of rebuilds (for example, upgrading GnuTLS or GLib). We have a

> -mailing list for commit notifications (@email{guix-commits@@gnu.org}),

> -so people can notice. Before pushing your changes, make sure to run

> -@code{git pull --rebase}.

> +In general though, all changes should be posted to

> +@email{guix-patches@@gnu.org}. This mailing list fills the

> +patch-tracking database (@pxref{Tracking Bugs and Patches}). 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.

I would write:

… changes), and if you're confident (which means you

successfully built it in a chroot setup, and have done a

reasonable copyright and license auditing), it’s OK to commit.

and I would keep the «two weeks» instead of the «one week except».

I think it is also useful to provide the information about commit

notifications (guix-commits mailing list).

For what it is worth, I find clearer the structure,

For patches that …

For anything else, …

For a minority of changes, …

For anything else, …

than «In general though, all changes …».

Cheers,

simon

Christopher Baines wrote on 24 Nov 2022 09:40

Recipients:(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 59513@debbugs.gnu.org)

Message-ID:871qps20fa.fsf@cbaines.net

zimoun <zimon.toutoune@gmail.com> writes:

Toggle quote (2 lines)

> Hi Chris,

Hey! Thanks for taking a look.

Toggle quote (30 lines)> On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote:
>
>> +For a minority of changes, it can be appropriate to push them directly
>> +without sending them for review.  This includes both trivial changes
>> +(e.g. fixing typos) but also reverting problomatic changes and
>                                               -^
>> +addressing regressions.
>
>
>> -For patches that just add a new package, and a simple one, it's OK to
>> -commit, if you're confident (which means you successfully built it in a
>> -chroot setup, and have done a reasonable copyright and license
>> -auditing).  Likewise for package upgrades, except upgrades that trigger
>> -a lot of rebuilds (for example, upgrading GnuTLS or GLib).  We have a
>> -mailing list for commit notifications (@email{guix-commits@@gnu.org}),
>> -so people can notice.  Before pushing your changes, make sure to run
>> -@code{git pull --rebase}.
>> +In general though, all changes should be posted to
>> +@email{guix-patches@@gnu.org}.  This mailing list fills the
>> +patch-tracking database (@pxref{Tracking Bugs and Patches}).  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.
>
> I would write:
>
>         … changes), and if you're confident (which means you
>         successfully built it in a chroot setup, and have done a
>         reasonable copyright and license auditing), it’s OK to commit.

chroot setup doesn't really make sense to me, I'm not sure why that

needs specifying (like do we not want things for the Hurd pushing, since

the guix-daemon doesn't support build isolation there yet)?

Also, this guidance is very general, and I think it should be applicable

to all changes. We already trust people with commit access to know what

needs doing, I see this documentation as more about how, so I'd prefer

not to try and put a list here.

Toggle quote (2 lines)

> and I would keep the «two weeks» instead of the «one week except».

My reason for changing this is that I think waiting two weeks after

sending a simple patch is unreasonable. The value from the automated

testing will come after one to two days, I just put a week to avoid

changing it too much, but maybe the lower bound should be two days.

Toggle quote (3 lines)

> I think it is also useful to provide the information about commit

> notifications (guix-commits mailing list).

Why though? What do we expect people with commit access to do when they

read about that here?

Toggle quote (12 lines)> For what it is worth, I find clearer the structure,
>
>     For patches that …
>     For anything else, …
>
> or
>
>     For a minority of changes, …
>     For anything else, …
>
> than «In general though, all changes …».

That seems fine to me, I think "everything" maybe carries more weight

than "anything" though.

Thanks,

Chris

Christopher Baines wrote on 24 Nov 2022 10:01

tag 59513 moreinfo

Recipients:(address . control@debbugs.gnu.org)

Message-ID:877czkg20j.fsf@cbaines.net

tags 59513 + moreinfo

quit

zimoun wrote on 24 Nov 2022 12:59

Re: [bug#59513] [PATCH] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 59513@debbugs.gnu.org)

Message-ID:86wn7kd0m9.fsf@gmail.com

Hi,

On Thu, 24 Nov 2022 at 08:40, Christopher Baines <mail@cbaines.net> wrote:

Toggle quote (8 lines)

>> On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote:

>>> +For a minority of changes, it can be appropriate to push them directly

>>> +without sending them for review. This includes both trivial changes

>>> +(e.g. fixing typos) but also reverting problomatic changes and

>> -^

>>> +addressing regressions.

To be sure you have not missed the typo here. :-)

s/problomatic/problematic

Toggle quote (8 lines)

>> … changes), and if you're confident (which means you

>> successfully built it in a chroot setup, and have done a

>> reasonable copyright and license auditing), it’s OK to commit.

> chroot setup doesn't really make sense to me, I'm not sure why that

> needs specifying (like do we not want things for the Hurd pushing, since

> the guix-daemon doesn't support build isolation there yet)?

Good point about chroot. :-)

Toggle quote (5 lines)

> Also, this guidance is very general, and I think it should be applicable

> to all changes. We already trust people with commit access to know what

> needs doing, I see this documentation as more about how, so I'd prefer

> not to try and put a list here.

Yes, we trust people. But a public and explicit policy reinforces the

trust, IMHO. It also documents what commit access means. It is not

because people with commit access already know what they need doing that

all people know, I guess.

Toggle quote (7 lines)

>> and I would keep the «two weeks» instead of the «one week except».

> My reason for changing this is that I think waiting two weeks after

> sending a simple patch is unreasonable. The value from the automated

> testing will come after one to two days, I just put a week to avoid

> changing it too much, but maybe the lower bound should be two days.

Who is verifying the impact of a change? :-) Just a recent example to

fix the ideas. The same situation is happening more than often but not

that often neither. :-)

(It is an example and no blame here. Or blame on me only, for not

taking enough care of Julia packages.)

Patch#58644 [1] submitted on 19 Oct and pushed on 8 Nov; which is 22

days. Unfortunately, this patch breaks julia-documenter [2], so it

means many Julia packages are currently broken; since 17 days.

Commit 83ede5a02e1fc531d912eb92eb0a22a4b897997c,

gnu: git: Update to 2.38.1.

Fixes CVE-2022-39253 and CVE-2022-39260.

* gnu/packages/version-control.scm (git): Update to 2.38.1.

Co-authored-by: Ludovic Courtès <ludo@gnu.org>

1 file changed, 3 insertions(+), 3 deletions(-)

gnu/packages/version-control.scm | 6 +++---

from v2.38.0 to v2.38.1 seems inoffensive. :-) But,

Toggle snippet (9 lines)

$ ag 'inherit git'

gnu/packages/version-control.scm

613: (inherit git)

676: (package/inherit git-minimal

$ guix refresh -l git git-minimal | cut -f1 -d':'

Building the following 292 packages would ensure 658 dependent packages are rebuilt

(The one at line 676 is not impacted by the change, IIRC.)

Who does check these 292 packages?

For instance, this patch has an impact on Julia packages,

Toggle snippet (14 lines)$ guix refresh -l git git-minimal  | cut -f2 -d':'  | sed 's/ /\n/g' | grep julia
julia-geometrybasics@0.4.1
julia-configurations@0.16.4
julia-pyplot@2.10.0
julia-recipespipeline@0.3.4
julia-quadmath@0.5.5
julia-plotthemes@2.0.1
julia-infinity@0.2.4
julia-testimages@1.5.0
julia-optim@1.6.0
julia-referencetests@0.9.7
julia-imagemagick@1.2.1

As part of the Julia team, maybe I could have a look. Well, I had not

had the time in these 2 weeks to fix it yet. For two reasons:

1. Because I noticed the failure just a couple of days ago.

2. Because I was busy elsewhere.

About #1, I can follow a RSS feed by Cuirass. But somehow, it is too

late then either I am working in a rush to minimize the breakage or

either the package is broken… as today.

I have not yet carefully looked at the new QA (neat!). Is it possible

to follow some notifications?

About #2, two weeks let the time to check the impact of a change. And

even, depending on my schedule, sometime it is short. :-) But hey, I

agree that the things need to move forward. :-)

My point is: Considering leaf packages, yeah once submitted, the review

can be fast (couple of days) especially with the new QA. Considering

all the other packages, who is checking the impact of a change?

Otherwise, we have again and again some broken packages. For sure, the

QA is helping *a lot* for improving! Well, on one hand, I understand

the willing to merge faster and, even I am not convinced that from two

weeks to one week would be detrimental. On the other hand, using Guix,

I replaced the pressure when running “apt-get upgrade” by an eternal

annoyance of broken packages popping here or there.

Somehow, https://ci.guix.gnu.org/eval/4095/dashboard should be always

all green and faster the Git tree moves, harder it is to achieve, IMHO.

1: https://issues.guix.gnu.org/58644

2: https://data.guix.gnu.org/repository/1/branch/master/package/julia-documenter/output-history

Toggle quote (6 lines)

>> I think it is also useful to provide the information about commit

>> notifications (guix-commits mailing list).

> Why though? What do we expect people with commit access to do when they

> read about that here?

Maybe it is a niche, I used it a couple of time. For instance, to

comment a change already merged, see [3] I guess.

3: https://lists.gnu.org/archive/html/guix-devel/2022-10/msg00099.html

Cheers,

simon

Christopher Baines wrote on 28 Nov 2022 12:46

Recipients:(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 59513@debbugs.gnu.org)

Message-ID:87tu2jxp41.fsf@cbaines.net

zimoun <zimon.toutoune@gmail.com> writes:

Toggle quote (14 lines)> On Thu, 24 Nov 2022 at 08:40, Christopher Baines <mail@cbaines.net> wrote:
>
>>> On Wed, 23 Nov 2022 at 10:49, Christopher Baines <mail@cbaines.net> wrote:
>>>
>>>> +For a minority of changes, it can be appropriate to push them directly
>>>> +without sending them for review.  This includes both trivial changes
>>>> +(e.g. fixing typos) but also reverting problomatic changes and
>>>                                               -^
>>>> +addressing regressions.
>
> To be sure you have not missed the typo here. :-)
>
> s/problomatic/problematic

Thanks, I've fixed that locally now.

Toggle quote (10 lines)>> Also, this guidance is very general, and I think it should be applicable
>> to all changes. We already trust people with commit access to know what
>> needs doing, I see this documentation as more about how, so I'd prefer
>> not to try and put a list here.
>
> Yes, we trust people.  But a public and explicit policy reinforces the
> trust, IMHO.  It also documents what commit access means.  It is not
> because people with commit access already know what they need doing that
> all people know, I guess.

I don't disagree that we should make the expectations about

functionality and testing explicit, but I want to see that separate from

the commit policy.

Toggle quote (11 lines)>>> and I would keep the «two weeks» instead of the «one week except».
>>
>> My reason for changing this is that I think waiting two weeks after
>> sending a simple patch is unreasonable. The value from the automated
>> testing will come after one to two days, I just put a week to avoid
>> changing it too much, but maybe the lower bound should be two days.
>
> Who is verifying the impact of a change? :-) Just a recent example to
> fix the ideas.  The same situation is happening more than often but not
> that often neither. :-)

...

Toggle quote (11 lines)> My point is: Considering leaf packages, yeah once submitted, the review
> can be fast (couple of days) especially with the new QA.  Considering
> all the other packages, who is checking the impact of a change?
>
> Otherwise, we have again and again some broken packages.  For sure, the
> QA is helping *a lot* for improving!  Well, on one hand, I understand
> the willing to merge faster and, even I am not convinced that from two
> weeks to one week would be detrimental.  On the other hand, using Guix,
> I replaced the pressure when running “apt-get upgrade” by an eternal
> annoyance of broken packages popping here or there.

This is going a bit off topic I think.

In general, the direction I'm trying to move the policy in here is one

where more changes get sent to guix-patches rather than getting pushed

straight to the repository.

Checking the impact of changes is important, but you can't do that with

a policy on committing. If however people send changes to guix-patches

prior to pushing, then there's at least a chance that some automatic

"verifying/checking" can take place.

Ludovic Courtès wrote on 1 Dec 2022 22:44

Re: bug#59513: [PATCH] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 59513@debbugs.gnu.org)

Message-ID:875yeuyf1z.fsf@gnu.org

Hi!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (19 lines)> Add more examples of when it can be appropriate to push changes without
> review, as I think this can be appropriate in the case of trivial changes (as
> mentioned before), but also non-trivial fixes.
>
> No longer suggest pushing simple new packages or package upgrades (that don't
> cause lots of rebuilds) without sending to guix-patches. Now there's some
> automation for testing changes sent to guix-patches, sending changes there
> before pushing can mean that more rigerious testing takes place and help speed
> up substitutes becoming available. This is true, even if no human review takes
> place.
>
> Only suggest waiting one week for review for simpler changes, wait two weeks
> for more significant changes.
>
> Also, reorder some of the information in this section so it's grouped together
> better.
>
> * doc/contributing.texi (Commit Policy): Tweak.

FWIW I like the spirit of these changes.

Now to the letter… :-)

Toggle quote (19 lines)>  @subsection Commit Policy
>  
> -If you get commit access, please make sure to follow
> -the policy below (discussions of the policy can take place on
> +If you get commit access, please make sure to follow the policy below
> +(discussions of the policy can take place on
>  @email{guix-devel@@gnu.org}).
>  
> -Non-trivial patches should always be posted to
> -@email{guix-patches@@gnu.org} (trivial patches include fixing typos,
> -etc.).  This mailing list fills the patch-tracking database
> -(@pxref{Tracking Bugs and Patches}).
> +For a minority of changes, it can be appropriate to push them directly
> +without sending them for review.  This includes both trivial changes
> +(e.g. fixing typos) but also reverting problomatic changes and
> +addressing regressions.
>  
> -For patches that just add a new package, and a simple one, it's OK to

Similar to zimoun’s first comment I think, I would like the beginning of

the sentence to clearly tell you whether it’s the situation you’re

interested in. “For a minority of changes” doesn’t fit the bill in my

view.

So I would suggest something along the lines of:

Changes should be posted to @email{guix-patches@@gnu.org}. This

mailing list […]. It also allows patches to be picked up and tested

by the quality assurance robot; the result of that testing eventually

shows up on the dashboard at

@indicateurl{https://qa.guix.gnu.org/issue/@var{number}},where

@var{number} is the number assigned by the issue tracker. Leave time

[…] it’s OK to commit.

As an exception, some changes considered consensual and ``trivial'' or

``obvious'' may instead be pushed directly. These include: fixing

typos, and reverting commits that caused immediate problems.

That way we state the general rule first, and the exception next. That

also explicitly mentions how that relates to qa.guix.

Regarding the list of exceptions, I feel that these two exceptions

listed here may be less than what we may except on a day-to-day basis;

perhaps there are other things to add there, but I’m not sure what.

Would be nice to get feedback from a maintainer too.

Thanks for working on this!

Ludo’.

Ludovic Courtès wrote on 2 Dec 2022 10:45

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)

Message-ID:877czauokf.fsf_-_@gnu.org

Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (9 lines)

>> I would write:

>> … changes), and if you're confident (which means you

>> successfully built it in a chroot setup, and have done a

>> reasonable copyright and license auditing), it’s OK to commit.

> chroot setup doesn't really make sense to me, I'm not sure why that

> needs specifying

The chroot requirement is specified because a non-isolated build is

worth nothing: it might work on some machine and fail on another one for

hard-to-find reasons.

We could stop mentioning it if we think everyone keeps chroot enabled

(that’s probably the case), but it doesn’t hurt to keep it.

Toggle quote (3 lines)

> (like do we not want things for the Hurd pushing, since the

> guix-daemon doesn't support build isolation there yet)?

Well yes, that’s the thing. For i586-gnu we’re currently stuck because

we haven’t yet defined how to normalize the build environment:

https://guix.gnu.org/en/blog/2020/childhurds-and-substitutes/

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

And it’s not theoretical: some util-linux tests may or may not work

depending on whether the Hurd box has /proc set up, the Texinfo test

suite would pass if there happens to be a usable ‘pt_chown’ program

around (see https://issues.guix.gnu.org/59616), etc.

So I think the current situation is that we can build base packages for

i586-gnu, that’s fine, but don’t have a solid foundation like we do on

GNU/Linux, so there’s no point in going too far.

Now, I don’t think the i586-gnu situation is an important consideration

for the commit policy.

Ludo’.

Christopher Baines wrote on 8 Dec 2022 12:20

[PATCH v2] doc: contributing: Tweak the Commit Policy.

Recipients:(address . 59513@debbugs.gnu.org)

Message-ID:20221208112051.5019-1-mail@cbaines.net

Add more examples of when it can be appropriate to push changes without

review, as I think this can be appropriate in the case of trivial changes (as

mentioned before), but also non-trivial fixes.

No longer suggest pushing simple new packages or package upgrades (that don't

cause lots of rebuilds) without sending to guix-patches. Now there's some

automation for testing changes sent to guix-patches, sending changes there

before pushing can mean that more rigerious testing takes place and help speed

up substitutes becoming available. This is true, even if no human review takes

place.

Only suggest waiting one week for review for simpler changes, wait two weeks

for more significant changes.

Also, reorder some of the information in this section so it's grouped together

better.

* doc/contributing.texi (Commit Policy): Tweak.

---

doc/contributing.texi | 41 ++++++++++++++++++-----------------------

1 file changed, 18 insertions(+), 23 deletions(-)

Toggle diff (63 lines)diff --git a/doc/contributing.texi b/doc/contributing.texi
index 6a8ffd6524..d2e7abba98 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -1824,23 +1824,26 @@ It additionally calls @code{make check-channel-news} to be sure
 
 @subsection Commit Policy
 
-If you get commit access, please make sure to follow
-the policy below (discussions of the policy can take place on
+If you get commit access, please make sure to follow the policy below
+(discussions of the policy can take place on
 @email{guix-devel@@gnu.org}).
 
-Non-trivial patches should always be posted to
-@email{guix-patches@@gnu.org} (trivial patches include fixing typos,
-etc.).  This mailing list fills the patch-tracking database
-(@pxref{Tracking Bugs and Patches}).
-
-For patches that just add a new package, and a simple one, it's OK to
-commit, if you're confident (which means you successfully built it in a
-chroot setup, and have done a reasonable copyright and license
-auditing).  Likewise for package upgrades, except upgrades that trigger
-a lot of rebuilds (for example, upgrading GnuTLS or GLib).  We have a
-mailing list for commit notifications (@email{guix-commits@@gnu.org}),
-so people can notice.  Before pushing your changes, make sure to run
-@code{git pull --rebase}.
+Changes should be posted to @email{guix-patches@@gnu.org}.  This mailing
+list fills the patch-tracking database (@pxref{Tracking Bugs and
+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{number}}, where
+@var{number} is the number assigned by the issue tracker.  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.
+
+As an exception, some changes considered ``trivial'' or ``obvious'' may
+be pushed directly.  This includes changes to fix typos and reverting
+commits that caused immediate problems.  This is subject to being
+adjusted, allowing individuals to commit directly on non-controversial
+changes on parts they’re familiar with.
 
 When pushing a commit on behalf of somebody else, please add a
 @code{Signed-off-by} line at the end of the commit log message---e.g.,
@@ -1855,14 +1858,6 @@ right before pushing:
 make check-channel-news
 @end example
 
-For anything else, please post to @email{guix-patches@@gnu.org} and
-leave time for a review, without committing anything (@pxref{Submitting
-Patches}).  If you didn’t receive any reply after two weeks, and if
-you're confident, it's OK to commit.
-
-That last part is subject to being adjusted, allowing individuals to commit
-directly on non-controversial changes on parts they’re familiar with.
-
 @subsection Addressing Issues
 
 Peer review (@pxref{Submitting Patches}) and tools such as
-- 
2.38.1

Liliana Marie Prikler wrote on 8 Dec 2022 14:53

Recipients:

Message-ID:f4f6e9cc1aaa6d56da86a40d20136af406f7384a.camel@gmail.com

Am Donnerstag, dem 08.12.2022 um 11:20 +0000 schrieb Christopher

Baines:

Toggle quote (10 lines)> Add more examples of when it can be appropriate to push changes
> without
> review, as I think this can be appropriate in the case of trivial
> changes (as
> mentioned before), but also non-trivial fixes.
> 
> No longer suggest pushing simple new packages or package upgrades
> (that don't cause lots of rebuilds) without sending to guix-patches.
> Now there's some automation for testing changes sent to guix-patches,
> sending changes there before pushing can mean that more rigerious

rigorous

Toggle quote (64 lines)> testing takes place and help speed up substitutes becoming available.
> This is true, even if no human review takes place.
> 
> Only suggest waiting one week for review for simpler changes, wait
> two weeks
> for more significant changes.
> 
> Also, reorder some of the information in this section so it's grouped
> together
> better.
> 
> * doc/contributing.texi (Commit Policy): Tweak.
> ---
> �doc/contributing.texi | 41 ++++++++++++++++++-----------------------
> �1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/doc/contributing.texi b/doc/contributing.texi
> index 6a8ffd6524..d2e7abba98 100644
> --- a/doc/contributing.texi
> +++ b/doc/contributing.texi
> @@ -1824,23 +1824,26 @@ It additionally calls @code{make check-
> channel-news} to be sure
> �
> �@subsection Commit Policy
> �
> -If you get commit access, please make sure to follow
> -the policy below (discussions of the policy can take place on
> +If you get commit access, please make sure to follow the policy
> below
> +(discussions of the policy can take place on
> �@email{guix-devel@@gnu.org}).
> �
> -Non-trivial patches should always be posted to
> -@email{guix-patches@@gnu.org} (trivial patches include fixing typos,
> -etc.).� This mailing list fills the patch-tracking database
> -(@pxref{Tracking Bugs and Patches}).
> -
> -For patches that just add a new package, and a simple one, it's OK
> to
> -commit, if you're confident (which means you successfully built it
> in a
> -chroot setup, and have done a reasonable copyright and license
> -auditing).� Likewise for package upgrades, except upgrades that
> trigger
> -a lot of rebuilds (for example, upgrading GnuTLS or GLib).� We have
> a
> -mailing list for commit notifications (@email{guix-
> commits@@gnu.org}),
> -so people can notice.� Before pushing your changes, make sure to run
> -@code{git pull --rebase}.
> +Changes should be posted to @email{guix-patches@@gnu.org}.� This
> mailing
> +list fills the patch-tracking database (@pxref{Tracking Bugs and
> +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{number}}, where
> +@var{number} is the number assigned by the issue tracker.� 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.

I would reword that so�

(not significant ∧ confident ∧ qa_green) → good after 1 week

whereas

(not significant ∧ confident ∧ qa_unknown) → good after 2 weeks

and significant changes should anyway take 2 weeks.

Cheers

Christopher Baines wrote on 12 Dec 2022 11:33

Re: bug#59513: [PATCH] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 59513@debbugs.gnu.org)

Message-ID:871qp4rj6y.fsf@cbaines.net

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

Toggle quote (30 lines)>>  @subsection Commit Policy
>>
>> -If you get commit access, please make sure to follow
>> -the policy below (discussions of the policy can take place on
>> +If you get commit access, please make sure to follow the policy below
>> +(discussions of the policy can take place on
>>  @email{guix-devel@@gnu.org}).
>>
>> -Non-trivial patches should always be posted to
>> -@email{guix-patches@@gnu.org} (trivial patches include fixing typos,
>> -etc.).  This mailing list fills the patch-tracking database
>> -(@pxref{Tracking Bugs and Patches}).
>> +For a minority of changes, it can be appropriate to push them directly
>> +without sending them for review.  This includes both trivial changes
>> +(e.g. fixing typos) but also reverting problomatic changes and
>> +addressing regressions.
>>
>> -For patches that just add a new package, and a simple one, it's OK to
>
> Similar to zimoun’s first comment I think, I would like the beginning of
> the sentence to clearly tell you whether it’s the situation you’re
> interested in.  “For a minority of changes” doesn’t fit the bill in my
> view.
>
> So I would suggest something along the lines of:
>
>   Changes should be posted to @email{guix-patches@@gnu.org}.  This
>   mailing list […].  It also allows patches to be picked up and tested
>   by the quality assurance robot; the result of that testing eventually

I've gone for "tooling" rather than "robot" as I'm not sure we want to

go the way of personifying it. I'm not against that, but the place to

start is probably not here.

Toggle quote (7 lines)

> shows up on the dashboard at

> @indicateurl{https://qa.guix.gnu.org/issue/@var{number}}, where

> @var{number} is the number assigned by the issue tracker. Leave time

> […] it’s OK to commit.

> As an exception, some changes considered consensual and ``trivial'' or

I removed "consensual" here as I wasn't sure what was meant by that, or

at least I'm not sure the phrasing fits the context here.

Are you trying to say something about a belief that no one will object

to the change being made?

Toggle quote (6 lines)

> ``obvious'' may instead be pushed directly. These include: fixing

> typos, and reverting commits that caused immediate problems.

> That way we state the general rule first, and the exception next. That

> also explicitly mentions how that relates to qa.guix.

Yeah, I think that's better. I've sent a v2 patch now (for some reason I

forgot to send this email until now).

Thanks,

Chris

Christopher Baines wrote on 12 Dec 2022 11:49

Re: [PATCH v2] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)(address . 59513@debbugs.gnu.org)

Message-ID:87wn6wq4dl.fsf@cbaines.net

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

Toggle quote (15 lines)> Am Donnerstag, dem 08.12.2022 um 11:20 +0000 schrieb Christopher
> Baines:
>> Add more examples of when it can be appropriate to push changes
>> without
>> review, as I think this can be appropriate in the case of trivial
>> changes (as
>> mentioned before), but also non-trivial fixes.
>>
>> No longer suggest pushing simple new packages or package upgrades
>> (that don't cause lots of rebuilds) without sending to guix-patches.
>> Now there's some automation for testing changes sent to guix-patches,
>> sending changes there before pushing can mean that more rigerious
>
> rigorous

Thanks, I've fixed that locally now.

Toggle quote (71 lines)>> testing takes place and help speed up substitutes becoming available.
>> This is true, even if no human review takes place.
>>
>> Only suggest waiting one week for review for simpler changes, wait
>> two weeks
>> for more significant changes.
>>
>> Also, reorder some of the information in this section so it's grouped
>> together
>> better.
>>
>> * doc/contributing.texi (Commit Policy): Tweak.
>> ---
>> �doc/contributing.texi | 41 ++++++++++++++++++-----------------------
>> �1 file changed, 18 insertions(+), 23 deletions(-)
>>
>> diff --git a/doc/contributing.texi b/doc/contributing.texi
>> index 6a8ffd6524..d2e7abba98 100644
>> --- a/doc/contributing.texi
>> +++ b/doc/contributing.texi
>> @@ -1824,23 +1824,26 @@ It additionally calls @code{make check-
>> channel-news} to be sure
>>
>> �@subsection Commit Policy
>>
>> -If you get commit access, please make sure to follow
>> -the policy below (discussions of the policy can take place on
>> +If you get commit access, please make sure to follow the policy
>> below
>> +(discussions of the policy can take place on
>> �@email{guix-devel@@gnu.org}).
>>
>> -Non-trivial patches should always be posted to
>> -@email{guix-patches@@gnu.org} (trivial patches include fixing typos,
>> -etc.).� This mailing list fills the patch-tracking database
>> -(@pxref{Tracking Bugs and Patches}).
>> -
>> -For patches that just add a new package, and a simple one, it's OK
>> to
>> -commit, if you're confident (which means you successfully built it
>> in a
>> -chroot setup, and have done a reasonable copyright and license
>> -auditing).� Likewise for package upgrades, except upgrades that
>> trigger
>> -a lot of rebuilds (for example, upgrading GnuTLS or GLib).� We have
>> a
>> -mailing list for commit notifications (@email{guix-
>> commits@@gnu.org}),
>> -so people can notice.� Before pushing your changes, make sure to run
>> -@code{git pull --rebase}.
>> +Changes should be posted to @email{guix-patches@@gnu.org}.� This
>> mailing
>> +list fills the patch-tracking database (@pxref{Tracking Bugs and
>> +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{number}}, where
>> +@var{number} is the number assigned by the issue tracker.� 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.
>
> I would reword that so
>   (not significant ∧ confident ∧ qa_green) → good after 1 week
> whereas
>   (not significant ∧ confident ∧ qa_unknown) → good after 2 weeks
> and significant changes should anyway take 2 weeks.

While I like the intent here, for the moment I prefer a simpler

policy. Maybe we can move in this direction when the QA tooling is more

usable and reliable.

Thanks,

Chris

Ludovic Courtès wrote on 12 Dec 2022 12:47

Re: bug#59513: [PATCH] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 59513@debbugs.gnu.org)

Message-ID:87o7s8268v.fsf@gnu.org

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (10 lines)> Ludovic Courtès <ludo@gnu.org> writes:
>
>>>  @subsection Commit Policy
>>>
>>> -If you get commit access, please make sure to follow
>>> -the policy below (discussions of the policy can take place on
>>> +If you get commit access, please make sure to follow the policy below
>>> +(discussions of the policy can take place on
>>>  @email{guix-devel@@gnu.org}).

BTW, we should follow that policy :-) and send a heads-up on guix-devel.

Toggle quote (4 lines)

> I've gone for "tooling" rather than "robot" as I'm not sure we want to

> go the way of personifying it. I'm not against that, but the place to

> start is probably not here.

Fine with me!

Toggle quote (13 lines)>>   shows up on the dashboard at
>>   @indicateurl{https://qa.guix.gnu.org/issue/@var{number}}, where
>>   @var{number} is the number assigned by the issue tracker.  Leave time
>>   […] it’s OK to commit.
>>
>>   As an exception, some changes considered consensual and ``trivial'' or
>
> I removed "consensual" here as I wasn't sure what was meant by that, or
> at least I'm not sure the phrasing fits the context here.
>
> Are you trying to say something about a belief that no one will object
> to the change being made?

Yes, exactly. With experience in the project (like committers usually

have), you can often tell whether a given change might raise objections

or not.

Thanks for following up!

Ludo’.

Liliana Marie Prikler wrote on 12 Dec 2022 21:27

Re: [PATCH v2] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 59513@debbugs.gnu.org)

Message-ID:c5b768318fb6face8bd8686607149333f0ccd42c.camel@gmail.com

Am Montag, dem 12.12.2022 um 10:49 +0000 schrieb Christopher Baines:

Toggle quote (108 lines)> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Am Donnerstag, dem 08.12.2022 um 11:20 +0000 schrieb Christopher
> > Baines:
> > > Add more examples of when it can be appropriate to push changes
> > > without
> > > review, as I think this can be appropriate in the case of trivial
> > > changes (as
> > > mentioned before), but also non-trivial fixes.
> > > 
> > > No longer suggest pushing simple new packages or package upgrades
> > > (that don't cause lots of rebuilds) without sending to guix-
> > > patches.
> > > Now there's some automation for testing changes sent to guix-
> > > patches,
> > > sending changes there before pushing can mean that more rigerious
> > 
> > rigorous
> 
> Thanks, I've fixed that locally now.
> 
> > > testing takes place and help speed up substitutes becoming
> > > available.
> > > This is true, even if no human review takes place.
> > > 
> > > Only suggest waiting one week for review for simpler changes,
> > > wait
> > > two weeks
> > > for more significant changes.
> > > 
> > > Also, reorder some of the information in this section so it's
> > > grouped
> > > together
> > > better.
> > > 
> > > * doc/contributing.texi (Commit Policy): Tweak.
> > > ---
> > > �doc/contributing.texi | 41 ++++++++++++++++++-------------------
> > > ----
> > > �1 file changed, 18 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/doc/contributing.texi b/doc/contributing.texi
> > > index 6a8ffd6524..d2e7abba98 100644
> > > --- a/doc/contributing.texi
> > > +++ b/doc/contributing.texi
> > > @@ -1824,23 +1824,26 @@ It additionally calls @code{make check-
> > > channel-news} to be sure
> > > 
> > > �@subsection Commit Policy
> > > 
> > > -If you get commit access, please make sure to follow
> > > -the policy below (discussions of the policy can take place on
> > > +If you get commit access, please make sure to follow the policy
> > > below
> > > +(discussions of the policy can take place on
> > > �@email{guix-devel@@gnu.org}).
> > > 
> > > -Non-trivial patches should always be posted to
> > > -@email{guix-patches@@gnu.org} (trivial patches include fixing
> > > typos,
> > > -etc.).� This mailing list fills the patch-tracking database
> > > -(@pxref{Tracking Bugs and Patches}).
> > > -
> > > -For patches that just add a new package, and a simple one, it's
> > > OK
> > > to
> > > -commit, if you're confident (which means you successfully built
> > > it
> > > in a
> > > -chroot setup, and have done a reasonable copyright and license
> > > -auditing).� Likewise for package upgrades, except upgrades that
> > > trigger
> > > -a lot of rebuilds (for example, upgrading GnuTLS or GLib).� We
> > > have
> > > a
> > > -mailing list for commit notifications (@email{guix-
> > > commits@@gnu.org}),
> > > -so people can notice.� Before pushing your changes, make sure to
> > > run
> > > -@code{git pull --rebase}.
> > > +Changes should be posted to @email{guix-patches@@gnu.org}.� This
> > > mailing
> > > +list fills the patch-tracking database (@pxref{Tracking Bugs and
> > > +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{number}}, where
> > > +@var{number} is the number assigned by the issue tracker.� 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.
> > 
> > I would reword that so
> > � (not significant ∧ confident ∧ qa_green) → good after 1 week
> > whereas
> > � (not significant ∧ confident ∧ qa_unknown) → good after 2 weeks
> > and significant changes should anyway take 2 weeks.
> 
> While I like the intent here, for the moment I prefer a simpler
> policy. Maybe we can move in this direction when the QA tooling is
> more usable and reliable.

I wrote this partly with the intent of resolving an ambiguity, but I
agree on the principle of having a simple policy.  I take it that QA
status is not that important at the moment – more that QA knows about
the patch and has already started building packages?

Cheers

Christopher Baines wrote on 13 Dec 2022 15:06

Recipients:(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)(address . 59513@debbugs.gnu.org)

Message-ID:87h6xzo0ot.fsf@cbaines.net

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

Toggle quote (33 lines)>> > > +Changes should be posted to @email{guix-patches@@gnu.org}.� This
>> > > mailing
>> > > +list fills the patch-tracking database (@pxref{Tracking Bugs and
>> > > +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{number}}, where
>> > > +@var{number} is the number assigned by the issue tracker.� 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.
>> >
>> > I would reword that so
>> > � (not significant ∧ confident ∧ qa_green) → good after 1 week
>> > whereas
>> > � (not significant ∧ confident ∧ qa_unknown) → good after 2 weeks
>> > and significant changes should anyway take 2 weeks.
>>
>> While I like the intent here, for the moment I prefer a simpler
>> policy. Maybe we can move in this direction when the QA tooling is
>> more usable and reliable.
>
> I wrote this partly with the intent of resolving an ambiguity, but I
> agree on the principle of having a simple policy.  I take it that QA
> status is not that important at the moment – more that QA knows about
> the patch and has already started building packages?

I don't think the QA stuff happening is important enough make some
requirements of it in the policy (yet), but, as you say, there are still
benefits right now with submitting patches to guix-patches, and that's
what I'm going for here.

Vagrant Cascadian wrote on 14 Dec 2022 01:54

Re: [bug#59513] [PATCH v2] doc: contributing: Tweak the Commit Policy.

Recipients:

Message-ID:87bko64xfq.fsf@contorta

On 2022-12-08, Christopher Baines wrote:

Toggle quote (2 lines)

> Only suggest waiting one week for review for simpler changes, wait two weeks

> for more significant changes.

...

Toggle quote (11 lines)> +Changes should be posted to @email{guix-patches@@gnu.org}.  This mailing
> +list fills the patch-tracking database (@pxref{Tracking Bugs and
> +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{number}}, where
> +@var{number} is the number assigned by the issue tracker.  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.

My one concern here for things that I tend to work on is

diffoscope... it has such a large dependency graph(?) because it

supports so many file formats, it pulls in quite a lot for the test

suites...

In a week or two of changes between submission and being able to push to

master, I'd worry that you could end up with a diffoscope that wouldn't

build because of changes to one of it's (native-)inputs or whatnot

because of changes to master in the previous week...

That said, overall, I think sending everything through guix-patches is a

good change, even if my lazier self pouts a little at having to deal

with more process for seemingly simple things. :)

live well,

vagrant

-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQRlgHNhO/zFx+LkXUXcUY/If5cWqgUCY5keugAKCRDcUY/If5cW

qpWkAQCX2+M36ZpVyU3VYOUE6Hgy1e+T2kxyZfqqs1mME20qxAD8Cg2g797J/DBT

NCD1tagyClhql+OZVOHowzjYbhx5ggs=

=xLb/

-----END PGP SIGNATURE-----

Christopher Baines wrote on 14 Dec 2022 11:21

Recipients:(name . Vagrant Cascadian)(address . vagrant@debian.org)(address . 59513@debbugs.gnu.org)

Message-ID:878rjanuzd.fsf@cbaines.net

Vagrant Cascadian <vagrant@debian.org> writes:

Toggle quote (31 lines)> [[PGP Signed Part:Undecided]]
> On 2022-12-08, Christopher Baines wrote:
>> Only suggest waiting one week for review for simpler changes, wait two weeks
>> for more significant changes.
> ...
>> +Changes should be posted to @email{guix-patches@@gnu.org}.  This mailing
>> +list fills the patch-tracking database (@pxref{Tracking Bugs and
>> +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{number}}, where
>> +@var{number} is the number assigned by the issue tracker.  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.
>
> My one concern here for things that I tend to work on is
> diffoscope... it has such a large dependency graph(?) because it
> supports so many file formats, it pulls in quite a lot for the test
> suites...
>
> In a week or two of changes between submission and being able to push to
> master, I'd worry that you could end up with a diffoscope that wouldn't
> build because of changes to one of it's (native-)inputs or whatnot
> because of changes to master in the previous week...
>
>
> That said, overall, I think sending everything through guix-patches is a
> good change, even if my lazier self pouts a little at having to deal
> with more process for seemingly simple things. :)

I think that's a valid concern. The QA tooling is affected similarly, in

that it tests against the latest processed revision when the patch is

picked up, but things could change in between the testing happening and

it being merged.

Remember that these time periods are only when no review takes place. My

hope is that manual review can happen sooner than one or two weeks after

patch submission, therefore enabling making changes more quickly.

Thanks,

Chris

Maxim Cournoyer wrote on 17 Dec 2022 06:01

Re: bug#59513: [PATCH] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 59513@debbugs.gnu.org)

Message-ID:87o7s2aajn.fsf_-_@gmail.com

Hi Christopher,

Christopher Baines <mail@cbaines.net> writes:

[...]

Toggle quote (49 lines)> * doc/contributing.texi (Commit Policy): Tweak.
> ---
>  doc/contributing.texi | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/doc/contributing.texi b/doc/contributing.texi
> index 6a8ffd6524..d2e7abba98 100644
> --- a/doc/contributing.texi
> +++ b/doc/contributing.texi
> @@ -1824,23 +1824,26 @@ It additionally calls @code{make check-channel-news} to be sure
>  
>  @subsection Commit Policy
>  
> -If you get commit access, please make sure to follow
> -the policy below (discussions of the policy can take place on
> +If you get commit access, please make sure to follow the policy below
> +(discussions of the policy can take place on
>  @email{guix-devel@@gnu.org}).
>  
> -Non-trivial patches should always be posted to
> -@email{guix-patches@@gnu.org} (trivial patches include fixing typos,
> -etc.).  This mailing list fills the patch-tracking database
> -(@pxref{Tracking Bugs and Patches}).
> -
> -For patches that just add a new package, and a simple one, it's OK to
> -commit, if you're confident (which means you successfully built it in a
> -chroot setup, and have done a reasonable copyright and license
> -auditing).  Likewise for package upgrades, except upgrades that trigger
> -a lot of rebuilds (for example, upgrading GnuTLS or GLib).  We have a
> -mailing list for commit notifications (@email{guix-commits@@gnu.org}),
> -so people can notice.  Before pushing your changes, make sure to run
> -@code{git pull --rebase}.
> +Changes should be posted to @email{guix-patches@@gnu.org}.  This mailing
> +list fills the patch-tracking database (@pxref{Tracking Bugs and
> +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{number}}, where
> +@var{number} is the number assigned by the issue tracker.  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.
> +
> +As an exception, some changes considered ``trivial'' or ``obvious'' may
> +be pushed directly.  This includes changes to fix typos and reverting
> +commits that caused immediate problems.  This is subject to being
> +adjusted, allowing individuals to commit directly on non-controversial
> +changes on parts they’re familiar with.

Like others, I like the direction of the change; the focus is changed
from "trivial patches are OK to push else wait 2 weeks" to "most changes
must go through the QA tooling", which should improve quality.  Like
Vagrant, I think it adds some friction, especially if the QA is still
sometimes still unreliable and doesn't provide clear results (false
positives for example), but I'm not against trying it.

I guess we can try this new process, and adjust as we go (or revert to
the current policy) in case something doesn't work well enough.

-- 
Thanks,
Maxim

Ludovic Courtès wrote on 20 Dec 2022 11:55

Re: [bug#59513] [PATCH v2] doc: contributing: Tweak the Commit Policy.

Recipients:(name . Vagrant Cascadian)(address . vagrant@debian.org)

Message-ID:877cymbazo.fsf@gnu.org

Howdy,

Vagrant Cascadian <vagrant@debian.org> skribis:

Toggle quote (25 lines)> On 2022-12-08, Christopher Baines wrote:
>> Only suggest waiting one week for review for simpler changes, wait two weeks
>> for more significant changes.
> ...
>> +Changes should be posted to @email{guix-patches@@gnu.org}.  This mailing
>> +list fills the patch-tracking database (@pxref{Tracking Bugs and
>> +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{number}}, where
>> +@var{number} is the number assigned by the issue tracker.  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.
>
> My one concern here for things that I tend to work on is
> diffoscope... it has such a large dependency graph(?) because it
> supports so many file formats, it pulls in quite a lot for the test
> suites...
>
> In a week or two of changes between submission and being able to push to
> master, I'd worry that you could end up with a diffoscope that wouldn't
> build because of changes to one of it's (native-)inputs or whatnot
> because of changes to master in the previous week...

I suppose there’s always this risk. Ideally, qa.guix would either

rebuild things periodically (rebasing them) or could be told to.

Toggle quote (4 lines)

> That said, overall, I think sending everything through guix-patches is a

> good change, even if my lazier self pouts a little at having to deal

> with more process for seemingly simple things. :)

Right, I can sympathize. :-) I’ve done my share of direct pushes for

“simple thing”, but I think there’s now evidence that sometimes simple

things aren’t that simple.

Waiting for a green flag from qa.guix and/or from fellow hackers might

seem annoying at first, but the gains in terms of peace of mind, smooth

collaboration, and overall stability are worth it.

Ludo’.

zimoun wrote on 5 Jan 2023 10:12

Recipients:

Message-ID:868rih9wfi.fsf@gmail.com

Hi,

On Thu, 08 Dec 2022 at 11:20, Christopher Baines <mail@cbaines.net> wrote:

Toggle quote (4 lines)

> Add more examples of when it can be appropriate to push changes without

> review, as I think this can be appropriate in the case of trivial changes (as

> mentioned before), but also non-trivial fixes.

This would be part of the commit message, right?

I would avoid the personal “I think” since it is a collective thought

with some consensus (I guess). For instance,

Toggle snippet (6 lines)

Add more examples of when it can be appropriate to push changes without

review, as in the case of trivial changes (as mentioned before), but

also non-trivial fixes.

Toggle quote (4 lines)

> No longer suggest pushing simple new packages or package upgrades (that don't

> cause lots of rebuilds) without sending to guix-patches. Now there's some

> automation for testing changes sent to guix-patches, sending changes there

> before pushing can mean that more rigerious testing takes place and help speed

--^

typo

s/rigerious/rigorous

Toggle quote (8 lines)

> +Changes should be posted to @email{guix-patches@@gnu.org}. This mailing

> +list fills the patch-tracking database (@pxref{Tracking Bugs and

> +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{number}}, where

> +@var{number} is the number assigned by the issue tracker. Leave time

To be consistent with (guix) Sending a Patch Series [1], I suggest to

use @var{ISSUE_NUMBER} instead of simply @var{number}.

1: https://guix.gnu.org/manual/devel/en/guix.html#Sending-a-Patch-Series

Aside the minor comments, all LGTM! This change appears to me a great

improvement, hoping it will reduce the number of red bullets in

dashboard [2].

2: https://ci.guix.gnu.org/eval/91795/dashboard

Cheers,

simon

Christopher Baines wrote on 11 Jan 2023 11:48

Recipients:(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 59513@debbugs.gnu.org)

Message-ID:875yddfiqw.fsf@cbaines.net

zimoun <zimon.toutoune@gmail.com> writes:

Toggle quote (10 lines)> Hi,
>
> On Thu, 08 Dec 2022 at 11:20, Christopher Baines <mail@cbaines.net> wrote:
>
>> Add more examples of when it can be appropriate to push changes without
>> review, as I think this can be appropriate in the case of trivial changes (as
>> mentioned before), but also non-trivial fixes.
>
> This would be part of the commit message, right?

Yeah, this is the commit message.

Toggle quote (7 lines)

> I would avoid the personal “I think” since it is a collective thought

> with some consensus (I guess). For instance,

> Add more examples of when it can be appropriate to push changes without

> review, as in the case of trivial changes (as mentioned before), but

> also non-trivial fixes.

It reads better to me with the "I think", and since it's my name on the

commit, I've left it as is.

Toggle quote (8 lines)

>> No longer suggest pushing simple new packages or package upgrades (that don't

>> cause lots of rebuilds) without sending to guix-patches. Now there's some

>> automation for testing changes sent to guix-patches, sending changes there

>> before pushing can mean that more rigerious testing takes place and help speed

> --^

> typo

> s/rigerious/rigorous

Thanks, I've fixed this.

Toggle quote (13 lines)>> +Changes should be posted to @email{guix-patches@@gnu.org}.  This mailing
>> +list fills the patch-tracking database (@pxref{Tracking Bugs and
>> +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{number}}, where
>> +@var{number} is the number assigned by the issue tracker.  Leave time
>
> To be consistent with (guix) Sending a Patch Series [1], I suggest to
> use @var{ISSUE_NUMBER} instead of simply @var{number}.
>
> 1: <https://guix.gnu.org/manual/devel/en/guix.html#Sending-a-Patch-Series>

Sure, I've made this change.

Christopher Baines wrote on 11 Jan 2023 11:50

Recipients:(address . 59513-done@debbugs.gnu.org)

Message-ID:871qo1fioh.fsf@cbaines.net

Christopher Baines <mail@cbaines.net> writes:

Toggle quote (22 lines)> Add more examples of when it can be appropriate to push changes without
> review, as I think this can be appropriate in the case of trivial changes (as
> mentioned before), but also non-trivial fixes.
>
> No longer suggest pushing simple new packages or package upgrades (that don't
> cause lots of rebuilds) without sending to guix-patches. Now there's some
> automation for testing changes sent to guix-patches, sending changes there
> before pushing can mean that more rigerious testing takes place and help speed
> up substitutes becoming available. This is true, even if no human review takes
> place.
>
> Only suggest waiting one week for review for simpler changes, wait two weeks
> for more significant changes.
>
> Also, reorder some of the information in this section so it's grouped together
> better.
>
> * doc/contributing.texi (Commit Policy): Tweak.
> ---
>  doc/contributing.texi | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)

I've now pushed this to master as

9aa2b7419854306b7ae78d4c4f7684316b834b1d, with some final tweaks.

Thanks everyone for taking a look.

Chris

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 59513@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 59513

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date