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

[zimoun <zimon.toutoune@gmail.com>]

Hi,

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


>>         … 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. :-)


> 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.


>> 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,

--8<---------------cut here---------------start------------->8---
$ 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
--8<---------------cut here---------------end--------------->8---

(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,

--8<---------------cut here---------------start------------->8---
$ 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
--8<---------------cut here---------------end--------------->8---

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>


>> 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