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

Message received at 66268 <at> debbugs.gnu.org:

Received: (at 66268) by debbugs.gnu.org; 2 Oct 2023 09:21:23 +0000
From debbugs-submit-bounces <at> debbugs.gnu.org Mon Oct 02 05:21:22 2023
Received: from localhost ([127.0.0.1]:36039 helo=debbugs.gnu.org)
	by debbugs.gnu.org with esmtp (Exim 4.84_2)
	(envelope-from <debbugs-submit-bounces <at> debbugs.gnu.org>)
	id 1qnF79-0007Qz-12
	for submit <at> debbugs.gnu.org; Mon, 02 Oct 2023 05:21:22 -0400
Received: from mail-wr1-x436.google.com ([2a00:1450:4864:20::436]:40407)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <zimon.toutoune@HIDDEN>) id 1qnF71-0007Q5-IX
 for 66268 <at> debbugs.gnu.org; Mon, 02 Oct 2023 05:21:15 -0400
Received: by mail-wr1-x436.google.com with SMTP id
 ffacd0b85a97d-32163c3ece5so1990525f8f.1
 for <66268 <at> debbugs.gnu.org>; Mon, 02 Oct 2023 02:20:54 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=gmail.com; s=20230601; t=1696238449; x=1696843249; darn=debbugs.gnu.org;
 h=content-transfer-encoding:mime-version:message-id:date:references
 :in-reply-to:subject:to:from:from:to:cc:subject:date:message-id
 :reply-to; bh=x9FCYtfbJ2+U16ZKW8LQb0DAS1/6j9f2E0l9cczmbs0=;
 b=gewYlxeArhBTVbJZlNTtT2DprV8LBPx+uGBGcezjcioeHGhnf+aiBptVWumygaylSS
 Qf35wNwW8WPghb59GgrsSz05f9grxltiT0RkQwR2qf9DAV+ubdjdJczj8YUaHxIEi4wD
 a47a/9Q3ysJGJhfQ/Oxv6J9ozopkBSpclFGVWlnm9BzvE8IEZuhIrOKoyUMCT3S8ge1Q
 K7trknlvxylPUC9lIpcvhTtCDFL3Lo6ukidZDZsdGZweKjsaTY3ERkylANi32IpCiWAB
 OhyYsVgfLs/Z+NcFX8vZDUCRS0YLVrCafPKqMyWzJg7UHvOb+qIvh6gZRF9MvHqTCTrw
 Nb6w==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20230601; t=1696238449; x=1696843249;
 h=content-transfer-encoding:mime-version:message-id:date:references
 :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject
 :date:message-id:reply-to;
 bh=x9FCYtfbJ2+U16ZKW8LQb0DAS1/6j9f2E0l9cczmbs0=;
 b=j2m1rkuOJYhyk/pNtttE49xkw/FVmuMXOEK3avS9lXc4XjrI8d+aShnhBs/qFn/DQC
 +QPk5mMvM9EC5PvN2sn4RwE1LNbxSmIC7WRLM0ZCLqwkYjDJc/PvVS0Mgka4GSQvmRJF
 GsYilNFokhu87vaZtdXBJbcmf13ag4X5jnAVrnZttAEkggU7wq8gacETVxug+aWAAMk5
 ouFPrWkugDXomveruvqRivP/ZXP7bu2LfbxG2CWZjkS8VlMQ921RUUuGO/07nQyS25gJ
 iuo745HXp7gvBpyJfVgfD8TJJg3CLVURuQQ+oCzQ5fdzPn4MlvpNFIkOuylxen1TCNyr
 WYeQ==
X-Gm-Message-State: AOJu0Yzlyie6GPn7Rx6aSYpvE/RFWW5qWTbkp3qlx6sj/A4PoJ9Y6q25
 jDvsc6IrITcn9HfoZ4T1Fu1GIc84cls=
X-Google-Smtp-Source: AGHT+IFgStVQGZgQUZ5jgCIwBmQM98xj5vb+9fYWjOqLUaGwlBvUwb/Y9u41Dduu513HSivOP2hVig==
X-Received: by 2002:a5d:6909:0:b0:322:5251:d78b with SMTP id
 t9-20020a5d6909000000b003225251d78bmr8945769wru.6.1696238447809; 
 Mon, 02 Oct 2023 02:20:47 -0700 (PDT)
