Re: How can we decrease the cognitive overhead for contributors?

[Simon Tournier]

Hi Maxim,

On Tue, 12 Sep 2023 at 10:42, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>> Even before looking at it, I have to spend some time to find a way to
>> manually apply the patches.  Then, rebasing on the top of master could
>> lead to conflict but that another story and the same appears whatever
>> the workflow.
>
> I think this kind of problem has improved though, since we deploy by
> default our etc/git/config snippet, which sets 'useAutoBase = true'.
>
> At least I don't have a problem when applying patches with 'git am -3s'.

We are drifting. :-)

This branch of the looooong thread starts with:

        Re: How can we decrease the cognitive overhead for contributors?
        Attila Lendvai <attila@lendvai.name>
        Fri, 25 Aug 2023 08:07:53 +0000
        
id:JRUs8LVm3AtAh0MnHjE5rnhB4sNET0vWTOO2N3w2KfvKoM3CALRNwHTmwJ0Y9Bg3ZDWCs8j-1bMCo9aRiaoac8TAkuXAvrWgSwt_8RcwhQA=@lendvai.name
        
https://yhetil.org/guix/JRUs8LVm3AtAh0MnHjE5rnhB4sNET0vWTOO2N3w2KfvKoM3CALRNwHTmwJ0Y9Bg3ZDWCs8j-1bMCo9aRiaoac8TAkuXAvrWgSwt_8RcwhQA=@lendvai.name
        https://lists.gnu.org/archive/html/guix-devel/2023-08

Attila speaks about the PR model.  Then many comments later about the
pros and cons, the discussion is split intro the contributor side and
the reviewer side of the PR model.  Ricardo reports then their
experience reviewing Pull Request:

        Re: How can we decrease the cognitive overhead for contributors?
        Ricardo Wurmus <rekado@elephly.net>
        Fri, 08 Sep 2023 16:44:41 +0200
        id:87sf7o67ia.fsf@elephly.net
        87sf7o67ia.fsf@elephly.net">https://yhetil.org/guix/87sf7o67ia.fsf@elephly.net
        https://lists.gnu.org/archive/html/guix-devel/2023-09

And I provide one typical example where “our“ model leads to some
friction for the reviewer: the first step, apply the patches.

And this exact same friction does not exist in the PR model by design of
this very PR model.  I am not saying the PR model is perfect and I am
not saying “our” workflow is crappy.  I just agree with Ricardo
messages.

Now, yes this kind of problem is improving.  Please note that the
problem is not new and I am aware of it. :-) I submitted on October 2020
the first mention of ‘git format-patch --base’ in the “Submitting
Patches” section and d18b787d8a5cada8d239f1335257d0c1bca7f825 had been
pushed only on September 2021.

For whatever reason, sometimes patches fall into some cracks or are
badly formatted or something else.  Then, applying them becomes boring.
I am not saying that’s impossible, I am just saying that sometimes we
have a friction (something that has no value for the project) because of
our workflow.

I am not speaking on the vacuum of an hypothetical problem but I am
using a concrete example patch#62202 from my own pile.

Using the usual “git am -3s” from Emacs Debbugs, then I get:

--8<---------------cut here---------------start------------->8---
Applying: import: juliahub: first script draft.
error: sha1 information is lacking or useless (guix/import/go.scm).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 import: juliahub: first script draft.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
--8<---------------cut here---------------end--------------->8---

Ok, maybe that’s because some empty commit message (I did not know that
and I had to search using search engine, well I do not know), let’s try
with “git apply”, then I get:

--8<---------------cut here---------------start------------->8---
error: patch failed: guix/import/go.scm:503
error: guix/import/go.scm: patch does not apply
--8<---------------cut here---------------end--------------->8---

Ok, that’s maybe this file had been modified.  So I do,

--8<---------------cut here---------------start------------->8---
$ git log --format="%h %cd %s" -- guix/import/go.scm
ae587c2ef041 Mon Mar 13 15:08:33 2023 +0100 guix: Strip #:use-module lists.
3c24da4260f2 Sat Dec 31 14:48:46 2022 +0100 import/utils: Pass all arguments 
through to package builder.
c1db66fa0a17 Sun Jan 9 23:17:17 2022 +0100 import: go: Correctly report 
diagnostics upon version mismatch.
--8<---------------cut here---------------end--------------->8---

And I checkout ae587c2ef041.  Same error.  The parent of ae587c2ef041.
Same error.  And I checkout 3c24da4260f2.  Same error.  Ok, do I try
with b4?

Well, at this point, do we agree there is useless friction? :-)

Maybe I am missing the obvious.  Maybe that’s a rare case.  That’s just
one from my pile.  And I see that time to time.

After 15 minutes today (and 10-15 minutes on April, 7th = 24 days after
the submission), I just give up.  And it ends on some eternal postponing
on my side; until I will just copy manually the 21 patches and manually
recreate the 21 commits.

Ricardo used the term “superior” and Liliana asked a definition.  That’s
the origin of my email.  Instead of a definition, I was pointing (and
again here :-)) an example where “our” workflow just fails when the PR
model could not by design.

Last, do not take me wrong.

 1. I am happy with “our” workflow.
 2. I recognize that “our” workflow leads to frictions – and I consider
    that I know enough Emacs lisp or Bash for writing small helpers.
 3. The submission I am using as example is a gift, I am happy that the
    project receives such gift.  The failure is on my side or on “our”
    workflow.
 4. Things are improving, hopefully. :-)  However, it remains very
    annoying corner-cases in our workflow that will be very hard to
    tackle.  For sure, Mumi subcommand could help.
 5. I would be happy if someone could send some v2 version providing
    enough information to be able to just apply.

About #4, as I said elsewhere, I think the most promising is to wrap
something like:

    guix time-machine \
      --url=https://git.guix-patches.cbaines.net/git/guix-patches \
      --branch=issue-123456 --disable-authentication \
      -- build <foo>

and probably via Mumi.

My 2 cents on the reviewing process.

Cheers,
simon