Subject: Re: [PATCH 1/2] git: Use ‘graph-descendant?’ from Guile-Git >= 0.10.0 when available.

From: Ludovic Courtès <[email-masked]>
Date: Mon, 21 Apr 2025 23:26:56 +0200

Hi Tomas,

Tomas Volf <[email-masked]> writes:

> Ludovic Courtès <[email-masked]> writes:
>
>> Fixes <https://issues.guix.gnu.org/66268>.
>>
>> Fixes a bug whereby ‘commit-relation’ and ‘commit-descendant?’ would
>> provide an incorrect result when two distinct <commit> objects would
>> exist for the same commit, which can happen when the commit’s metadata
>> is beyond 4 KiB, as of libgit2 1.8/1.9.
>
> Ooh, so this *used to work* in <1.8, interesting, I had no idea. :)

No no; what I meant is that this is known to be the case with 1.8 and
1.9, but I haven't checked with other versions.

>> +  (if (resolve-module '(guix graph) #:ensure #f)  ;Guile-Git >= 0.10.0
>
> Two notes here:
>
> 1. Should this not be '(git graph)?

Oops, yes!  Fixed locally.

> 2. I thought that when you do `guix pull`, the new Guix is built against
> the packages in the newly pulled version.  So everyone who pulls these
> changes, should also pull the guile-git 0.10.0 no?  It should not be
> possible to end up in "new guix, old guile-git" situation.  So I do not
> understand why you need both branches.

That's correct.  The second branch is a convenience for developers as I
wrote, typically for someone developing in “guix shell” but from a Guix
that doesn’t have 0.10.0 yet.  We can remove that second branch in a few
weeks.

> How is the `graph-descendant?' set?

It’s from the (git) module, automatically pulled in when available.

>> +      (lambda (new old)
>> +        "Return true if NEW is the descendant of one of OLD, a list of
>> +commits."
>> +        (let ((repository (commit-owner new))
>> +              (new (commit-id new)))
>> +          (any (lambda (old)
>> +                 (let ((old (commit-id old)))
>> +                   (or (graph-descendant? repository new old)
>> +                       (oid=? old new))))
>> +               old)))
>
> I would be tempted to write this in terms of commit-relation and memq
> with '(self descendant).  Beside being slightly easier to grasp (for
> me), it would allow you to have just a single branch, since
> commit-relation is available regardless of guile-git version.  Not sure
> if there would be performance impact in <0.10.0 though.

I thought about this but I think the implementation above is slightly
more efficient, and since the "else" branch will be removed soon, let's
not bother.

Sending v2 right away.

Thanks,
Ludo'.