Received: from lili ([2a01:e0a:59b:9120:65d2:2476:f637:db1e])
 by smtp.gmail.com with ESMTPSA id
 p5-20020a5d4585000000b0031ad5470f89sm27581510wrq.18.2023.10.02.02.20.47
 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
 Mon, 02 Oct 2023 02:20:47 -0700 (PDT)
From: Simon Tournier <zimon.toutoune@HIDDEN>
To: wolf <wolf@HIDDEN>, 66268 <at> debbugs.gnu.org
Subject: Re: bug#66268: Guix makes invalid assumptions regarding guile-git
 guarantees leading to guix pull failing
In-Reply-To: <ZRcA23RUYvBuE1JX@ws>
References: <ZRcA23RUYvBuE1JX@ws>
Date: Sat, 30 Sep 2023 17:48:55 +0200
Message-ID: <86fs2vek60.fsf@HIDDEN>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
X-Spam-Score: 0.5 (/)
X-Debbugs-Envelope-To: 66268
X-BeenThere: debbugs-submit <at> debbugs.gnu.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: <debbugs-submit.debbugs.gnu.org>
List-Unsubscribe: <https://debbugs.gnu.org/cgi-bin/mailman/options/debbugs-submit>, 
 <mailto:debbugs-submit-request <at> debbugs.gnu.org?subject=unsubscribe>
List-Archive: <https://debbugs.gnu.org/cgi-bin/mailman/private/debbugs-submit/>
List-Post: <mailto:debbugs-submit <at> debbugs.gnu.org>
List-Help: <mailto:debbugs-submit-request <at> debbugs.gnu.org?subject=help>
List-Subscribe: <https://debbugs.gnu.org/cgi-bin/mailman/listinfo/debbugs-submit>, 
 <mailto:debbugs-submit-request <at> debbugs.gnu.org?subject=subscribe>
Errors-To: debbugs-submit-bounces <at> debbugs.gnu.org
Sender: "Debbugs-submit" <debbugs-submit-bounces <at> debbugs.gnu.org>
X-Spam-Score: -0.5 (/)

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=3D%H --after=3D2022-05-13); do \
    echo "$(git show -s $ci | wc -c) $ci"                 \
        | awk '$1>4500{print $2 " " $1}'                  \
;done=20
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. :-)

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

On Fri, 29 Sep 2023 at 18:52, wolf <wolf@HIDDEN> wrote:

>   ,----
>   | scheme@(guile-user)> (use-modules (git) (guix git))
>   | scheme@(guile-user)> (define %repo (repository-open "/tmp/my-fork"))
>   | scheme@(guile-user)> (define %hash "d51135e8477118dc63a7e5462355cd27e=
884f4fb")
>   | scheme@(guile-user)> (commit-relation
>   |  (commit-lookup %repo (string->oid %hash))
>   |  (commit-lookup %repo (string->oid %hash)))
>   | $5 =3D unrelated
>   `----

[...]

>   ,----
>   | $ git merge-base --is-ancestor 9b985229bcd 71f544c102a; echo $?
>   | 0
>   `----

[...]

> 2 Possible solutions
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Naive question.  Why not rely on =E2=80=9Cgit merge-base --is-ancestor=E2=
=80=9D for
implementing =E2=80=9Ccommit-relation=E2=80=9D?

Since f651a359691cbe4750f1fe8d14dd964f7971f91 from Sep 26 2023:

    build: Add dependency on Git.

    * configure.ac: Check for =E2=80=98git=E2=80=99 and substitute =E2=80=
=98GIT=E2=80=99.
    * guix/config.scm.in (%git): New variable.
    * guix/self.scm (compiled-guix): Define =E2=80=98git=E2=80=99 and pass =
it to
    =E2=80=98make-config.scm=E2=80=99.
    (make-config.scm): Add #:git; emit a =E2=80=98%git=E2=80=99 variable.
    * doc/guix.texi (Requirements): Add it.

we can assume Git is available by the code that run =E2=80=9Ccommit-relatio=
n=E2=80=9D,
no?

The implementation relying on =E2=80=9Cgit merge-base --is-ancestor=E2=80=
=9D does not
have the problem, right?

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

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@HIDDEN>
Tue, 12 Sep 2023 00:48:30 +0200
id:865y4gz5q9.fsf@HIDDEN
https://lists.gnu.org/archive/html/guix-devel/2023-09
https://yhetil.org/guix/865y4gz5q9.fsf@HIDDEN

Message received at submit <at> debbugs.gnu.org:

Received: (at submit) by debbugs.gnu.org; 29 Sep 2023 16:53:25 +0000
From debbugs-submit-bounces <at> debbugs.gnu.org Fri Sep 29 12:53:25 2023
Received: from localhost ([127.0.0.1]:57069 helo=debbugs.gnu.org)
	by debbugs.gnu.org with esmtp (Exim 4.84_2)
	(envelope-from <debbugs-submit-bounces <at> debbugs.gnu.org>)
	id 1qmGk0-0007HV-UW
	for submit <at> debbugs.gnu.org; Fri, 29 Sep 2023 12:53:25 -0400
Received: from lists.gnu.org ([2001:470:142::17]:53076)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <ws@HIDDEN>) id 1qmGjx-0007H3-I2
 for submit <at> debbugs.gnu.org; Fri, 29 Sep 2023 12:53:22 -0400
