bug#66268: Guix makes invalid assumptions regarding guile-git guarantees leading to guix pull failing

[wolf <wolf@wolfsden.cz>]

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

On 2023-09-30 17:48:55 +0200, Simon Tournier wrote:
> Hi,
> 
> Hum, the updates seem:
> 
>  + libgit2 on Feb 17 2023,
>  + guile-git on May 15 2022.
> 
> (See 8d8e1438ae5a2e50005b500dacd0a26be540fe69 and
> b6bfe9ea6a1b19159455b34f1af4ac00ef9b94ab)
> 
> And some commits with large body are around:
> 
>  + 7b45ead9ec40a5ea1ef8332d55c2bb4beff85eb5 from Jul 18 2023
>  + 1e6ddceb8318d413745ca1c9d91fde01b1e0364b from Feb 19 2023
>  + 5897d873d0c902f08d13c38500eff11098f2a634 from Aug 10 2022
> 
> And I have not investigated more about their commit object size.  Just
> counting the number of characters per commit message.  The one you
> provided is about 3030, if I am correct.  Here, let filter with the
> criteria of 4500, why not. :-)
> 
> --8<---------------cut here---------------start------------->8---
> $ for ci in $(git log --format=%H --after=2022-05-13); do \
>     echo "$(git show -s $ci | wc -c) $ci"                 \
>         | awk '$1>4500{print $2 " " $1}'                  \
> ;done 
> 7b45ead9ec40a5ea1ef8332d55c2bb4beff85eb5 4997
> 1e6ddceb8318d413745ca1c9d91fde01b1e0364b 16120
> 575a03ab3997edee08d20867228e886043d5240f 5511
> 5897d873d0c902f08d13c38500eff11098f2a634 6258
> --8<---------------cut here---------------end--------------->8---
> 
> Well, it is probably not a regression.  Or I am missing some details. :-)

Thank you for raising this up and making me look into it closer.  The issue
(commits not being eq? to themselves) does happen for those listed above,
however commit-relation still works fine.  I am unsure why, I spent most of
today on it and did not manage to find clear rules.

However the fact remains that when one is on
d51135e8477118dc63a7e5462355cd27e884f4fb, guix pull to
4dbd25fa0e09b40ba2ab01d1e64fa36db652b501 does fail.  I pushed those commits into
https://git.sr.ht/~graywolf/guix-guile-git-repro as branch xxx in case anyone is
curious and wants to investigate.

> 
> I am probably overlooking something, from my understanding, the issue
> arises for some corner cases when the bound of the closure does not fit
> ’eq?’.  For these cases, instead of relying on ’setq’ using ’eq?’, we
> could rely on ’set’ using ’equal?’.  No?

I do not believe so, the mismatch happens even for equal?.  I do not know enough
about scheme to know whether guile-git could override equal? for the commit
record type, but it does not seem to do so.

    scheme@(guile-user)> (use-modules (git) (guix git))
    scheme@(guile-user)> (define %repo (repository-open "/home/wolf/src/guix"))
    scheme@(guile-user)> (define %hash "1e6ddceb8318d413745ca1c9d91fde01b1e0364b")
    scheme@(guile-user)> (equal? (commit-lookup %repo (string->oid %hash)) (commit-lookup %repo (string->oid %hash)))
    $1 = #f

> 
> On Fri, 29 Sep 2023 at 18:52, wolf <wolf@wolfsden.cz> wrote:
> 
> >   ,----
> >   | scheme@(guile-user)> (use-modules (git) (guix git))
> >   | scheme@(guile-user)> (define %repo (repository-open "/tmp/my-fork"))
> >   | scheme@(guile-user)> (define %hash "d51135e8477118dc63a7e5462355cd27e884f4fb")
> >   | scheme@(guile-user)> (commit-relation
> >   |  (commit-lookup %repo (string->oid %hash))
> >   |  (commit-lookup %repo (string->oid %hash)))
> >   | $5 = unrelated
> >   `----
> 
> [...]
> 
> >   ,----
> >   | $ git merge-base --is-ancestor 9b985229bcd 71f544c102a; echo $?
> >   | 0
> >   `----
> 
> [...]
> 
> > 2 Possible solutions
> > ====================
> 
> Naive question.  Why not rely on “git merge-base --is-ancestor” for
> implementing “commit-relation”?
> 
> Since f651a359691cbe4750f1fe8d14dd964f7971f91 from Sep 26 2023:
> 
>     build: Add dependency on Git.
> 
>     * configure.ac: Check for ‘git’ and substitute ‘GIT’.
>     * guix/config.scm.in (%git): New variable.
>     * guix/self.scm (compiled-guix): Define ‘git’ and pass it to
>     ‘make-config.scm’.
>     (make-config.scm): Add #:git; emit a ‘%git’ variable.
>     * doc/guix.texi (Requirements): Add it.
> 
> we can assume Git is available by the code that run “commit-relation”,
> no?
> 
> The implementation relying on “git merge-base --is-ancestor” does not
> have the problem, right?

Well, that is true.  I forgot to mention this option, because there did not seem
to be a consensus regarding replacing more parts of guile-git with git proper.
But this would likely be the best solution if that approach is acceptable.

> 
> And from [1], it is 35x faster.  Win-win, no?  Because the fix for ’eq?’
> will introduce performance cost and ’commit-relation’ will be even
> slower, no?

For what it is worth, the implementation using <commit-set> (I needed it for my
fork to be able to pull) is not noticeably slower.  The slow down is likely
measurable, but since I did not even notice it I did not bother measuring it.

Not that I am arguing against using git.

> 
> Well, I do not know.  My words are probably irrelevant.
> 
> Cheers,
> simon
> 
> 
> 1: comparing commit-relation using Scheme+libgit2 vs shellout plumbing Git
> Simon Tournier <zimon.toutoune@gmail.com>
> Tue, 12 Sep 2023 00:48:30 +0200
> id:865y4gz5q9.fsf@gmail.com
> https://lists.gnu.org/archive/html/guix-devel/2023-09
> https://yhetil.org/guix/865y4gz5q9.fsf@gmail.com

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

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