Received: from eggs.gnu.org ([2001:470:142:3::10])
 by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <ws@HIDDEN>) id 1qmGjW-0004Hc-VR
 for bug-guix@HIDDEN; Fri, 29 Sep 2023 12:52:57 -0400
Received: from wolfsden.cz ([37.205.8.62])
 by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <ws@HIDDEN>) id 1qmGjS-0005zr-69
 for bug-guix@HIDDEN; Fri, 29 Sep 2023 12:52:54 -0400
Received: by wolfsden.cz (Postfix, from userid 104)
 id 9E2CF25FF78; Fri, 29 Sep 2023 16:52:44 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=wolfsden.cz; s=mail;
 t=1696006364; bh=oj5b7mUnaSSEcmecccWHc8TpJO3BG/H+9sF0J8SdrXY=;
 h=Date:From:To:Subject;
 b=KQOM5AIw0B1RoyIfKN0EEb8gW4xRu06B2q95muApiXVPo8zSS76/O+WeQb1JDyXbQ
 6w4GBo6vr1hOrJIaWaLv9KYW+lhdQ9pxLH9chE1T9mpftHH4I8qlYhIclfj0kDfaQ3
 eTgjpKlUose7Bzij2TGwLRi4/SR0hOJp9gsKhCpoHbrZJ350H1ZgqosmY7hUEJqIHM
 rABntmuNAeU/tHvK3CGEdYkwsNUe8TyEAWnzw3TR52Qqx6d8BffnpAt1ctiUsLDvlC
 /kK2udLWVFJjzhwwEdqhW4EsTNi2QtIJo4L8tq7RiY0X3kSWCEUbkWuHUFpq8rAl1U
 BUnsjiNP6UxsMpouO5+5h3Yt92E6tjKSHvi59PwvKzcNWScJ+eezQOM9XUEHUH+Xfg
 8SlRSJjqxayImrWyv+FoDyNXrB2uqf8tyYGH0hc35hK/0WxXRGAJvapptiympiz7FZ
 dhqyFrfD8l5i9OrAsvwpM3BHM/1kM/czFU2GYVm8I0Hng5xy/U9TRrxSB4/OErUYMl
 3vOcoNedM9f6wnBchoCav6vp45kZoS4s+zu7E03p0+bXo+hdwxxVhyPLkSp+k5T4F6
 xQpe34yqQsYxS7HlBdlz7JuCWDzVc1e/Fw9WX7Ez4CVxkoSisrdaCS+pYoE+GdJ7kh
 jNripxij7OJHlYfaJyx88Y94=
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on wolfsden
X-Spam-Level: 
X-Spam-Status: No, score=-0.8 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED,
 DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,URIBL_BLOCKED
 autolearn=no autolearn_force=no version=3.4.6
Received: from localhost (unknown [81.17.16.87])
 by wolfsden.cz (Postfix) with ESMTPSA id B2B0625FF77
 for <bug-guix@HIDDEN>; Fri, 29 Sep 2023 16:52:43 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=wolfsden.cz; s=mail;
 t=1696006363; bh=oj5b7mUnaSSEcmecccWHc8TpJO3BG/H+9sF0J8SdrXY=;
 h=Date:From:To:Subject;
 b=AgiAvD/dLnNhjpqRI6h6XCY2F096U5+17Kc4V9havYCnBs9ZkLroRIS8MUx3cheFV
 DDnCEpxmuRrlA4Rvc66tKWMVqfS0qixqzPkw/R28bSw6b3P5Drc/6Dumra6lLOjK44
 CUfaizJiGLryr6xBuuvtmyqeDPzxl9mPwozUEas7ufuC10pv5pfSkUA4ffHXN09i9x
 8givr7fVLRC5lNs1M2W9gwr3h2jZWVY72/PtkFvzlbKo7ds7FB8a6gM69asL+V3pP/
 rzc2IE4/XH2ztoEPvfVBX4loi1Y+4fp2qLi+1XoxNQs6pM7efldS8kGETGoOB4ovEL
 2PLc43FYLU+eZNxIFZAeOqkhHaFnZQHN2eDPJHO1fFyLuzVl2rz+ttutEwOp/cvIIr
 7Gp9DZACEPaw8CKIpttWUxfxq/voDs9O2B2Y1nyOwoGrflvfU/FJ750A8rAEBLrpRW
 aVhVLJPqp++y7XVLQsT45+2L7o/n4X+GGpP68aSKZ0/0RREqzd8jwOIE0XAbzgVmXJ
 ZPpGQ8EaXDmQsGV9djHYGYfNTGzj1DIe3lTs/QyYx1gS/LuceRlZm4HL1RQGB4PMdU
 8zElGYm7teovOIctb5C9RYH2NCToOoixXvy+aK5LiJjmo+XVFLwdqOZDqiQZ+nonQQ
 FSQazIoPKP38UHzf7ddvdb8g=
Received: from localhost (localhost [local])
 by localhost (OpenSMTPD) with ESMTPA id 8b14761c
 for <bug-guix@HIDDEN>; Fri, 29 Sep 2023 16:52:43 +0000 (UTC)
Date: Fri, 29 Sep 2023 18:52:43 +0200
From: wolf <wolf@HIDDEN>
To: bug-guix@HIDDEN
Subject: Guix makes invalid assumptions regarding guile-git guarantees
 leading to guix pull failing
Message-ID: <ZRcA23RUYvBuE1JX@ws>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature"; boundary="pesXC/INFpw/3QMw"
Content-Disposition: inline
Received-SPF: none client-ip=37.205.8.62; envelope-from=ws@HIDDEN;
 helo=wolfsden.cz
X-Spam_score_int: -17
X-Spam_score: -1.8
X-Spam_bar: -
X-Spam_report: (-1.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1,
 DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HEADER_FROM_DIFFERENT_DOMAINS=0.249,
 SPF_HELO_PASS=-0.001, SPF_NONE=0.001,
 UNPARSEABLE_RELAY=0.001 autolearn=no autolearn_force=no
X-Spam_action: no action
X-Spam-Score: 0.2 (/)
X-Debbugs-Envelope-To: submit
X-BeenThere: debbugs-submit <at> debbugs.gnu.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: <debbugs-submit.debbugs.gnu.org>
List-Unsubscribe: <https://debbugs.gnu.org/cgi-bin/mailman/options/debbugs-submit>, 
 <mailto:debbugs-submit-request <at> debbugs.gnu.org?subject=unsubscribe>
List-Archive: <https://debbugs.gnu.org/cgi-bin/mailman/private/debbugs-submit/>
List-Post: <mailto:debbugs-submit <at> debbugs.gnu.org>
List-Help: <mailto:debbugs-submit-request <at> debbugs.gnu.org?subject=help>
List-Subscribe: <https://debbugs.gnu.org/cgi-bin/mailman/listinfo/debbugs-submit>, 
 <mailto:debbugs-submit-request <at> debbugs.gnu.org?subject=subscribe>
Errors-To: debbugs-submit-bounces <at> debbugs.gnu.org
Sender: "Debbugs-submit" <debbugs-submit-bounces <at> debbugs.gnu.org>
X-Spam-Score: -0.8 (/)


--pesXC/INFpw/3QMw
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Table of Contents
_________________

1. Guix makes an invalid assumption regarding guile-git's guarantees
=2E. 1. Root cause analysis
=2E.... 1. Object size
=2E.... 2. Cache space
=2E. 2. Reproduction to verify the analysis
2. Possible solutions
=2E. 1. Do not use `eq?'
=2E. 2. Increase the cache size / limits
=2E. 3. Ensure the id stability in guile-git
3. Questions
=2E. 1. Is this a regression?
4. Attachments
=2E. 1. d51135e8477118dc63a7e5462355cd27e884f4fb
=2E. 2. test.scm


1 Guix makes an invalid assumption regarding guile-git's guarantees
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

  There is an assumption made by Guix regarding guile-git, which is not
  true.  The problem is demonstrated using my fork, since that is where
  I encountered it first, but official Guix will hit the same problem
  sooner or later.  I will also provide an independent repository for
  the verification.

  Guix made a design decision to compare commit objects using eq?, based
  on the assumption that guile-git will return the same object for the
  same commit.  However that assumption is wrong and can lead to fun
  issues like:

  ,----
  | scheme@(guile-user)> (use-modules (git) (guix git))
  | scheme@(guile-user)> (define %repo (repository-open "/tmp/my-fork"))
  | scheme@(guile-user)> (define %hash "d51135e8477118dc63a7e5462355cd27e88=
4f4fb")
  | scheme@(guile-user)> (commit-relation
  |  (commit-lookup %repo (string->oid %hash))
  |  (commit-lookup %repo (string->oid %hash)))
  | $5 =3D unrelated
  `----

  This does break (at least) `guix pull':

  ,----
  | Updating channel 'guix' from Git repository at 'https://git.sr.ht/~gray=
wolf/guix'...
  | guix pull: error: aborting update of channel 'guix' to commit 4dbd25fa0=
e09b40ba2ab01d1e64fa36db652b501, which is not a descendant of d51135e847711=
8dc63a7e5462355cd27e884f4fb
  | hint: This could indicate that the channel has been tampered with and i=
s trying to force a roll-back, preventing you from getting the latest updat=
es.  If you think this is not the case,
  | explicitly allow non-forward updates.
  `----

  The commit actually is a descendant, but it is not found in the
  `commit-closure' due to the `eq?' comparison being used.  The
  verification of the relation between the commits:

  ,----
  | $ git log --first-parent --oneline -4
  | 4dbd25fa0e (HEAD -> master, origin/master) etc/fork-guix: Use absolute =
path for the patch file.
  | 601029b97a Merge updates from the Guix proper
  | afa5eabc93 git-authenticate: Fix tracking of trusted parents.
  | d51135e847 Merge updates from the Guix proper
  `----


1.1 Root cause analysis
~~~~~~~~~~~~~~~~~~~~~~~

  Guile-git is a wrapper around libgit2, and libgit2 (as far as I can
  tell) makes no guarantees about returning the same commit object every
  time lookup is performed for the same hash.  It just does it,
  sometimes, due to the internal use of a cache.

  There are two rules in place.


1.1.1 Object size
-----------------

  Object has to be smaller than configured size limit for the object
  type.  For commits, that is 4096B.  It can be increased
  (`git_libgit2_opts' called with `GIT_OPT_SET_CACHE_OBJECT_LIMIT'), but
  guile-git does not offer any binding for that functionality.

  In the case illustrated above, the commit object for 4.1 has size of
  4139B, and is therefore never cached.  That means that a new object is
  returned every time, and therefore they are never considered equal by
  Guix.  As you can see, the commit message, while long, is sensible and
  useful, so it is fairly easy to hit the limit.


1.1.2 Cache space
-----------------

  Cache is limited to 268435456B, and it is pruned if the need to get
  more space arises.  That is especially problematic because it could
  lead to hard to debug failures based on the access pattern to the
  repository.

  Typical signed commit seems to be larger than 1kB.  Based on the cache
  size, rough calculation suggests that Guix (which requires the commits
  to be signed) can only store somewhere around 252052 commits before
  running into possible issues.  Currently there are 64416 commits since
  the channel introduction, so we are already getting close and it
  *will* be a problem in the future, even if we keep the commits
  smallish.


1.2 Reproduction to verify the analysis
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  To verify that the analysis above is correct, I created a test
  repository to run tests against.  `head' refers to a commit at the
  HEAD, it is a small one (210B libgit2's entry size).  `large' refers
  to a commit with a large (4kB) commit message, it is the parent of
  `head'.  `root' refers to the root commit.  All other commits are of
  210B entry size.  The repository is located here[0].

  When the 4.2 is executed, it seems to support the conclusions reached
  in previous section.  The output when executed on my machine is:

  ,----
  | Checking (eq? (%commit X) (find-c (%hash X))) for X:
  |=20
  | ;;; (large #f)
  |=20
  | ;;; (head #t)
  |=20
  | ;;; (root #t)
  | Collecting all commits...
  | Checking if they match themselves...
  | # of mismatches 300530 of 1578267 commits
  | Relation between 'head and 'large: unrelated
  `----

  We can see that both `head' and `root' do match themselves, since they
  are small and fit into the cache.  However `large' does not fit into
  the cache, and therefore we get new, distinct, object from each
  `commit-lookup' call.  That seems to confirm the hypothesis regarding
  the limit on object size that fits into the cache.

  After that we record all commits into a vhash, overwhelming the cache
  and forcing evictions.  After that we do a second sweep to check how
  many of the commit will match itself.  We can see that about 20% of
  commits do not match, confirming the hypothesis regarding the cache
  capacity.

  And finally we see that `head' and `large' are unrelated, despite that
  not being the case, after all, `head' is the parent of `large'.  Git
  tooling does confirm that:

  ,----
  | $ git merge-base --is-ancestor 9b985229bcd 71f544c102a; echo $?
  | 0
  `----

  0: <https://git.sr.ht/~graywolf/guix-guile-git-repro>


2 Possible solutions
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

2.1 Do not use `eq?'
~~~~~~~~~~~~~~~~~~~~

  The correct solution is to stop using `eq?' to compare the commits
  (and other objects from guile-git, if that is being done).  That will
  come at some performance cost, but the benefit of being actually
  correct does out-weight that.

  My partial solution is based on new record type, `<commit-set>', to
  use instead of (guix sets).

  ,----
  | (define-record-type <commit-set>
  |   (%make-commit-set vhash)
  |   commit-set?
  |   (vhash commit-set-vhash))
  |=20
  | (define (make-commit-set)
  |   (%make-commit-set vlist-null))
  |=20
  | (define (commit-set-contains? commit commit-set)
  |   (->bool (vhash-assoc (oid->string (commit-id commit))
  |                        (commit-set-vhash commit-set))))
  |=20
  | (define (commit-set-insert commit commit-set)
  |   (%make-commit-set (vhash-cons (oid->string (commit-id commit))
  |                                #t
  |                                (commit-set-vhash commit-set))))
  `----

  It is not complete, for example I do not check that the commits do
  come from the same repository.

  However I believe similar approach is the preferred solution.


2.2 Increase the cache size / limits
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  While the size limit for objects to be cache-able is configurable, the
  maximum size of the cache is hard coded.  However patching libgit2 is
  always an option.  But this does not really solve the issue, it just
  pushes it down the line.

  Removing the size limits is also an option, but would lead to unbound
  memory usage.


2.3 Ensure the id stability in guile-git
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  Technically it should be possible to improve guile-git to ensure the
  object stability without patching the libgit2, but it would basically
  involve a second cache, without size limits.  It could probably
  utilize weak hash tables to make the memory usage reasonable.  However
  it would be tricky to make sure everything is correctly wrapped.


3 Questions
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

3.1 Is this a regression?
~~~~~~~~~~~~~~~~~~~~~~~~~

  Since this was the approach chosen by Guix, did guile-git or libgit2
  used to guarantee this?  Should this be reported as a regression?


4 Attachments
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

4.1 d51135e8477118dc63a7e5462355cd27e884f4fb
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  ,----
  | commit d51135e8477118dc63a7e5462355cd27e884f4fb
  | Merge: 75daa689f1 0500af5556
  | Author: Tomas Volf <wolf@HIDDEN>
  | Date:   Wed Sep 27 21:03:40 2023 +0200
  |=20
  |     Merge updates from the Guix proper
  |    =20
  |     * upstream/master:
  |       Revert "build: Add missing guix-gc.timer file to binary tarball."
  |       gnu: tio: Update to 2.7.
  |       gnu: bcachefs-tools: Restore mount.bcachefs shell script version.
  |       gnu: bcachefs-tools: Remove obsolete phase.
  |       gnu: bcachefs-tools: Update to 1.2-0.1e35840.
  |       Revert "gnu: bcachefs-tools: Restyle format."
  |       gnu: sssd: Update to 2.9.2.
  |       gnu: wvkbd: Update to 0.14.1.
  |       gnu: yt-dlp: Update to 2023.09.24.
  |       gnu: lhasa: Update to 0.4.0.
  |       gnu: vcmi: Update to 1.3.2.
  |       gnu: cmark: Update to 0.30.3.
  |       gnu: python-tslearn: Update to 0.6.2.
  |       gnu: python-astropy: Update to 5.3.3.
  |       gnu: python-stdatamodels: Update to 1.8.0.
  |       gnu: python-roman-datamodels: Remove all test constraints.
  |       gnu: python-roman-datamodels: Update to 0.17.1.
  |       gnu: python-rad: Update to 0.17.1.
  |       gnu: python-pyvo: Update to 1.4.2.
  |       gnu: python-photutils: Update to 1.9.0.
  |       gnu: python-jwst: Update to 1.11.4.
  |       gnu: python-fitsio: Update to 1.2.0.
  |       gnu: python-crds: Update to 11.17.4.
  |       gnu: python-bayesicfitting: Update to 3.2.0.
  |       gnu: python-sunpy: Enable more tests.
  |       gnu: python-cdflib: Fix version detection.
  |       gnu: python-cdflib: Update to 1.1.0.
  |       gnu: python-astropy-healpix: Update to 1.0.0.
  |       gnu: splash: Update to 3.8.4.
  |       gnu: libxisf: Extend description.
  |       gnu: libxisf: Update to 0.2.9.
  |       gnu: python-coloful: Update to 0.5.5.
  |       gnu: python-pyotp: Update to 2.9.0.
  |       gnu: gajim: Clean up formatting.
  |       gnu: python-nbxmpp: Clean up formatting.
  |       gnu: gajim-openpgp: Update to 1.5.0.
  |       gnu: gajim-omemo: Update to 2.9.0.
  |       gnu: gajim: Update to 1.7.3.
  |       gnu: python-nbxmpp: Update to 4.2.2.
  |       gnu: transmission: Fix loading icons in pure environments.
  |       gnu: alex4: Remove non-free package.
  |       doc: Update bug-reference configuration snippet.
  |       tests: Assume =E2=80=98git=E2=80=99 is always available.
  |       git-download: Use =E2=80=9Cbuiltin:git-download=E2=80=9D when ava=
ilable.
  |       perform-download: Use the =E2=80=98git=E2=80=99 command captured =
at configure time.
  |       build: Add dependency on Git.
  |       daemon: Add =E2=80=9Cgit-download=E2=80=9D built-in builder.
  |       perform-download: Remove unused one-argument clause.
  |       git-download: Honor the =E2=80=98GUIX_DOWNLOAD_FALLBACK_TEST=E2=
=80=99 environment variable.
  |       git-download: Move fallback code to (guix build git).
  |       tests: Adjust =E2=80=98guix graph --path=E2=80=99 test to latest =
Emacs changes.
  |       gnu: imgui: Update to 1.89.9.
  |       gnu: Add tracy.
  |       gnu: Add tracy-wayland.
  |       gnu: glfw: Patch dlopen calls.
  |       gnu: imgui: Enable freetype support.
  |       gnu: capstone: Update to 5.0.1.
  |       gnu: gtypist: Install the gtypist-mode Emacs major mode.
  |       multiqc: Don't propagate inputs.
  |       gnu: transmission: Restore HTML files in the default output.
  |       gnu: aalib: Really build the shared library on powerpc64le-linux.
  |       gnu: edk2-tools: Update to 202308.
  |       doc: Add new 'Circular Module Dependencies' section.
  |       gnu: embedded: Turn packages using top-level variables into proce=
dures.
  |       gnu: avr: Delay all cross compilation packages.
  |       gnu: Add satdump.
  |       gnu: nng: Update to 1.5.2.
  |       gnu: sdrangel: Update to 7.16.0.
  `----


4.2 test.scm
~~~~~~~~~~~~

  ,----
  | (use-modules (ice-9 vlist)
  |              (git)
  |              (guix git))
  |=20
  | ;;; Tweak the path as necessary.
  | (define %repo (repository-open "/home/wolf/tmp/guix-guile-git-repro"))
  |=20
  | ;;; All hashes that are of interest to us.
  | (define %hashes '((large . "9b985229bcd447261b147c6bf70a86c2a345f234")
  |                   (head . "71f544c102a658ed5f2f2258862f2d59cbe70b8b")
  |                   (root . "db3f74122f4c384897ba7fddac73b893d19c1c67")))
  | (define (%hash for)
  |   "Return a sha1 hash for a specified key."
  |   (assoc-ref %hashes for))
  |=20
  | (define %Xs
  |   (map car %hashes))
  |=20
  | (define (find-c hash)
  |   "Return a commit based on the string sha1 hash."
  |   (commit-lookup %repo (string->oid hash)))
  |=20
  | ;;; Memoize the commits so that we can compare against them later.
  | (define %commits (map (=CE=BB (k) `(,k . ,(find-c (%hash k))))
  |                       %Xs))
  | (define (%commit for)
  |   "Return a memoized commit for a specified key."
  |   (assoc-ref %commits for))
  |=20
  | (display "Checking (eq? (%commit X) (find-c (%hash X))) for X:\n")
  | (for-each (=CE=BB (x)
  |             (pk x (eq? (%commit x) (find-c (%hash x)))))
  |           %Xs)
  |=20
  | (display "Collecting all commits...\n")
  | (define all-commits
  |   (let loop ((commits vlist-null)
  |              (hash (%hash 'head)))
  |     (let* ((c (find-c hash))
  |            (parents (commit-parents c))
  |            (commits (vhash-cons (oid->string (commit-id c))
  |                                 c
  |                                 commits)))
  |       (if (null? parents)
  |           commits
  |           (loop commits
  |                 (oid->string (commit-id (car parents))))))))
  | (display "Checking if they match themselves...\n")
  | (format #t "# of mismatches ~a of ~a commits\n"
  |         (vlist-fold (=CE=BB (x total)
  |                       (let ((hash (car x))
  |                             (commit (cdr x)))
  |                         (if (eq? commit (find-c hash))
  |                             total
  |                             (+ total 1))))
  |                     0
  |                     all-commits)
  |         (vlist-length all-commits))
  |=20
  | (format #t "Relation between 'head and 'large: ~a\n"
  |         (commit-relation (%commit 'head)
  |                          (%commit 'large)))
  `----

Have a nice day,
W.

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

--pesXC/INFpw/3QMw
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAEBCgAdFiEEt4NJs4wUfTYpiGikL7/ufbZ/wakFAmUXANUACgkQL7/ufbZ/
wakODA/8DxkKlgO32+U9sJxD0ZS8QN7Hua1SzsUmVChlPasuFqHgqx0Exu9y8qSv
I7N72W0zam6LEjWtGhzfF8kTamtrNALObqPi6Xw21VrXm4iO2LFQy9tZaGAxQvtb
+I8CCVtAxS/Gk8ehCYGXpGpeafE0/TBzB/b+0Mjqx+KdAq7amWTzLGo8/5O1Qe8l
Zrr+VW2KdYCDRi6qMR5DsxCQin38m8yuYiD/mX7vjTF6w/XHf73U9QqzTNwYwYLi
vgEscW+5VbseEE3ESyyei9S0H2cORfRT2UIFZGwsg6IZE1nHCD+lQjfPdGl8pIo8
3iFWgffkOYdp6zqo1VLPNcEGIDwvN5JdPRE3O+skqzCUhfoT9a3wymDgd/92HDKp
YvxNiGLQfXmXIq3UsCbBIAvSy6wec+rn8twxRDFf8fNlB1Ma8UPUXwV0h3TSFet5
xWlMeTyki/uWcLDSBSaVt5SyRvKw3DZ9XO+G6L91G5KqdWvjn/Ll03OuHezM8K7I
SIhcZylwdEhBCRdZr6JVN/uu005nkhKQ1omgi8hjsZaHGZFVg9o9+r22n5cFPOol
6ocs4DDXf81fMuhY48FB4+/9Ha62xsO/m/tEhFsNEqrCIPe9ciAPjoa5ElPsYaiI
iJEwqduWUGCQ9TXWS1ZyKhIZkG9iO2DU7e+4GGP9BwSjzEQp0iU=
=yCeP
-----END PGP SIGNATURE-----

--pesXC/INFpw/3QMw--