[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

* doc/guix.texi (Invoking guix time-machine): Document limitation.
* guix/scripts/time-machine.scm (%oldest-possible-commit): New variable.
(guix-time-machine): Raise an error when the channel commit is too old.

Suggested-by: Simon Tournier <zimon.toutoune@gmail.com>
---
 doc/guix.texi                 |  6 ++++++
 guix/scripts/time-machine.scm | 23 ++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 1d8ebcd72f..30fef813c0 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -5056,6 +5056,12 @@ Invoking guix time-machine
 large number of packages; the result is cached though and subsequent
 commands targeting the same commit are almost instantaneous.
 
+Due to @command{guix time-machine} relying on the ``inferiors''
+mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
+commit @samp{2ca299caf} (``Add (guix inferior) and (guix scripts
+repl).''), dated July 10@sup{th}, 2018.  An error is returned when
+attempting to navigate to older commits.
+
 @quotation Note
 The history of Guix is immutable and @command{guix time-machine}
 provides the exact same software as they are in a specific Guix
diff --git a/guix/scripts/time-machine.scm b/guix/scripts/time-machine.scm
index d7c71ef705..36a40a1538 100644
--- a/guix/scripts/time-machine.scm
+++ b/guix/scripts/time-machine.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2019 Konrad Hinsen <konrad.hinsen@fastmail.net>
 ;;; Copyright © 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -19,13 +20,15 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (guix scripts time-machine)
+  #:use-module (guix channels)
+  #:use-module (guix diagnostics)
   #:use-module (guix ui)
   #:use-module (guix scripts)
   #:use-module (guix inferior)
   #:use-module (guix store)
   #:use-module (guix status)
   #:use-module ((guix git)
-                #:select (with-git-error-handling))
+                #:select (update-cached-checkout with-git-error-handling))
   #:use-module ((guix utils)
                 #:select (%current-system))
   #:use-module ((guix scripts pull)
@@ -38,9 +41,16 @@ (define-module (guix scripts time-machine)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
+  #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-37)
+  #:use-module (srfi srfi-71)
   #:export (guix-time-machine))
 
+;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
+;;; that can be travelled to.
+(define %oldest-possible-commit
+  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
+
 \f
 ;;;
 ;;; Command-line options.
@@ -139,9 +149,20 @@ (define-command (guix-time-machine . args)
     (with-git-error-handling
      (let* ((opts         (parse-args args))
             (channels     (channel-list opts))
+            (guix-channel (find guix-channel? channels))
             (command-line (assoc-ref opts 'exec))
+            (ref          (assoc-ref opts 'ref))
+            (checkout commit relation (update-cached-checkout
+                                       (channel-url guix-channel)
+                                       #:ref (or ref '())
+                                       #:starting-commit
+                                       %oldest-possible-commit))
             (substitutes?  (assoc-ref opts 'substitutes?))
             (authenticate? (assoc-ref opts 'authenticate-channels?)))
+       (unless (memq relation '(ancestor self))
+         (raise (formatted-message
+                 (G_ "cannot travel past commit `~a' from July 10th, 2018")
+                 (string-take %oldest-possible-commit 12))))
        (when command-line
          (let* ((directory
                  (with-store store
-- 
2.41.0

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> * doc/guix.texi (Invoking guix time-machine): Document limitation.
> * guix/scripts/time-machine.scm (%oldest-possible-commit): New variable.
> (guix-time-machine): Raise an error when the channel commit is too old.
>
> Suggested-by: Simon Tournier <zimon.toutoune@gmail.com>
> ---
>  doc/guix.texi                 |  6 ++++++
>  guix/scripts/time-machine.scm | 23 ++++++++++++++++++++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 1d8ebcd72f..30fef813c0 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -5056,6 +5056,12 @@ Invoking guix time-machine
>  large number of packages; the result is cached though and subsequent
>  commands targeting the same commit are almost instantaneous.
>  
> +Due to @command{guix time-machine} relying on the ``inferiors''
> +mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
> +commit @samp{2ca299caf} (``Add (guix inferior) and (guix scripts
> +repl).''), dated July 10@sup{th}, 2018.  An error is returned when
> +attempting to navigate to older commits.
> +
>  @quotation Note
>  The history of Guix is immutable and @command{guix time-machine}
>  provides the exact same software as they are in a specific Guix
> diff --git a/guix/scripts/time-machine.scm b/guix/scripts/time-machine.scm
> index d7c71ef705..36a40a1538 100644
> --- a/guix/scripts/time-machine.scm
> +++ b/guix/scripts/time-machine.scm
> @@ -2,6 +2,7 @@
>  ;;; Copyright © 2019 Konrad Hinsen <konrad.hinsen@fastmail.net>
>  ;;; Copyright © 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
> +;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -19,13 +20,15 @@
>  ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
>  
>  (define-module (guix scripts time-machine)
> +  #:use-module (guix channels)
> +  #:use-module (guix diagnostics)
>    #:use-module (guix ui)
>    #:use-module (guix scripts)
>    #:use-module (guix inferior)
>    #:use-module (guix store)
>    #:use-module (guix status)
>    #:use-module ((guix git)
> -                #:select (with-git-error-handling))
> +                #:select (update-cached-checkout with-git-error-handling))
>    #:use-module ((guix utils)
>                  #:select (%current-system))
>    #:use-module ((guix scripts pull)
> @@ -38,9 +41,16 @@ (define-module (guix scripts time-machine)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-11)
>    #:use-module (srfi srfi-26)
> +  #:use-module (srfi srfi-34)
>    #:use-module (srfi srfi-37)
> +  #:use-module (srfi srfi-71)
>    #:export (guix-time-machine))
>  
> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
> +;;; that can be travelled to.
> +(define %oldest-possible-commit
> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")

I just tried travelling to that assumed oldest commit (because it
corresponds to the introduction of the inferiors mechanism), but it
fails like:

--8<---------------cut here---------------start------------->8---
Computing Guix derivation for 'x86_64-linux'...  Backtrace:
-           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
In ice-9/eval.scm:
    155:9  4 (_ _)
    159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
   173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
In ./guix/self.scm:
    932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
    903:2  0 (guile-for-build "3.0")

./guix/self.scm:903:2: In procedure guile-for-build:
Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.
Backtrace:
In ice-9/boot-9.scm:
  1752:10 19 (with-exception-handler _ _ #:unwind? _ # _)
In guix/store.scm:
   659:37 18 (thunk)
In guix/status.scm:
    839:4 17 (call-with-status-report _ _)
In guix/store.scm:
   1298:8 16 (call-with-build-handler #<procedure 7ff1daabfd20 at g…> …)
In guix/monads.scm:
    576:2 15 (run-with-store #<store-connection 256.99 7ff1dab842d0> …)
In guix/inferior.scm:
    927:8 14 (_ _)
In guix/channels.scm:
    982:2 13 (_ _)
    924:2 12 (_ _)
In guix/store.scm:
   1883:0 11 (_ _)
   1996:8 10 (_ _)
In guix/channels.scm:
   675:14  9 (_ #<store-connection 256.99 7ff1dab842d0>)
In guix/monads.scm:
    576:2  8 (run-with-store #<store-connection 256.99 7ff1dab842d0> …)
In guix/store.scm:
   1298:8  7 (call-with-build-handler _ _)
   1298:8  6 (call-with-build-handler #<procedure 7ff1db8108b8 at g…> …)
In guix/channels.scm:
   690:14  5 (_)
In guix/monads.scm:
    576:2  4 (run-with-store #<store-connection 256.99 7ff1dab842d0> …)
In ice-9/eval.scm:
   191:27  3 (_ #(#(#<directory (build-self) 7ff1da984aa0> #<pr…>) …))
In ice-9/boot-9.scm:
   2007:7  2 (error _ . _)
  1685:16  1 (raise-exception _ #:continuable? _)
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
invalid build result (#<derivation /gnu/store/rj2g4x23lqyaq16471qm94xp90slxp3h-compute-guix-derivation.drv => /gnu/store/b70mihsj9xx0xxp6izliqb5vm462yifl-compute-guix-derivation 7ff1d8b55000> "")
--8<---------------cut here---------------end--------------->8---

Is this a bug or expected?

-- 
Thanks,
Maxim

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Ludovic Courtès]

Hi!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
>> +;;; that can be travelled to.
>> +(define %oldest-possible-commit
>> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
>
> I just tried travelling to that assumed oldest commit (because it
> corresponds to the introduction of the inferiors mechanism), but it
> fails like:
>
> Computing Guix derivation for 'x86_64-linux'...  Backtrace:
> -           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
> In ice-9/eval.scm:
>     155:9  4 (_ _)
>     159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>    173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
> In ./guix/self.scm:
>     932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
>     903:2  0 (guile-for-build "3.0")
>
> ./guix/self.scm:903:2: In procedure guile-for-build:
> Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.

I would pick ‘v0.15.0’ (= 359fdda40f754bbf1b5dc261e7427b75463b59be) as
the oldest commit one can travel to; it’s a bit newer than the one
above, but it fails in the same way (to my surprise).  It would be
interesting to investigate.

That said, we could just as well pick ‘v1.0.0’, which is the official
warranty-void limit, and which seems to work (it needs to build things,
though…).

Ludo’.

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

Hi Ludo!

Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
>>> +;;; that can be travelled to.
>>> +(define %oldest-possible-commit
>>> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
>>
>> I just tried travelling to that assumed oldest commit (because it
>> corresponds to the introduction of the inferiors mechanism), but it
>> fails like:
>>
>> Computing Guix derivation for 'x86_64-linux'...  Backtrace:
>> -           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
>> In ice-9/eval.scm:
>>     155:9  4 (_ _)
>>     159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>>    173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>> In ./guix/self.scm:
>>     932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
>>     903:2  0 (guile-for-build "3.0")
>>
>> ./guix/self.scm:903:2: In procedure guile-for-build:
>> Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.
>
> I would pick ‘v0.15.0’ (= 359fdda40f754bbf1b5dc261e7427b75463b59be) as
> the oldest commit one can travel to; it’s a bit newer than the one
> above, but it fails in the same way (to my surprise).  It would be
> interesting to investigate.
>
> That said, we could just as well pick ‘v1.0.0’, which is the official
> warranty-void limit, and which seems to work (it needs to build things,
> though…).

I tried building v1.0.0 but it failed the same as earlier attempts on
Python 2, which has a SSL test failing due to now-expired certificates:

--8<---------------cut here---------------start------------->8---
@ build-log 18017 117                                                                                                                                                                           
/gnu/store/gfprsx2m62cvqbh7ysc9ay9slhijvmal-module-import/guix/build/gnu-build-system.scm:369:6: In procedso                                                                                                        ure check:                                                                                                                                          
@ build-log 18017 167                                                                                                                                                                           
Throw to key `srfi-34' with args `(#<condition &invoke-error [program: "make" arguments: ("test" "-j" "24"                                                                                                          ) exit-status: 2 term-signal: #f stop-signal: #f] 9a4900>)'.                                              40                                                                                                        
\@ build-log 18017 101                                                                                                                                                                              
builder for `/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed with exit code 1
@ build-log 18017 183                                                                                     -o
@ build-failed /gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv - 1 builder for `/gnu/store/h-sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed with exit code 1
@ build-log 18017 189
derivation '/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' offloaded to '10.0.0.7' failede-: build of `/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed                        ar
@ build-failed /gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv - 1 builder for `/gnu/store/cesp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed with exit code 100
Backtrace:
In ./guix/gexp.scm:
   573:13 19 (_ _)                                                                                          
In ./guix/store.scm:
   1667:8 18 (_ _)
   1667:8 17 (_ _)                                                                                        so
In ./guix/gexp.scm:
    708:2 16 (_ _)
In ./guix/monads.scm:
    482:9 15 (_ _)
In ./guix/gexp.scm:
   573:13 14 (_ _)
In ./guix/store.scm:
   1667:8 13 (_ _)
In ./guix/gexp.scm:
    708:2 12 (_ _)
In ./guix/monads.scm:                                                                                     9}
    482:9 11 (_ _)
In ./guix/gexp.scm:
   573:13 10 (_ _)
In ./guix/store.scm:                                                                                      37
   1667:8  9 (_ _)                                                                                        36
In ./guix/gexp.scm:                                                                                       1c
    708:2  8 (_ _)                                                                                        2!
In ./guix/monads.scm:                                                                                     3I
    482:9  7 (_ _)                                                                                        \1
|
In ./guix/gexp.scm:
   573:13  6 (_ _)
In ./guix/store.scm:
   1667:8  5 (_ _)
  1690:38  4 (_ #<store-connection 256.99 7f6b38e620c0>)
In ./guix/packages.scm:
   936:16  3 (cache! #<weak-table 406/883> #<package guile-gcrypt@0?> ?)
  1255:22  2 (thunk)
  1188:25  1 (bag->derivation #<store-connection 256.99 7f6b38e620c0> ?)
In srfi/srfi-1.scm:
   592:17  0 (map1 (("source" #<origin #<<git-reference> url: "?>) ?))

srfi/srfi-1.scm:592:17: In procedure map1:
Throw to key `srfi-34' with args `(#<condition &store-protocol-error [message: "build of `/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed" status: 100] 7f6b3a5b2c60>)'.
guix time-machine: erreur : You found a bug: the program '/gnu/store/d12x45lz2dv3mgp594kzjv0d121g0ncs-compute-guix-derivation'
failed to compute the derivation for Guix (version: "6298c3ffd9654d3231a6f25390b056483e8f407c"; system: "xso86_64-linux";
host version: "985638aea14720e16ed5fd94a0e1382a57dec7ac"; pull-version: 1).
Please report it by email to <bug-guix@gnu.org>.
--8<---------------cut here---------------end--------------->8---

Since we can't retroactively fix this kind of problem, it means we
should find the oldest commit which is immune to that problem?

-- 
Thanks,
Maxim

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Ludovic Courtès]

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Since we can't retroactively fix this kind of problem, it means we
> should find the oldest commit which is immune to that problem?

Timebombs (typically expired SSL certificates as in the Python case you
mention) can be worked around.  Currently it’s inconvenient at best
because you need to manually set up a VM or physical machine with its
date set to an older date, but it’s feasible.

For other problems, there’s (guix quirks), which can, to some extent,
let us retroactively fix problems, though that should be rare.

The <https://ci.guix.gnu.org/jobset/time-travel> job set, which uses
‘etc/time-travel-manifest.scm’, is supposed to test these things, but
apparently it’s been stuck for a while.  Needs investigation!

Ludo’.

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Josselin Poiret via Guix-patches via]

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

Hi everyone,

Ludovic Courtès <ludo@gnu.org> writes:

> For other problems, there’s (guix quirks), which can, to some extent,
> let us retroactively fix problems, though that should be rare.

Is there any established policy around what (guix quirks) is allowed to
do?  To be more specific, can/will it ever change derivation hashes?

Best,
-- 
Josselin Poiret

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

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Ludovic Courtès]

Hi,

Josselin Poiret <dev@jpoiret.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> For other problems, there’s (guix quirks), which can, to some extent,
>> let us retroactively fix problems, though that should be rare.
>
> Is there any established policy around what (guix quirks) is allowed to
> do?  To be more specific, can/will it ever change derivation hashes?

It should not change derivations, which means it can only touch
auxiliary files like ‘build-aux/build-self.scm’.

Ludo’.

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

Hello,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
>>> +;;; that can be travelled to.
>>> +(define %oldest-possible-commit
>>> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
>>
>> I just tried travelling to that assumed oldest commit (because it
>> corresponds to the introduction of the inferiors mechanism), but it
>> fails like:
>>
>> Computing Guix derivation for 'x86_64-linux'...  Backtrace:
>> -           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
>> In ice-9/eval.scm:
>>     155:9  4 (_ _)
>>     159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>>    173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>> In ./guix/self.scm:
>>     932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
>>     903:2  0 (guile-for-build "3.0")
>>
>> ./guix/self.scm:903:2: In procedure guile-for-build:
>> Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.
>
> I would pick ‘v0.15.0’ (= 359fdda40f754bbf1b5dc261e7427b75463b59be) as
> the oldest commit one can travel to; it’s a bit newer than the one
> above, but it fails in the same way (to my surprise).  It would be
> interesting to investigate.
>
> That said, we could just as well pick ‘v1.0.0’, which is the official
> warranty-void limit, and which seems to work (it needs to build things,
> though…).

I've picked v1.0.0.  It already seems hard enough to get there (you'd
have to build Python 2 with the hardware clock set to a value in the
past to avoid time bombs in its test suite).

As discussed with Ludovic, it's best to avoid doing work on critical
path, that is, when there is a cache hit for the channel.  I've
implemented their idea to delay such work to within
'cached-channel-instance', where we know if there's a cache hit or not.
Here's a simple 'strace -c' benchmark:

Without this change (Guix commit
985638aea14720e16ed5fd94a0e1382a57dec7ac), on a warm cache, it results
on something like:

--8<---------------cut here---------------start------------->8---
$ strace -c guix time-machine --commit=v1.3.0 -- \
  environment --ad-hoc hello -- hello
% time     seconds  usecs/call     calls    errors syscall
[...]
100,00    0,072028           4     16963      2026 total

With this change (v2 incoming) installed:
--8<---------------cut here---------------start------------->8---
$ strace -c ./pre-inst-env guix time-machine --commit=v1.3.0 -- \
  environment --ad-hoc hello -- hello
% time     seconds  usecs/call     calls    errors syscall
[...]
100,00    0,074576           4     18005      2700 total
--8<---------------cut here---------------end--------------->8---

It seems a small cost to pay for the increased user friendliness.

What do you think?

-- 
Thanks,
Maxim

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Ludovic Courtès]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> @@ -139,9 +149,20 @@ (define-command (guix-time-machine . args)
>      (with-git-error-handling
>       (let* ((opts         (parse-args args))
>              (channels     (channel-list opts))
> +            (guix-channel (find guix-channel? channels))
>              (command-line (assoc-ref opts 'exec))
> +            (ref          (assoc-ref opts 'ref))
> +            (checkout commit relation (update-cached-checkout
> +                                       (channel-url guix-channel)
> +                                       #:ref (or ref '())
> +                                       #:starting-commit
> +                                       %oldest-possible-commit))
>              (substitutes?  (assoc-ref opts 'substitutes?))
>              (authenticate? (assoc-ref opts 'authenticate-channels?)))

Following your question earlier today on IRC, I realized that this would
unconditionally add a Git checkout update every time ‘time-machine’ is
started.

This would noticeably degrade performance in the cache-hit case (when
running a cached channel set).  Currently, on cache hits,
‘cached-channel-instance’ directly returns ~/.cache/guix/profiles/xyz
without performing any Git or (guix store) operation.

This is important because it guarantees that one can run ‘guix
time-machine -C channels.scm -- COMMAND’ at pretty much the same speed
as ‘guix COMMAND’.

Perhaps the solution would be for ‘cached-channel-instance’ to have a
new #:validate-channel procedure (or similar) that would be passed
channel/commit/relation and would have the opportunity to validate,
before the thing is in cache.

How does that sound?

Ludo’.

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Simon Tournier]

Hi,

Thanks Maxim for this improvement.


On Tue, 15 Aug 2023 at 18:14, Ludovic Courtès <ludo@gnu.org> wrote:

> Following your question earlier today on IRC, I realized that this would
> unconditionally add a Git checkout update every time ‘time-machine’ is
> started.

Please note that if git.savannah.gnu.org is not reachable, then “guix
time-machine” fails.

Let start with the regular:

--8<---------------cut here---------------start------------->8---
$ guix describe
Generation 26   Jul 12 2023 09:13:39    (current)
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    branch: master
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330

$ guix time-machine --commit=4a027d2 -- describe
Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
Computing Guix derivation for 'x86_64-linux'... |
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
The following derivations will be built:
[...]
building profile with 1 package...
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
--8<---------------cut here---------------end--------------->8---

So far, so good.  Here all is cached and so on.  Now, let make
git.savannah.gnu.org unreachable by tweaking some stuff.  Then,

--8<---------------cut here---------------start------------->8---
$ guix time-machine --commit=4a027d2 -- describe
guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
--8<---------------cut here---------------end--------------->8---

Well, even if this patch will add another inefficiency, there is already
one. :-) I would say without having looking closely to the code that the
issue comes because something™ tries to connect before checking if the
commit is already in the local checkout.


> This would noticeably degrade performance in the cache-hit case (when
> running a cached channel set).  Currently, on cache hits,
> ‘cached-channel-instance’ directly returns ~/.cache/guix/profiles/xyz
> without performing any Git or (guix store) operation.

As shown above, I guess there is bug…


> This is important because it guarantees that one can run ‘guix
> time-machine -C channels.scm -- COMMAND’ at pretty much the same speed
> as ‘guix COMMAND’.

Yes but I guess it could also be improved as shown above. :-)

The check if the requested commit is newer than the
%oldest-possible-commit should use the Git history graph closure
similarly as the authentication mechanism, no?

And this check should come after checking the cache, no?

Cheers,
simon

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi,
>
> Thanks Maxim for this improvement.
>
>
> On Tue, 15 Aug 2023 at 18:14, Ludovic Courtès <ludo@gnu.org> wrote:
>
>> Following your question earlier today on IRC, I realized that this would
>> unconditionally add a Git checkout update every time ‘time-machine’ is
>> started.
>
> Please note that if git.savannah.gnu.org is not reachable, then “guix
> time-machine” fails.
>
> Let start with the regular:
>
> $ guix describe
> Generation 26   Jul 12 2023 09:13:39    (current)
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     branch: master
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
> $ guix time-machine --commit=4a027d2 -- describe
> Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
> Computing Guix derivation for 'x86_64-linux'... |
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> The following derivations will be built:
> [...]
> building profile with 1 package...
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
>
> So far, so good.  Here all is cached and so on.  Now, let make
> git.savannah.gnu.org unreachable by tweaking some stuff.  Then,
>
> $ guix time-machine --commit=4a027d2 -- describe
> guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
>

[...]

> The check if the requested commit is newer than the
> %oldest-possible-commit should use the Git history graph closure
> similarly as the authentication mechanism, no?
>
> And this check should come after checking the cache, no?

Interesting finding!  I think it'd make sense to raise this issue
separately and discuss its resolution there, too keep things focused and
discoverable :-).

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi,

As discussed in patch#64746, here the fix. :-)

-------------------- Start of forwarded message --------------------
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Wed, 16 Aug 2023 14:41:55 -0400

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Please note that if git.savannah.gnu.org is not reachable, then “guix
> time-machine” fails.
>
> Let start with the regular:
>
> $ guix describe
> Generation 26   Jul 12 2023 09:13:39    (current)
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     branch: master
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
> $ guix time-machine --commit=4a027d2 -- describe
> Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
> Computing Guix derivation for 'x86_64-linux'... |
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> The following derivations will be built:
> [...]
> building profile with 1 package...
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
>
> So far, so good.  Here all is cached and so on.  Now, let make
> git.savannah.gnu.org unreachable by tweaking some stuff.  Then,
>
> $ guix time-machine --commit=4a027d2 -- describe
> guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known

Interesting finding!  I think it'd make sense to raise this issue
separately and discuss its resolution there, too keep things focused and
discoverable :-).
-------------------- End of forwarded message --------------------

The issue is introduced by commit
dce2cf311bc12dee4560329f53ccb53470d5793e in the procedure
reference-available?.  The variable ’ref’ is the pair

    (tag-or-commit . "123abc")

and fails with commit-id? in

  (match ref
    ((or ('commit . commit)
         ('tag-or-commit . (? commit-id? commit)))

Therefore, reference-available? returns #f and the ’when’ branch is run
in update-cached-checkout.

     ;; Only fetch remote if it has not been cloned just before.
     (when (and cache-exists?
                (not (reference-available? repository ref)))
       (remote-fetch (remote-lookup repository "origin")
                     #:fetch-options (make-default-fetch-options)))

Hence the network access required by remote-fetch.

Well, the heavy work to know if the reference is available or not in the
local checkout is done by ’resolve-reference’ in (guix git) doing all
the cases, and especially dealing with tag-or-commit:

      (match ref
        (('branch . branch)
         (let ((oid (reference-target
                     (branch-lookup repository branch BRANCH-REMOTE))))
           (object-lookup repository oid)))
        (('commit . commit)
         (let ((len (string-length commit)))
           ;; 'object-lookup-prefix' appeared in Guile-Git in Mar. 2018, so we
           ;; can't be sure it's available.  Furthermore, 'string->oid' used to
           ;; read out-of-bounds when passed a string shorter than 40 chars,
           ;; which is why we delay calls to it below.
           (if (< len 40)
               (if (module-defined? (resolve-interface '(git object))
                                    'object-lookup-prefix)
                   (object-lookup-prefix repository (string->oid commit) len)
                   (raise (condition
                           (&message
                            (message "long Git object ID is required")))))
               (object-lookup repository (string->oid commit)))))
        (('tag-or-commit . str)
         (if (or (> (string-length str) 40)
                 (not (string-every char-set:hex-digit str)))
             (resolve `(tag . ,str))              ;definitely a tag
             (catch 'git-error
               (lambda ()
                 (resolve `(tag . ,str)))
               (lambda _
                 ;; There's no such tag, so it must be a commit ID.
                 (resolve `(commit . ,str))))))
        (('tag    . tag)
         (let ((oid (reference-name->oid repository
                                         (string-append "refs/tags/" tag))))
           (object-lookup repository oid))))

Instead of duplicating, I propose to reuse it.  See the trivial first
patch.  I think it fixes the annoyance.

Aside, please note that (guix channels) provide commit-or-tag.  It
change nothing but I would find more consistent to have the same
nomenclature.

--8<---------------cut here---------------start------------->8---
(define (sexp->channel-news-entry entry)
  "Return the <channel-news-entry> record corresponding to ENTRY, an sexp."
  (define (pair language message)
    (cons (symbol->string language) message))

  (match entry
    (('entry ((and (or 'commit 'tag) type) commit-or-tag)
             ('title ((? symbol? title-tags) (? string? titles)) ...)
             ('body ((? symbol? body-tags) (? string? bodies)) ...)
             _ ...)
     (channel-news-entry (and (eq? type 'commit) commit-or-tag)
                         (and (eq? type 'tag) commit-or-tag)
--8<---------------cut here---------------end--------------->8---

WDYT about tag-or-commit everywhere?


Last, as I pointed in a naive comment [1], I do not think that
guix/scripts/pull.scm or guix/time-machine.scm need to support both the
pair (commit . x) and (tag-or-commit . x) because the value ’ref’ is set
by the option.  Hence the second patch.

1: https://yhetil.org/guix/87o7j7f2tb.fsf@gmail.com


Let me know if I am not missing something.


Cheers,
simon

[bug#65352] [PATCH 1/2] guix: git: Fix the procedure reference-available?.

[Simon Tournier]

* guix/git/scm (reference-available?): Rely of the procedure resolve-reference
to determine if the reference belongs to the local Git checkout.
---
 guix/git.scm | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/guix/git.scm b/guix/git.scm
index dbc3b7caa7..ebe2600209 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp)
 (define (reference-available? repository ref)
   "Return true if REF, a reference such as '(commit . \"cabba9e\"), is
 definitely available in REPOSITORY, false otherwise."
-  (match ref
-    ((or ('commit . commit)
-         ('tag-or-commit . (? commit-id? commit)))
-     (let ((len (string-length commit))
-           (oid (string->oid commit)))
-       (false-if-git-not-found
-        (->bool (if (< len 40)
-                    (object-lookup-prefix repository oid len OBJ-COMMIT)
-                    (commit-lookup repository oid))))))
-    (_
-     #f)))
+  (false-if-git-not-found
+   (->bool (resolve-reference repository ref))))
 
 (define (clone-from-swh url tag-or-commit output)
   "Attempt to clone TAG-OR-COMMIT (a string), which originates from URL, using

base-commit: 1b2d43fe016848ea2ec16ff18cbc14340944fc4e
-- 
2.38.1

[bug#65352] [PATCH 2/2] scripts: pull: Remove unused reference pair.

[Simon Tournier]

* guix/scripts/pull.scm (channel-list): Remove commit pair reference
specification.
---
 guix/scripts/pull.scm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
index 9b78d4b5ca..616926ee0b 100644
--- a/guix/scripts/pull.scm
+++ b/guix/scripts/pull.scm
@@ -774,8 +774,7 @@ (define (channel-list opts)
                (if (guix-channel? c)
                    (let ((url (or url (channel-url c))))
                      (match ref
-                       ((or ('commit . commit)
-                            ('tag-or-commit . commit))
+                       (('tag-or-commit . commit)
                         (channel (inherit c)
                                  (url url) (commit commit) (branch #f)))
                        (('branch . branch)
-- 
2.38.1

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> * guix/scripts/pull.scm (channel-list): Remove commit pair reference
> specification.
> ---
>  guix/scripts/pull.scm | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
> index 9b78d4b5ca..616926ee0b 100644
> --- a/guix/scripts/pull.scm
> +++ b/guix/scripts/pull.scm
> @@ -774,8 +774,7 @@ (define (channel-list opts)
>                 (if (guix-channel? c)
>                     (let ((url (or url (channel-url c))))
>                       (match ref
> -                       ((or ('commit . commit)
> -                            ('tag-or-commit . commit))
> +                       (('tag-or-commit . commit)
>                          (channel (inherit c)
>                                   (url url) (commit commit) (branch #f)))
>                         (('branch . branch)

Not that channel-list is a public API, so this is effectively changing
the contract, no?

Otherwise, the series LGTM, thank you!

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi Maxim,

On Thu, 17 Aug 2023 at 17:42, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> >                       (match ref
> > -                       ((or ('commit . commit)
> > -                            ('tag-or-commit . commit))
> > +                       (('tag-or-commit . commit)

> Not that channel-list is a public API, so this is effectively changing
> the contract, no?

Well, the contract is not clearly defined. ;-)

The REF is defined by the docstring of update-cached-checkout,

  REF is pair whose key is [branch | commit | tag | tag-or-commit ] and value
  the associated data: [<branch name> | <sha1> | <tag name> | <string>].
  If REF is the empty list, the remote HEAD is used.

Therefore, if we want to be compliant with the public API, we also
need to add 'tag' to the 'or' match case; as I suggested when
commenting your patch tweaking this part. :-)

Well, from my point of view, the alternative is:

 a)
                     (match ref
                       (('tag-or-commit . commit)
                        (channel (inherit c)
                                 (url url) (commit commit) (branch #f)))
                       (('branch . branch)
                        (channel (inherit c)
                                 (url url) (commit #f) (branch branch)))
                       (#f
                        (channel (inherit c) (url url))))

or b)
                     (match ref
                       ((or ('commit . commit)
                            ('tag-or-commit . commit)
                            ('tag . commit))
                        (channel (inherit c)
                                 (url url) (commit commit) (branch #f)))
                       (('branch . branch)
                        (channel (inherit c)
                                 (url url) (commit #f) (branch branch)))
                       (#f
                        (channel (inherit c) (url url)))))

but not ecab937897385fce3e3ce0c5f128afba4304187c. :-)

Cheers,
simon

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Thu, 17 Aug 2023 at 17:42, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> >                       (match ref
>> > -                       ((or ('commit . commit)
>> > -                            ('tag-or-commit . commit))
>> > +                       (('tag-or-commit . commit)
>
>> Not that channel-list is a public API, so this is effectively changing
>> the contract, no?
>
> Well, the contract is not clearly defined. ;-)
>
> The REF is defined by the docstring of update-cached-checkout,
>
>   REF is pair whose key is [branch | commit | tag | tag-or-commit ] and value
>   the associated data: [<branch name> | <sha1> | <tag name> | <string>].
>   If REF is the empty list, the remote HEAD is used.

Good catch, it seems tag is not covered.

> Therefore, if we want to be compliant with the public API, we also
> need to add 'tag' to the 'or' match case; as I suggested when
> commenting your patch tweaking this part. :-)
>
> Well, from my point of view, the alternative is:
>
>  a)
>                      (match ref
>                        (('tag-or-commit . commit)
>                         (channel (inherit c)
>                                  (url url) (commit commit) (branch #f)))
>                        (('branch . branch)
>                         (channel (inherit c)
>                                  (url url) (commit #f) (branch branch)))
>                        (#f
>                         (channel (inherit c) (url url))))
>
> or b)
>                      (match ref
>                        ((or ('commit . commit)
>                             ('tag-or-commit . commit)
>                             ('tag . commit))
>                         (channel (inherit c)
>                                  (url url) (commit commit) (branch #f)))
>                        (('branch . branch)
>                         (channel (inherit c)
>                                  (url url) (commit #f) (branch branch)))
>                        (#f
>                         (channel (inherit c) (url url)))))
>
> but not ecab937897385fce3e3ce0c5f128afba4304187c. :-)

I was driven by my use case where adding support for tag-or-commit was
enough, but I think it'd be a good idea to cover all the potential ref
types documented in update-cached-checkout, so b) makes sense to me.

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi Maxim,

On Tue, 22 Aug 2023 at 22:56, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>> or b)
>>                      (match ref
>>                        ((or ('commit . commit)
>>                             ('tag-or-commit . commit)
>>                             ('tag . commit))
>>                         (channel (inherit c)
>>                                  (url url) (commit commit) (branch #f)))
>>                        (('branch . branch)
>>                         (channel (inherit c)
>>                                  (url url) (commit #f) (branch branch)))
>>                        (#f
>>                         (channel (inherit c) (url url)))))
>>
>> but not ecab937897385fce3e3ce0c5f128afba4304187c. :-)
>
> I was driven by my use case where adding support for tag-or-commit was
> enough, but I think it'd be a good idea to cover all the potential ref
> types documented in update-cached-checkout, so b) makes sense to me.

Ok, b) is fine with me.

Sorry for not being clear in #64746 but this consistency was the subject
of my comment [1]. :-)

Cheers,
simon


1: https://issues.guix.gnu.org/64746#13

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Tue, 22 Aug 2023 at 22:56, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>>> or b)
>>>                      (match ref
>>>                        ((or ('commit . commit)
>>>                             ('tag-or-commit . commit)
>>>                             ('tag . commit))
>>>                         (channel (inherit c)
>>>                                  (url url) (commit commit) (branch #f)))
>>>                        (('branch . branch)
>>>                         (channel (inherit c)
>>>                                  (url url) (commit #f) (branch branch)))
>>>                        (#f
>>>                         (channel (inherit c) (url url)))))
>>>
>>> but not ecab937897385fce3e3ce0c5f128afba4304187c. :-)
>>
>> I was driven by my use case where adding support for tag-or-commit was
>> enough, but I think it'd be a good idea to cover all the potential ref
>> types documented in update-cached-checkout, so b) makes sense to me.
>
> Ok, b) is fine with me.
>
> Sorry for not being clear in #64746 but this consistency was the subject
> of my comment [1]. :-)

I'm glad we finally came to a common understanding, ah!

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Simon Tournier <zimon.toutoune@gmail.com> writes:
>
>> * guix/scripts/pull.scm (channel-list): Remove commit pair reference
>> specification.
>> ---
>>  guix/scripts/pull.scm | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
>> index 9b78d4b5ca..616926ee0b 100644
>> --- a/guix/scripts/pull.scm
>> +++ b/guix/scripts/pull.scm
>> @@ -774,8 +774,7 @@ (define (channel-list opts)
>>                 (if (guix-channel? c)
>>                     (let ((url (or url (channel-url c))))
>>                       (match ref
>> -                       ((or ('commit . commit)
>> -                            ('tag-or-commit . commit))
>> +                       (('tag-or-commit . commit)
>>                          (channel (inherit c)
>>                                   (url url) (commit commit) (branch #f)))
>>                         (('branch . branch)
>
> Not that channel-list is a public API, so this is effectively changing
> the contract, no?

Yes, but it’s really meant to be used internally, where it’s either
'tag-or-commit or 'branch in practice.  So to me either way is fine.

Thanks,
Ludo’.

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Simon Tournier <zimon.toutoune@gmail.com> writes:
>>
>>> * guix/scripts/pull.scm (channel-list): Remove commit pair reference
>>> specification.
>>> ---
>>>  guix/scripts/pull.scm | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
>>> index 9b78d4b5ca..616926ee0b 100644
>>> --- a/guix/scripts/pull.scm
>>> +++ b/guix/scripts/pull.scm
>>> @@ -774,8 +774,7 @@ (define (channel-list opts)
>>>                 (if (guix-channel? c)
>>>                     (let ((url (or url (channel-url c))))
>>>                       (match ref
>>> -                       ((or ('commit . commit)
>>> -                            ('tag-or-commit . commit))
>>> +                       (('tag-or-commit . commit)
>>>                          (channel (inherit c)
>>>                                   (url url) (commit commit) (branch #f)))
>>>                         (('branch . branch)
>>
>> Not that channel-list is a public API, so this is effectively changing
>> the contract, no?
>
> Yes, but it’s really meant to be used internally, where it’s either
> 'tag-or-commit or 'branch in practice.  So to me either way is fine.

In this case, should we stop exporting it from the module?  (and use it
via the (@ (...)) trick as needed).  This would communicate the
intention best.

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>>> Not that channel-list is a public API, so this is effectively changing
>>> the contract, no?
>>
>> Yes, but it’s really meant to be used internally, where it’s either
>> 'tag-or-commit or 'branch in practice.  So to me either way is fine.
>
> In this case, should we stop exporting it from the module?  (and use it
> via the (@ (...)) trick as needed).  This would communicate the
> intention best.

Well, there are different levels of “internal” I guess.  :-)

@@ (double-at) should only be used as a last resort; whether it’s usable
at all depends on inlining decisions made by the compiler.  So in this
case, I’m for plain #:export.

Thanks,
Ludo’.

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>>> Not that channel-list is a public API, so this is effectively changing
>>>> the contract, no?
>>>
>>> Yes, but it’s really meant to be used internally, where it’s either
>>> 'tag-or-commit or 'branch in practice.  So to me either way is fine.
>>
>> In this case, should we stop exporting it from the module?  (and use it
>> via the (@ (...)) trick as needed).  This would communicate the
>> intention best.
>
> Well, there are different levels of “internal” I guess.  :-)
>
> @@ (double-at) should only be used as a last resort; whether it’s usable
> at all depends on inlining decisions made by the compiler.  So in this
> case, I’m for plain #:export.

OK!  Yes, whatever suites the bill best :-).

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Hi!

Simon Tournier <zimon.toutoune@gmail.com> skribis:

> * guix/git/scm (reference-available?): Rely of the procedure resolve-reference
> to determine if the reference belongs to the local Git checkout.

LGTM too, thanks!

Ludo’.

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

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

Hi,

Simon Tournier <zimon.toutoune@gmail.com> skribis:

> * guix/git/scm (reference-available?): Rely of the procedure resolve-reference
> to determine if the reference belongs to the local Git checkout.
> ---
>  guix/git.scm | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/guix/git.scm b/guix/git.scm
> index dbc3b7caa7..ebe2600209 100644
> --- a/guix/git.scm
> +++ b/guix/git.scm
> @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp)
>  (define (reference-available? repository ref)
>    "Return true if REF, a reference such as '(commit . \"cabba9e\"), is
>  definitely available in REPOSITORY, false otherwise."
> -  (match ref
> -    ((or ('commit . commit)
> -         ('tag-or-commit . (? commit-id? commit)))
> -     (let ((len (string-length commit))
> -           (oid (string->oid commit)))
> -       (false-if-git-not-found
> -        (->bool (if (< len 40)
> -                    (object-lookup-prefix repository oid len OBJ-COMMIT)
> -                    (commit-lookup repository oid))))))
> -    (_
> -     #f)))
> +  (false-if-git-not-found
> +   (->bool (resolve-reference repository ref))))

Houston, we have a problem:

--8<---------------cut here---------------start------------->8---
$ guix time-machine -C <(echo %default-channels) -- describe
Backtrace:
          17 (primitive-load "/home/ludo/.config/guix/current/bin/gu…")
In guix/ui.scm:
   2323:7 16 (run-guix . _)
  2286:10 15 (run-guix-command _ . _)
In ice-9/boot-9.scm:
  1752:10 14 (with-exception-handler _ _ #:unwind? _ # _)
  1747:15 13 (with-exception-handler #<procedure 7f987de73fc0 at ic…> …)
In guix/store.scm:
    672:3 12 (_)
In ice-9/boot-9.scm:
  1752:10 11 (with-exception-handler _ _ #:unwind? _ # _)
In guix/store.scm:
   659:37 10 (thunk)
In guix/status.scm:
    839:4  9 (call-with-status-report _ _)
In guix/store.scm:
   1298:8  8 (call-with-build-handler #<procedure 7f987de84420 at g…> …)
In guix/inferior.scm:
   932:10  7 (cached-channel-instance #<store-connection 256.99 7f9…> …)
In guix/scripts/time-machine.scm:
   171:42  6 (validate-guix-channel _)
In guix/git.scm:
   471:21  5 (update-cached-checkout _ #:ref _ #:recursive? _ # _ # _ …)
In ice-9/boot-9.scm:
  1747:15  4 (with-exception-handler #<procedure 7f987de900c0 at ic…> …)
In guix/git.scm:
   364:11  3 (_)
    235:4  2 (resolve _)
In ice-9/boot-9.scm:
  1685:16  1 (raise-exception _ #:continuable? _)
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
Throw to key `match-error' with args `("match" "no matching pattern" ())'.
$ guix describe
Generation 272  Sep 03 2023 23:46:47    (current)
  guix e365c26
    repository URL: https://git.savannah.gnu.org/git/guix.git
    branch: master
    commit: e365c26a34fa485f9af46538fcea128db681c33d
--8<---------------cut here---------------end--------------->8---

I’m testing the fix below:


[-- Attachment #2: Type: text/x-patch, Size: 1074 bytes --]

diff --git a/guix/git.scm b/guix/git.scm
index ebe2600209..5fa604f9a0 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -1,6 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017, 2020 Mathieu Othacehe <m.othacehe@gmail.com>
-;;; Copyright © 2018-2022 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2018-2023 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2021 Kyle Meyer <kyle@kyleam.com>
 ;;; Copyright © 2021 Marius Bakke <marius@gnu.org>
 ;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
@@ -282,7 +282,10 @@ (define (resolve-reference repository ref)
          (if (= OBJ-TAG (object-type obj))
              (object-lookup repository
                             (tag-target-id (tag-lookup repository oid)))
-             obj))))))
+             obj)))
+      (()
+       (resolve-reference repository
+                          '(symref . "refs/remotes/origin/HEAD"))))))
 
 (define (switch-to-ref repository ref)
   "Switch to REPOSITORY's branch, commit or tag specified by REF.  Return the

[-- Attachment #3: Type: text/plain, Size: 12 bytes --]


Ludo’.

[bug#65352] Fix time-machine and network

[Simon Tournier]

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

Hi Ludo,

On Mon, 04 Sep 2023 at 10:49, Ludovic Courtès <ludo@gnu.org> wrote:

> Houston, we have a problem:

This is Houston. Say again, please. :-)


> diff --git a/guix/git.scm b/guix/git.scm
> index ebe2600209..5fa604f9a0 100644
> --- a/guix/git.scm
> +++ b/guix/git.scm

> +      (()
> +       (resolve-reference repository
> +                          '(symref . "refs/remotes/origin/HEAD"))))))

The fix is to simple return #false when the reference is not resolved.

Well, let me now if the attached patch fixes the issue.

Cheers,
simon


[-- Attachment #2: p.patch --]
[-- Type: text/x-diff, Size: 1246 bytes --]

From e1fdd6748ebb1088fb805d77cfb176758bab5618 Mon Sep 17 00:00:00 2001
Message-Id: <e1fdd6748ebb1088fb805d77cfb176758bab5618.1693826861.git.zimon.toutoune@gmail.com>
From: Simon Tournier <zimon.toutoune@gmail.com>
Date: Mon, 4 Sep 2023 13:23:59 +0200
Subject: [PATCH] guix: git: Add default case when resolving reference.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Reported by Ludovic Courtès <ludo@gnu.org>.

* guix/git.scm (resolve-reference): Return #false when the reference is not
resolved.
---
 guix/git.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/guix/git.scm b/guix/git.scm
index ebe2600209d4..d4076d4a0a0c 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -282,7 +282,8 @@ (define (resolve-reference repository ref)
          (if (= OBJ-TAG (object-type obj))
              (object-lookup repository
                             (tag-target-id (tag-lookup repository oid)))
-             obj))))))
+             obj)))
+      (_ #f))))
 
 (define (switch-to-ref repository ref)
   "Switch to REPOSITORY's branch, commit or tag specified by REF.  Return the

base-commit: bedcdf0fb5ac035f696790827679406c7146396c
-- 
2.38.1

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

[...]

>>From e1fdd6748ebb1088fb805d77cfb176758bab5618 Mon Sep 17 00:00:00 2001
> Message-Id: <e1fdd6748ebb1088fb805d77cfb176758bab5618.1693826861.git.zimon.toutoune@gmail.com>
> From: Simon Tournier <zimon.toutoune@gmail.com>
> Date: Mon, 4 Sep 2023 13:23:59 +0200
> Subject: [PATCH] guix: git: Add default case when resolving reference.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Reported by Ludovic Courtès <ludo@gnu.org>.
>
> * guix/git.scm (resolve-reference): Return #false when the reference is not
> resolved.
> ---
>  guix/git.scm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/guix/git.scm b/guix/git.scm
> index ebe2600209d4..d4076d4a0a0c 100644
> --- a/guix/git.scm
> +++ b/guix/git.scm
> @@ -282,7 +282,8 @@ (define (resolve-reference repository ref)
>           (if (= OBJ-TAG (object-type obj))
>               (object-lookup repository
>                              (tag-target-id (tag-lookup repository oid)))
> -             obj))))))
> +             obj)))
> +      (_ #f))))

This doesn't look right to me; the contract of resolve-reference is to
accept a REF, which is well defined.  It's not supposed fall into cracks
and return #f.  The problem lies elsewhere.

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi Maxim,

On Tue, 05 Sep 2023 at 16:33, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>> -             obj))))))
>> +             obj)))
>> +      (_ #f))))
>
> This doesn't look right to me; the contract of resolve-reference is to
> accept a REF, which is well defined.  It's not supposed fall into cracks
> and return #f.  The problem lies elsewhere.

Yes, the problem lies elsewhere!  By the code you introduced with
79ec651a286c71a3d4c72be33a1f80e76a560031.  As explained here:

        [bug#65352] Fix time-machine and network
        Simon Tournier <zimon.toutoune@gmail.com>
        Mon, 04 Sep 2023 19:37:08 +0200
        id:87wmx5on5n.fsf@gmail.com
        https://issues.guix.gnu.org//65352
        https://issues.guix.gnu.org/msgid/87wmx5on5n.fsf@gmail.com
        https://yhetil.org/guix/87wmx5on5n.fsf@gmail.com

Because of this code, you are breaking the contract and passing '() as
REF.  Hence my patch.

Cheers,
simon

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Hi again,

Simon Tournier <zimon.toutoune@gmail.com> skribis:

> * guix/git/scm (reference-available?): Rely of the procedure resolve-reference
> to determine if the reference belongs to the local Git checkout.
> ---
>  guix/git.scm | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/guix/git.scm b/guix/git.scm
> index dbc3b7caa7..ebe2600209 100644
> --- a/guix/git.scm
> +++ b/guix/git.scm
> @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp)
>  (define (reference-available? repository ref)
>    "Return true if REF, a reference such as '(commit . \"cabba9e\"), is
>  definitely available in REPOSITORY, false otherwise."
> -  (match ref
> -    ((or ('commit . commit)
> -         ('tag-or-commit . (? commit-id? commit)))
> -     (let ((len (string-length commit))
> -           (oid (string->oid commit)))
> -       (false-if-git-not-found
> -        (->bool (if (< len 40)
> -                    (object-lookup-prefix repository oid len OBJ-COMMIT)
> -                    (commit-lookup repository oid))))))
> -    (_
> -     #f)))
> +  (false-if-git-not-found
> +   (->bool (resolve-reference repository ref))))
>  
>  (define (clone-from-swh url tag-or-commit output)
>    "Attempt to clone TAG-OR-COMMIT (a string), which originates from URL, using
>
> base-commit: 1b2d43fe016848ea2ec16ff18cbc14340944fc4e

In fact, now I recall why that procedure was written that way: it’s
meant to say whether a given commit (and only a commit) is already in
the checkout, meaning we don’t need to pull.  By definition, it’s an
answer that can only be given for a specific commit; we cannot tell
whether “master” or “HEAD” is available, that wouldn’t make sense.

Thus, I think we need to revert
a789dd58656d5f7f1b8edf790d77753fc71670af, and probably add a comment
explaining why it’s written this way.

Thoughts?

Ludo’.

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi,

On Mon, 04 Sep 2023 at 11:32, Ludovic Courtès <ludo@gnu.org> wrote:

> In fact, now I recall why that procedure was written that way: it’s
> meant to say whether a given commit (and only a commit) is already in
> the checkout, meaning we don’t need to pull.  By definition, it’s an
> answer that can only be given for a specific commit; we cannot tell
> whether “master” or “HEAD” is available, that wouldn’t make sense.

Yeah, that’s the job of ’reference-available?’ to say if a given
reference is or not in the repository, IMHO.

The patch I proposed earlier fixes the issue you reported, I guess.


When debugging, I have noticed that this update-cached-checkout is
called many times.  For instance,
79ec651a286c71a3d4c72be33a1f80e76a560031 introduced a call.
Investigating, I notice that this new procedure is incorrect:

--8<---------------cut here---------------start------------->8---
       (define (validate-guix-channel channels)
         "Finds the Guix channel among CHANNELS, and validates that REF as
captured from the closure, a git reference specification such as a commit hash
or tag associated to CHANNEL, is valid and new enough to satisfy the 'guix
time-machine' requirements.  A `formatted-message' condition is raised
otherwise."
         (let* ((guix-channel (find guix-channel? channels))
                (checkout commit relation (update-cached-checkout
                                           (channel-url guix-channel)
                                           #:ref (or ref '())
                                           #:starting-commit
                                           %oldest-possible-commit)))
--8<---------------cut here---------------end--------------->8---

Here, the symbol ’ref’ is bound by:

            (ref          (assoc-ref opts 'ref))

which comes from:

         (option '("commit") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
         (option '("branch") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'ref `(branch . ,arg) result)))

Therefore, it means that when none of the options --commit= or --branch=
is provided by the user at the CLI, this ’ref’ is bounded to #false.

Therefore, it can lead to unexpected behaviour when providing a
channels.scm file.

Well, instead, the correct something like:

         (let* ((guix-channel (find guix-channel? channels))
                (reference (or ref
                               (match (channel-commit guix-channel)
                                 (#f `(branch . ,(channel-branch guix-channel)))
                                 (commit `(tag-or-commit . ,commit)))))
                (checkout commit relation (update-cached-checkout
                                           (channel-url guix-channel)
                                           #:ref reference
                                           #:starting-commit
                                           %oldest-possible-commit)))

which works using my tests (with or without network).

The remaining issue is the order when displaying messages.  This
’validate-guix-channel’ happens before “Updating channel 'guix'”
therefore the progress bar appears before etc.

I have not investigated how to improve cached-channel-instance.  Let me
know if the current tiny fix tweaking resolve-interface is enough for
now waiting some rework of validate-guix-channel. :-)

Cheers,
simon

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

[...]

> Well, instead, the correct something like:
>
>          (let* ((guix-channel (find guix-channel? channels))
>                 (reference (or ref
>                                (match (channel-commit guix-channel)
>                                  (#f `(branch . ,(channel-branch guix-channel)))
>                                  (commit `(tag-or-commit . ,commit)))))
>                 (checkout commit relation (update-cached-checkout
>                                            (channel-url guix-channel)
>                                            #:ref reference
>                                            #:starting-commit
>                                            %oldest-possible-commit)))
>
> which works using my tests (with or without network).

I've installed something along this with c3d48d0.  If there are other
issues, I think it'd be best if they are described clearly in a new
issue, as that one is getting crowded :-).

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi again,
>
> Simon Tournier <zimon.toutoune@gmail.com> skribis:
>
>> * guix/git/scm (reference-available?): Rely of the procedure resolve-reference
>> to determine if the reference belongs to the local Git checkout.
>> ---
>>  guix/git.scm | 13 ++-----------
>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/guix/git.scm b/guix/git.scm
>> index dbc3b7caa7..ebe2600209 100644
>> --- a/guix/git.scm
>> +++ b/guix/git.scm
>> @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp)
>>  (define (reference-available? repository ref)
>>    "Return true if REF, a reference such as '(commit . \"cabba9e\"), is
>>  definitely available in REPOSITORY, false otherwise."
>> -  (match ref
>> -    ((or ('commit . commit)
>> -         ('tag-or-commit . (? commit-id? commit)))
>> -     (let ((len (string-length commit))
>> -           (oid (string->oid commit)))
>> -       (false-if-git-not-found
>> -        (->bool (if (< len 40)
>> -                    (object-lookup-prefix repository oid len OBJ-COMMIT)
>> -                    (commit-lookup repository oid))))))
>> -    (_
>> -     #f)))
>> +  (false-if-git-not-found
>> +   (->bool (resolve-reference repository ref))))
>>  
>>  (define (clone-from-swh url tag-or-commit output)
>>    "Attempt to clone TAG-OR-COMMIT (a string), which originates from URL, using
>>
>> base-commit: 1b2d43fe016848ea2ec16ff18cbc14340944fc4e
>
> In fact, now I recall why that procedure was written that way: it’s
> meant to say whether a given commit (and only a commit) is already in
> the checkout, meaning we don’t need to pull.  By definition, it’s an
> answer that can only be given for a specific commit; we cannot tell
> whether “master” or “HEAD” is available, that wouldn’t make sense.
>
> Thus, I think we need to revert
> a789dd58656d5f7f1b8edf790d77753fc71670af, and probably add a comment
> explaining why it’s written this way.
>
> Thoughts?

I've reviewed this thread and the code, and I agree.  This is a special
case.  I've added a comment so we aren't tempted to use
'resolve-reference' there again.

Will install shortly.

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi Maxim,

On Tue, 05 Sep 2023 at 16:39, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> I've reviewed this thread and the code, and I agree.  This is a special
> case.  I've added a comment so we aren't tempted to use
> 'resolve-reference' there again.

I disagree.  There is no special case.  The culprit is the procedure
’validate-guix-channel’ as explained in:

        [bug#65352] Fix time-machine and network
        Simon Tournier <zimon.toutoune@gmail.com>
        Mon, 04 Sep 2023 19:37:08 +0200
        id:87wmx5on5n.fsf@gmail.com
        https://issues.guix.gnu.org//65352
        https://issues.guix.gnu.org/msgid/87wmx5on5n.fsf@gmail.com
        https://yhetil.org/guix/87wmx5on5n.fsf@gmail.com


> Will install shortly.

I do not know what you will install shortly.  The fix belong to
validate-guix-channel, something like:

         (let* ((guix-channel (find guix-channel? channels))
                (reference (or ref
                               (match (channel-commit guix-channel)
                                 (#f `(branch . ,(channel-branch guix-channel)))
                                 (commit `(tag-or-commit . ,commit)))))
                (checkout commit relation (update-cached-checkout
                                           (channel-url guix-channel)
                                           #:ref reference
                                           #:starting-commit
                                           %oldest-possible-commit)))

and that would avoid to break the “contract” of resolve-reference.
Before committing something, I was testing.

Cheers,
simon

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

Hi,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Tue, 05 Sep 2023 at 16:39, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> I've reviewed this thread and the code, and I agree.  This is a special
>> case.  I've added a comment so we aren't tempted to use
>> 'resolve-reference' there again.
>
> I disagree.  There is no special case.  The culprit is the procedure
> ’validate-guix-channel’ as explained in:

I was referring to the special case of resolved-reference? (that it
mustn't trust tags or branches in a git cache -- at least currently,
compared to resolve-reference.  Maybe we want to change that?

>         [bug#65352] Fix time-machine and network
>         Simon Tournier <zimon.toutoune@gmail.com>
>         Mon, 04 Sep 2023 19:37:08 +0200
>         id:87wmx5on5n.fsf@gmail.com
>         https://issues.guix.gnu.org//65352
>         https://issues.guix.gnu.org/msgid/87wmx5on5n.fsf@gmail.com
>         https://yhetil.org/guix/87wmx5on5n.fsf@gmail.com
>
>
>> Will install shortly.
>
> I do not know what you will install shortly.  The fix belong to
> validate-guix-channel, something like:
>
>          (let* ((guix-channel (find guix-channel? channels))
>                 (reference (or ref
>                                (match (channel-commit guix-channel)
>                                  (#f `(branch . ,(channel-branch guix-channel)))
>                                  (commit `(tag-or-commit . ,commit)))))
>                 (checkout commit relation (update-cached-checkout
>                                            (channel-url guix-channel)
>                                            #:ref reference
>                                            #:starting-commit
>                                            %oldest-possible-commit)))
>
> and that would avoid to break the “contract” of resolve-reference.
> Before committing something, I was testing.

That's orthogonal to the other issue discussed, right?  What I was
referring to about 'installing' was c3d48d02, which implements the above
(with let-bound variables and 'if' instead of match, but the logic is
the same).

I feel like we need to agree on what reference-available? is supposed to
achieve, and where it needs to differentiate from resolve-reference.

-- 
Thanks,
Maxim

bug#65352: Fix time-machine and network

[Maxim Cournoyer]

Hi,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> * guix/git/scm (reference-available?): Rely of the procedure resolve-reference
> to determine if the reference belongs to the local Git checkout.
> ---
>  guix/git.scm | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/guix/git.scm b/guix/git.scm
> index dbc3b7caa7..ebe2600209 100644
> --- a/guix/git.scm
> +++ b/guix/git.scm
> @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp)
>  (define (reference-available? repository ref)
>    "Return true if REF, a reference such as '(commit . \"cabba9e\"), is
>  definitely available in REPOSITORY, false otherwise."
> -  (match ref
> -    ((or ('commit . commit)
> -         ('tag-or-commit . (? commit-id? commit)))
> -     (let ((len (string-length commit))
> -           (oid (string->oid commit)))
> -       (false-if-git-not-found
> -        (->bool (if (< len 40)
> -                    (object-lookup-prefix repository oid len OBJ-COMMIT)
> -                    (commit-lookup repository oid))))))
> -    (_
> -     #f)))
> +  (false-if-git-not-found
> +   (->bool (resolve-reference repository ref))))

This was applied some time ago as
a789dd58656d5f7f1b8edf790d77753fc71670af.

Closing.

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi Maxim,

On Tue, 5 Sept 2023 at 15:24, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> This was applied some time ago as
> a789dd58656d5f7f1b8edf790d77753fc71670af.

Thanks for having applied it.

> Closing.

However, I do not think it should be closed.  Maybe you have missed:

    [bug#65352] Fix time-machine and network
    Ludovic Courtès <ludo@gnu.org>
    Mon, 04 Sep 2023 10:49:24 +0200
    id:87wmx6qq5n.fsf_-_@gnu.org
    https://issues.guix.gnu.org//65352
    https://issues.guix.gnu.org/msgid/87wmx6qq5n.fsf_-_@gnu.org
    https://yhetil.org/guix/87wmx6qq5n.fsf_-_@gnu.org

And this thread contains some fixes.  Well, from my point of view, the
discussion is still pending...

Cheers,
simon

bug#65352: Fix time-machine and network

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Tue, 5 Sept 2023 at 15:24, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> This was applied some time ago as
>> a789dd58656d5f7f1b8edf790d77753fc71670af.
>
> Thanks for having applied it.
>
>> Closing.
>
> However, I do not think it should be closed.  Maybe you have missed:
>
>     [bug#65352] Fix time-machine and network
>     Ludovic Courtès <ludo@gnu.org>
>     Mon, 04 Sep 2023 10:49:24 +0200
>     id:87wmx6qq5n.fsf_-_@gnu.org
>     https://issues.guix.gnu.org//65352
>     https://issues.guix.gnu.org/msgid/87wmx6qq5n.fsf_-_@gnu.org
>     https://yhetil.org/guix/87wmx6qq5n.fsf_-_@gnu.org
>
> And this thread contains some fixes.  Well, from my point of view, the
> discussion is still pending...

I had indeed missed its continuation!  I've reverted a789dd5865 with
756e336fa0 and installed c3d48d024, which should now validate the
branch/commit of a channel file as well.

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi Maxim,

On Wed, 6 Sept 2023 at 02:04, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> I had indeed missed its continuation!  I've reverted a789dd5865 with
> 756e336fa0 and installed c3d48d024, which should now validate the
> branch/commit of a channel file as well.

Thanks for the follow up.

The other issue about the order of the progress bar and the message
"Updating guix ..." is not yet fixed. :-) I am fine to open another
issue for that but since it appears to me the same patch series as
this one.  Well you are applying patches faster than I am able to
process my emails or comment your messages. ;-)  Anyway, I will open a
report for that order issue.

However, this bug #65352 is not done.

    https://issues.guix.gnu.org/65352#0

The bug I report is, for instance, consider "guix time-machine
--commit=v1.4.0", this will pass (tag-or-commit . "v1.4.0") as REF to
reference-available? which is not a commit-id? if I read correctly.
And so reference-available? will return #f triggered an network update
when the reference if already in the cache checkout.

It is similar with short commit hash as "guix time-machine
--commit=4a027d2".  That's what I reported.

I am fine with the revert 756e336fa008c2469b4a7317ad5c641ed48f25d6
waiting my fix for what I am reporting.  But I disagree with the
comment because that's incorrect.

In order to detect the tag or commit string, the procedure
reference-available? needs to implement the string tag case and the
short commit hash case, something like:

      (('tag-or-commit . str)
       (cond ((and (string-contains str "-g")
                   (match (string-split str #\-)
                     ((version ... revision g+commit)
                      (if (and (> (string-length g+commit) 4)
                               (string-every char-set:digit revision)
                               (string-every char-set:hex-digit
                                             (string-drop g+commit 1)))
                          ;; Looks like a 'git describe' style ID, like
                          ;; v1.3.0-7-gaa34d4d28d.
                          (string-drop g+commit 1)
                          #f))
                     (_ #f)))
              => (lambda (commit) (resolve `(commit . ,commit))))
             ((or (> (string-length str) 40)
                  (not (string-every char-set:hex-digit str)))
              (resolve `(tag . ,str)))      ;definitely a tag
             (else
              (catch 'git-error
                (lambda ()
                  (resolve `(tag . ,str)))
                (lambda _
                  ;; There's no such tag, so it must be a commit ID.
                  (resolve `(commit . ,str)))))))

which is the same as resolve-reference. ;-)  Hence my proposal.

I agree with your words: if REF passed to reference-available? is not
a valid REF defined by the docstring of update-cached-checkout, it
means that the "contract" is broken and so there is a bug.

It appears to me inconsistent to allow the clause (_ #f) in
reference-available? and not in resolve-reference.

Therefore, the change I proposed that is now reverted has just exposed
the bug. :-)

All in all, this issue should be kept open.


Cheers,
simon

[bug#65352] Fix time-machine and network

[Maxim Cournoyer]

reopen 65352
quit

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Wed, 6 Sept 2023 at 02:04, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> I had indeed missed its continuation!  I've reverted a789dd5865 with
>> 756e336fa0 and installed c3d48d024, which should now validate the
>> branch/commit of a channel file as well.
>
> Thanks for the follow up.
>
> The other issue about the order of the progress bar and the message
> "Updating guix ..." is not yet fixed. :-) I am fine to open another
> issue for that but since it appears to me the same patch series as
> this one.  Well you are applying patches faster than I am able to
> process my emails or comment your messages. ;-)  Anyway, I will open a
> report for that order issue.

OK, thank you.  It's a bit hard to keep track of multiple issues and
their resolutions in a longish thread.

> However, this bug #65352 is not done.
>
>     https://issues.guix.gnu.org/65352#0
>
> The bug I report is, for instance, consider "guix time-machine
> --commit=v1.4.0", this will pass (tag-or-commit . "v1.4.0") as REF to
> reference-available? which is not a commit-id? if I read correctly.
> And so reference-available? will return #f triggered an network update
> when the reference if already in the cache checkout.

I don't know if we want to consider tags are immutable or not; the
safest is to consider them an *not* immutable, which is what we had been
doing.  I agree it doesn't cover all the potential git refspecs; we can
get there if we want (although I suppose it's uncommon for someone to
try 'guix time-machine --commit=v1.3.0-47405-ge0767a24d0' or similar).

> It is similar with short commit hash as "guix time-machine
> --commit=4a027d2".  That's what I reported.

I'm not sure if short commit IDs should be treated as immutable, since
in theory they can collide; the safest would be to check if there are
collisions and report an error if there is; and this requires fetching
new objects first.

So, what is the behavior that we want?

-- 
Thanks,
Maxim

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi,

On Tue, 05 Sep 2023 at 22:00, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>>                                                  Anyway, I will open a
>> report for that order issue.
>
> OK, thank you.  It's a bit hard to keep track of multiple issues and
> their resolutions in a longish thread.

For cross-referencing, done with bug#65788,

        bug#65788: poor information when updating using “guix time-machine”
        Simon Tournier <zimon.toutoune@gmail.com>
        Wed, 06 Sep 2023 18:57:38 +0200
        id:87pm2vme7x.fsf@gmail.com
        https://yhetil.org/guix/87pm2vme7x.fsf@gmail.com
        https://issues.guix.gnu.org/msgid/87pm2vme7x.fsf@gmail.com

Cheers,
simon

[bug#65352] time-machine, unavailable network or Savannah down

[Simon Tournier]

Hi Maxim,

Let start another branch in that thread of #65352. :-)

Let start the discussion on good basis, let start with an example:

--8<---------------cut here---------------start------------->8---
$ guix describe
Generation 26   Jul 12 2023 09:13:39    (current)
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    branch: master
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330

$ guix time-machine --commit=4a027d2 -- describe
Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
Computing Guix derivation for 'x86_64-linux'... |
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
The following derivations will be built:
[...]
building profile with 1 package...
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
--8<---------------cut here---------------end--------------->8---

So far, so good.  Here all is cached and so on.  Now, let make
git.savannah.gnu.org unreachable by tweaking some stuff.  Then,

--8<---------------cut here---------------start------------->8---
$ guix time-machine --commit=4a027d2 -- describe
guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
--8<---------------cut here---------------end--------------->8---

Do we agree it is bug?  Do we agree that the behaviour is not POLA?

It is the same when specifying a release tag,

--8<---------------cut here---------------start------------->8---
$ guix time-machine --commit=v1.4.0 -- describe
guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
--8<---------------cut here---------------end--------------->8---

I think Guix needs to be reliable whatever the status of Savannah when a
local Git ref is in the local cached checkout.


After this introduction, let start the core discussion.


On Tue, 05 Sep 2023 at 22:00, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> I don't know if we want to consider tags are immutable or not; the
> safest is to consider them an *not* immutable, which is what we had been
> doing.  I agree it doesn't cover all the potential git refspecs; we can
> get there if we want (although I suppose it's uncommon for someone to
> try 'guix time-machine --commit=v1.3.0-47405-ge0767a24d0' or similar).

[...]

> I'm not sure if short commit IDs should be treated as immutable, since
> in theory they can collide; the safest would be to check if there are
> collisions and report an error if there is; and this requires fetching
> new objects first.

Well, the behaviour that I want is that it just works whatever the
status of Savannah when I have a local Git ref that matches what I
provide to ’guix time-machine’ (or guix pull or else).

I think you are inferring a rule from two corner-cases.  And from my
point of view, there are only hypothetical. :-)

1. About tag.  The ones from upstream are defacto immutable.  It is
uncommon that people set local tag under ~/.cache/guix/checkouts.  And,
the failure when Savannah is unreachable appears to me more annoying
than hypothetical mutable tags.  Therefore, I propose what I already
proposed. :-) It will make it works for most of the cases.

Even, what would happen if a tag is changed?  The user does not get the
same inferior for two invocations.  The question is: what triggers the
update of the cached checkout?

What is the consequence for not updating when the user-specified Git ref
is a mutable one (tag or else)?  Here, I am proposing to delay the
update until the next “guix pull” or “guix time-machine -q”, well until
the user invokes a command with a Git ref that does not belong to the
local cached checkout.

I do not see why this delay is a problem.  And it avoids an update.


2. About short commit IDs.  The same reasoning applies. :-)

About the collision, it is the same.  It only delays the collision
report.


All in all, I think that reference-available? needs to check if the Git
ref belongs to the local cached checkout and that’s all.  If it is, use
it, else update the local cached checkout.

At time t, the user-specificity Git ref can match some local Git ref but
not the upstream state.  And so?

Somehow, I am considering the local cached checkout as the primary
reference for looking up Git ref.

Do you see a potential issue that I am missing?


Cheers,
simon

[bug#65352] [PATCH v2] DRAFT git: Avoid touching the network unless needed in 'reference-available?'.

[Simon Tournier]

Follow-up of 756e336fa008c2469b4a7317ad5c641ed48f25d6 fixing the issue.

* guix/git/scm (reference-available?): Address case by case to determine
whether the reference exists in the local Git checkout.
---
 guix/git.scm | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

Hi,

Here a draft about what I think is the correct solution.

Well, the tests we have talked about are all passing.

Let me know what you think.

Cheers,
simon


diff --git a/guix/git.scm b/guix/git.scm
index 1cb87a45607b..1b3355109e42 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -5,6 +5,7 @@
 ;;; Copyright © 2021 Marius Bakke <marius@gnu.org>
 ;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
 ;;; Copyright © 2023 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -360,21 +361,16 @@ (define-syntax-rule (false-if-git-not-found exp)
 (define (reference-available? repository ref)
   "Return true if REF, a reference such as '(commit . \"cabba9e\"), is
 definitely available in REPOSITORY, false otherwise."
-  ;; Note: this must not rely on 'resolve-reference', as that procedure always
-  ;; resolves the references for branch names such as master.  The semantic we
-  ;; want here is that unless the reference is exact (e.g. a commit), the
-  ;; reference should not be considered available, as it could have changed on
-  ;; the remote.
   (match ref
-    ((or ('commit . commit)
-         ('tag-or-commit . (? commit-id? commit)))
-     (let ((len (string-length commit))
-           (oid (string->oid commit)))
-       (false-if-git-not-found
-        (->bool (if (< len 40)
-                    (object-lookup-prefix repository oid len OBJ-COMMIT)
-                    (commit-lookup repository oid))))))
+    (('commit . (? commit-id? commit))
+     (let ((oid (string->oid commit)))
+       (->bool (commit-lookup repository oid))))
+    ((or ('tag . str)
+         ('tag-or-commit . str))
+     (false-if-git-not-found
+      (->bool (resolve-reference repository ref))))
     (_
+     ;; For the others REF as branch or symref, the REF cannot be available
      #f)))
 
 (define (clone-from-swh url tag-or-commit output)

base-commit: 6113e0529d61df7425f64e30a6bf77f7cfdfe5a5
-- 
2.38.1

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Hi,

Simon Tournier <zimon.toutoune@gmail.com> skribis:

> Follow-up of 756e336fa008c2469b4a7317ad5c641ed48f25d6 fixing the issue.
>
> * guix/git/scm (reference-available?): Address case by case to determine
> whether the reference exists in the local Git checkout.

[...]

>  (define (reference-available? repository ref)
>    "Return true if REF, a reference such as '(commit . \"cabba9e\"), is
>  definitely available in REPOSITORY, false otherwise."
> -  ;; Note: this must not rely on 'resolve-reference', as that procedure always
> -  ;; resolves the references for branch names such as master.  The semantic we
> -  ;; want here is that unless the reference is exact (e.g. a commit), the
> -  ;; reference should not be considered available, as it could have changed on
> -  ;; the remote.
>    (match ref
> -    ((or ('commit . commit)
> -         ('tag-or-commit . (? commit-id? commit)))
> -     (let ((len (string-length commit))
> -           (oid (string->oid commit)))
> -       (false-if-git-not-found
> -        (->bool (if (< len 40)
> -                    (object-lookup-prefix repository oid len OBJ-COMMIT)
> -                    (commit-lookup repository oid))))))
> +    (('commit . (? commit-id? commit))
> +     (let ((oid (string->oid commit)))
> +       (->bool (commit-lookup repository oid))))
> +    ((or ('tag . str)
> +         ('tag-or-commit . str))
> +     (false-if-git-not-found
> +      (->bool (resolve-reference repository ref))))

IIUC, the differences compared to what we had are:

  1. 'tag references are now handled on the fast path
     (‘reference-available?’ can return #t);

  2. short commit strings are now always on the slow path
     (‘reference-available?’ always returns #f).

Is that correct?

It would be nice to have #1 without #2.

>      (_
> +     ;; For the others REF as branch or symref, the REF cannot be available

“For other values of REF such as branch or symref, the target is by
definition unavailable locally.”

Thanks,
Ludo’.

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi Ludo,

On Wed, 13 Sep 2023 at 22:16, Ludovic Courtès <ludo@gnu.org> wrote:

>> +    (('commit . (? commit-id? commit))
>> +     (let ((oid (string->oid commit)))
>> +       (->bool (commit-lookup repository oid))))
>> +    ((or ('tag . str)
>> +         ('tag-or-commit . str))
>> +     (false-if-git-not-found
>> +      (->bool (resolve-reference repository ref))))
>
> IIUC, the differences compared to what we had are:
>
>   1. 'tag references are now handled on the fast path
>      (‘reference-available?’ can return #t);
>
>   2. short commit strings are now always on the slow path
>      (‘reference-available?’ always returns #f).
>
> Is that correct?

No, or I am missing some details.

> It would be nice to have #1 without #2.

It’s already the case because of that:

         (option '("commit") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))

Currently, the heuristic to determine if it is a tag or a commit is
implemented by ’resolve-reference’.

Somehow, considering the command-line parser, the alternative is:

    #1 and #2 on the fast path (the patch)
 or
    #1 and #2 on the slow path (the current implementation)

Let ’pk’ (see below) to convince you. :-)

Before the proposed patch:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix time-machine --commit=v1.4.0 -- describe

;;; (ref (tag-or-commit . "v1.4.0"))

;;; (reference-available? #f)

;;; (remote-fetch NETWORK)
  C-c C-c

$ ./pre-inst-env guix time-machine --commit=8e2f32c -- describe

;;; (ref (tag-or-commit . "8e2f32c"))

;;; (reference-available? #f)

;;; (remote-fetch NETWORK)
  C-c C-c
--8<---------------cut here---------------end--------------->8---

After the proposed patch:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix time-machine --commit=v1.4.0 -- describe

;;; (ref (tag-or-commit . "v1.4.0"))

;;; (reference-available? #t)
  guix 8e2f32c
    repository URL: https://git.savannah.gnu.org/git/guix.git
    commit: 8e2f32cee982d42a79e53fc1e9aa7b8ff0514714

$ ./pre-inst-env guix time-machine --commit=8e2f32c -- describe

;;; (ref (tag-or-commit . "8e2f32c"))

;;; (reference-available? #t)
  guix 8e2f32c
    repository URL: https://git.savannah.gnu.org/git/guix.git
    commit: 8e2f32cee982d42a79e53fc1e9aa7b8ff0514714
--8<---------------cut here---------------end--------------->8---


Cheers,
simon

--8<---------------cut here---------------start------------->8---
diff --git a/guix/git.scm b/guix/git.scm
index 1cb87a45607b..c927555cce18 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -481,6 +481,8 @@ (define* (update-cached-checkout url
                              (repository-open cache-directory)
                              (clone/swh-fallback url ref cache-directory))))
      ;; Only fetch remote if it has not been cloned just before.
+     (pk 'ref ref)
+     (pk 'reference-available? (reference-available? repository ref))
      (when (and cache-exists?
                 (not (reference-available? repository ref)))
        (remote-fetch (remote-lookup repository "origin")
--8<---------------cut here---------------end--------------->8---

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Hi,

Simon Tournier <zimon.toutoune@gmail.com> skribis:

> On Wed, 13 Sep 2023 at 22:16, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>> +    (('commit . (? commit-id? commit))
>>> +     (let ((oid (string->oid commit)))
>>> +       (->bool (commit-lookup repository oid))))
>>> +    ((or ('tag . str)
>>> +         ('tag-or-commit . str))
>>> +     (false-if-git-not-found
>>> +      (->bool (resolve-reference repository ref))))
>>
>> IIUC, the differences compared to what we had are:
>>
>>   1. 'tag references are now handled on the fast path
>>      (‘reference-available?’ can return #t);
>>
>>   2. short commit strings are now always on the slow path
>>      (‘reference-available?’ always returns #f).
>>
>> Is that correct?
>
> No

Sorry, could you explain what the difference is then on the hunk I
quoted?

  https://issues.guix.gnu.org/65352#34-lineno11

I see different treatment of short commit IDs and tags, and no
difference for full commit IDs.

Thanks,
Ludo’.

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Hi again,

Simon Tournier <zimon.toutoune@gmail.com> skribis:

> Let ’pk’ (see below) to convince you. :-)
>
> Before the proposed patch:
>
> $ ./pre-inst-env guix time-machine --commit=v1.4.0 -- describe
>
> ;;; (ref (tag-or-commit . "v1.4.0"))
>
> ;;; (reference-available? #f)
>
> ;;; (remote-fetch NETWORK)
>   C-c C-c
>
> $ ./pre-inst-env guix time-machine --commit=8e2f32c -- describe
>
> ;;; (ref (tag-or-commit . "8e2f32c"))
>
> ;;; (reference-available? #f)
>
> ;;; (remote-fetch NETWORK)
>   C-c C-c

Yes this seems to confirm what I thought.

So anyway, go for it!

Great that you’re improving performance here.

Ludo’.

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi,

On Thu, 14 Sept 2023 at 11:04, Ludovic Courtès <ludo@gnu.org> wrote:

> Yes this seems to confirm what I thought.

Hum, maybe we have miscommunicated because we were speaking on
different levels, I guess. :-)

By 'tag references, you meant (tag . "foo") right?  And that case is
not possible from the command-line and even I am not sure about the
use-case of passing (tag . "foo") to reference-available?.  Another
story.

Reconsidering your question, yes the case (tag . "foo") is currently
on the fast path and will stay on the fast path.

I have read "tag references" as the user is passing a Git tag.  Which
is currently managed the same way as short commit ID.

Hence my previous answer. :-)

> So anyway, go for it!

Cool!  I will proceed.

> Great that you’re improving performance here.

Now, we can give a look to bug#65787 [1]. ;-)

1: bug#65787: time-machine is doing too much network requests
Simon Tournier <zimon.toutoune@gmail.com>
Mon, 11 Sep 2023 11:41:54 +0200
id:87tts1jbbx.fsf@gmail.com
https://yhetil.org/guix/87tts1jbbx.fsf@gmail.com
https://issues.guix.gnu.org/msgid/87tts1jbbx.fsf@gmail.com

Cheers,
simon

bug#65352: Fix time-machine and network

[Simon Tournier]

Hi,

On Thu, 14 Sep 2023 at 11:42, Simon Tournier <zimon.toutoune@gmail.com> wrote:

>> So anyway, go for it!
>
> Cool!  I will proceed.

Done with 6d33c1f8061e86d63ab5c9ec75df9c58130c7264.

Cheers,
simon

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Hi,

Ludovic Courtès <ludo@gnu.org> skribis:

> Yes this seems to confirm what I thought.
>
> So anyway, go for it!

Apologies, I clearly lost track of what I was saying.

There are two things we missed here:

  1. ‘false-if-git-not-found’ was removed around the call to
     ‘commit-lookup’, which breaks things as reported just today on
     IRC.  Could you reintroduce it?

  2. Short commit IDs are no longer handled in the 'commit case, as I
     mentioned before in this thread (and then forgot).  Could you
     reintroduce support for them?

(Cc’ing Chris who’s been debugging it and discussing it on IRC.)

Thanks,
Ludo’.

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi,

On Mon, 25 Sept 2023 at 11:32, Ludovic Courtès <ludo@gnu.org> wrote:

>   1. ‘false-if-git-not-found’ was removed around the call to
>      ‘commit-lookup’, which breaks things as reported just today on
>      IRC.  Could you reintroduce it?

About "guix system"?

Yes, for sure let reintroduce it.

But I miss why it would work for one case and not for the other.  I
was looking at 'check-forward-update'.

>   2. Short commit IDs are no longer handled in the 'commit case, as I
>      mentioned before in this thread (and then forgot).  Could you
>      reintroduce support for them?

Short commit ID are handled by tag-or-commit (guix time-machine and
guix pull).  If there is a discrepancy elsewhere with short commit ID,
it should be fixed overthere, IMHO.

Else, I do not understand what you are asking.  From my understanding,
it would not make sense to have short commit ID handled with (commit .
"abc123") for some part of the code and (tag-or-commit . "abc123") for
some other part.

Cheers,
simon

[bug#65352] Fix time-machine and network

[Simon Tournier]

Re

On Mon, 25 Sept 2023 at 11:57, Simon Tournier <zimon.toutoune@gmail.com> wrote:
> On Mon, 25 Sept 2023 at 11:32, Ludovic Courtès <ludo@gnu.org> wrote:
>
> >   1. ‘false-if-git-not-found’ was removed around the call to
> >      ‘commit-lookup’, which breaks things as reported just today on
> >      IRC.  Could you reintroduce it?
[...]
> Yes, for sure let reintroduce it.

Done with 94f3831e5bb1e04eeb3a0e7d31a0675208ce6f4c.

Cheers,
simon

[bug#65352] Fix time-machine and network

[Ludovic Courtès]

Simon Tournier <zimon.toutoune@gmail.com> skribis:

> On Mon, 25 Sept 2023 at 11:32, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>   1. ‘false-if-git-not-found’ was removed around the call to
>>      ‘commit-lookup’, which breaks things as reported just today on
>>      IRC.  Could you reintroduce it?
>
> About "guix system"?
>
> Yes, for sure let reintroduce it.
>
> But I miss why it would work for one case and not for the other.  I
> was looking at 'check-forward-update'.

‘commit-lookup’ throws to 'git-error when passed a commit that is not
found, that’s why.

>>   2. Short commit IDs are no longer handled in the 'commit case, as I
>>      mentioned before in this thread (and then forgot).  Could you
>>      reintroduce support for them?
>
> Short commit ID are handled by tag-or-commit (guix time-machine and
> guix pull).  If there is a discrepancy elsewhere with short commit ID,
> it should be fixed overthere, IMHO.
>
> Else, I do not understand what you are asking.  From my understanding,
> it would not make sense to have short commit ID handled with (commit .
> "abc123") for some part of the code and (tag-or-commit . "abc123") for
> some other part.

It the caller passes (commit . "1234"), this is no longer handled
efficiently as it used to be.

Maybe that’s a bit of a theoretical issue though because in practice
CLIs would pass (tag-or-commit . "1234"), right?

Thanks for your quick response!

Ludo’.

[bug#65352] Fix time-machine and network

[Simon Tournier]

Hi,

On Mon, 25 Sep 2023 at 17:01, Ludovic Courtès <ludo@gnu.org> wrote:

>>>   2. Short commit IDs are no longer handled in the 'commit case, as I
>>>      mentioned before in this thread (and then forgot).  Could you
>>>      reintroduce support for them?
>>
>> Short commit ID are handled by tag-or-commit (guix time-machine and
>> guix pull).  If there is a discrepancy elsewhere with short commit ID,
>> it should be fixed overthere, IMHO.
>>
>> Else, I do not understand what you are asking.  From my understanding,
>> it would not make sense to have short commit ID handled with (commit .
>> "abc123") for some part of the code and (tag-or-commit . "abc123") for
>> some other part.
>
> It the caller passes (commit . "1234"), this is no longer handled
> efficiently as it used to be.
>
> Maybe that’s a bit of a theoretical issue though because in practice
> CLIs would pass (tag-or-commit . "1234"), right?

Well, to my knowledge, ’guix pull’ and ’guix time-machine’ pass
'tag-or-commit for short commit ID.  Somehow, that’s the beginning of
all this. :-)

The story starts with 79ec651a286c71a3d4c72be33a1f80e76a560031 and
ecab937897385fce3e3ce0c5f128afba4304187c:

          (option '("commit") #t #f
                  (lambda (opt name arg result)
-                   (alist-cons 'ref `(commit . ,arg) result)))
+                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))

and then I just try to keep consistent some rest with these changes,
cleaning unnecessary network access.  The root of all is maybe this
change f36522416e69d95f222fdfa6404d1981eb5225b6, introducing
tag-or-commit.

Having all that in mind, we have to make clear how to internally
represent a short commit ID:

  + either (commit . "1234")
  + either (tag-or-commit . "1234")

but not both.  It appears to me a slippery slope with potential nasty
bugs if we mix the both.

From a CLI point of view, say “guix pull --comnit=foo123”, it is hard to
know beforehand if “foo123“ is a tag or a short commit ID, hence the
representation (tag-or-commit . "foo123") then resolved by the heuristic
implemented by ’resolve-reference’; as nicely implemented by f36522. :-)

Now, if elsewhere in the Guix code base, something is reading ‘foo123’
and constructs (commit . "foo123") in order to pass it to
’update-cached-checkout’, my opinion is that we need to correct it and
instead construct (tag-or-commit . "foo123").  Somehow, be in agreement
with 79ec65, ecab93 and f36522. :-)

To conclude, we had all this long thread discussion, partially because
REF is not explicitly specified but implicitly used here or there.

Therefore, to end this lengthy thread, I propose to send a patch
documenting these cases.  For example, update the docstring of
reference-available?.  Well, let close this « Fix time-machine and
network » since it is done, I guess.  And open another one for this
discussion about short commit ID internal representation.  WDYT?

Cheers,
simon

[bug#65352] time-machine, unavailable network or Savannah down

[Maxim Cournoyer]

Hi Simon, Ludovic,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> Let start another branch in that thread of #65352. :-)

Alright :-).

> Let start the discussion on good basis, let start with an example:
>
> $ guix describe
> Generation 26   Jul 12 2023 09:13:39    (current)
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     branch: master
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
> $ guix time-machine --commit=4a027d2 -- describe
> Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
> Computing Guix derivation for 'x86_64-linux'... |
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> The following derivations will be built:
> [...]
> building profile with 1 package...
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
>
> So far, so good.  Here all is cached and so on.  Now, let make
> git.savannah.gnu.org unreachable by tweaking some stuff.  Then,
>
> $ guix time-machine --commit=4a027d2 -- describe
> guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
>
>
> Do we agree it is bug?  Do we agree that the behaviour is not POLA?

Thanks for the example, it helps :-).  I agree it's an undesirable
behavior to reach to the network after having (supposedly) cached the
very same ref.

[...]

> On Tue, 05 Sep 2023 at 22:00, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> I don't know if we want to consider tags are immutable or not; the
>> safest is to consider them an *not* immutable, which is what we had been
>> doing.  I agree it doesn't cover all the potential git refspecs; we can
>> get there if we want (although I suppose it's uncommon for someone to
>> try 'guix time-machine --commit=v1.3.0-47405-ge0767a24d0' or similar).
>
> [...]
>
>> I'm not sure if short commit IDs should be treated as immutable, since
>> in theory they can collide; the safest would be to check if there are
>> collisions and report an error if there is; and this requires fetching
>> new objects first.
>
> Well, the behaviour that I want is that it just works whatever the
> status of Savannah when I have a local Git ref that matches what I
> provide to ’guix time-machine’ (or guix pull or else).
>
> I think you are inferring a rule from two corner-cases.  And from my
> point of view, there are only hypothetical. :-)

Also, from the current state of things (the code) :-).  But I agree that
there seems to be space for improvements here.

> 1. About tag.  The ones from upstream are defacto immutable.  It is
> uncommon that people set local tag under ~/.cache/guix/checkouts.  And,
> the failure when Savannah is unreachable appears to me more annoying
> than hypothetical mutable tags.  Therefore, I propose what I already
> proposed. :-) It will make it works for most of the cases.

More annoying but also, much more likely!

> Even, what would happen if a tag is changed?  The user does not get the
> same inferior for two invocations.  The question is: what triggers the
> update of the cached checkout?
>
> What is the consequence for not updating when the user-specified Git ref
> is a mutable one (tag or else)?  Here, I am proposing to delay the
> update until the next “guix pull” or “guix time-machine -q”, well until
> the user invokes a command with a Git ref that does not belong to the
> local cached checkout.
>
> I do not see why this delay is a problem.  And it avoids an update.
>
>
> 2. About short commit IDs.  The same reasoning applies. :-)
>
> About the collision, it is the same.  It only delays the collision
> report.

Sounds reasonable; it'll reduce some load from Savannah ;-).

>
> All in all, I think that reference-available? needs to check if the Git
> ref belongs to the local cached checkout and that’s all.  If it is, use
> it, else update the local cached checkout.
>
> At time t, the user-specificity Git ref can match some local Git ref but
> not the upstream state.  And so?
>
> Somehow, I am considering the local cached checkout as the primary
> reference for looking up Git ref.
>
> Do you see a potential issue that I am missing?

So all the refs such as commit(ish) or tags would be referenced locally,
and branches such as 'master' would still trigger an update.

LGTM, but I'd be curious to hear what Ludovic thinks, since their
original code treated tags as mutable (which they technically are, but I
agree to the value of treating them as immutable, and it appears low
risk to me).

-- 
Thanks,
Maxim

[bug#65352] time-machine, unavailable network or Savannah down

[Simon Tournier]

Hi,

On Wed, 06 Sep 2023 at 13:41, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> So all the refs such as commit(ish) or tags would be referenced locally,
> and branches such as 'master' would still trigger an update.

That’s the intent of this patch:

        [bug#65352] [PATCH v2] DRAFT git: Avoid touching the network unless needed in 'reference-available?'.
        Simon Tournier <zimon.toutoune@gmail.com>
        Wed, 06 Sep 2023 16:17:08 +0200
        id:32d3fb5066e0b20e200dabef0fba897634e21dda.1694009405.git.zimon.toutoune@gmail.com
        https://issues.guix.gnu.org//65352
        https://issues.guix.gnu.org/msgid/32d3fb5066e0b20e200dabef0fba897634e21dda.1694009405.git.zimon.toutoune@gmail.com
        https://yhetil.org/guix/32d3fb5066e0b20e200dabef0fba897634e21dda.1694009405.git.zimon.toutoune@gmail.com


> LGTM, but I'd be curious to hear what Ludovic thinks, since their
> original code treated tags as mutable (which they technically are, but I
> agree to the value of treating them as immutable, and it appears low
> risk to me).

Do we have an use-case where tags are mutable?

To my knowledge, the Guix remote tags have always been immutable.  Do we
have one counter-example?

Well, here an attempt for a scenario with mutable tags – although I
think that’s a corner case considering the current state for
manipulating Guix cache checkouts.  I am using Guix 6113e05, nothing
about the patch I am proposing. :-)

$ cp -r ~/.cache/guix/checkouts/pjmkglp4t7znuugeurpurzikxq3tnlaywmisyr27shj7apsnalwq /tmp/guix,git
$ guix time-machine -q --commit=4a027d2 --url=/tmp/guix.git -- describe

So far, so good.  Let add one tag.

$ git -C /tmp/guix.git tag -a mutable -m "some tag" 4a027d2
$ git -C /tmp/guix.git tag -l mut*

And…

$ guix time-machine -q --commit=mutable --url=/tmp/guix.git -- describe
guix time-machine: error: Git error: reference 'refs/tags/mutable' not found

…bang!

Well, the basic Git tags does not seem supported by the Guile-Git
’remote-fetch’ procedure.  I have not investigated more.  Maybe I am
missing something.

My opinion is to stay focused on the current real annoyances first and
not try to fix another hypothetical use-case which seems already buggy.

Ludo, WDYT about the proposed patch?  Does it work for your use-cases?

Cheers,
simon

[bug#64746] [PATCH v2 1/3] git: Clarify commit relation reference in doc.

[Maxim Cournoyer]

* guix/git.scm (update-cached-checkout): Clarify that it is the relation of
STARTING-COMMIT that is returned, relative to the new commit, not the other
way around.
---

(no changes since v1)

 guix/git.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/git.scm b/guix/git.scm
index be20cde019..dbc3b7caa7 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -439,7 +439,7 @@ (define* (update-cached-checkout url
                                    #:recursive? recursive?)))
   "Update the cached checkout of URL to REF in CACHE-DIRECTORY.  Return three
 values: the cache directory name, and the SHA1 commit (a string) corresponding
-to REF, and the relation of the new commit relative to STARTING-COMMIT (if
+to REF, and the relation of STARTING-COMMIT relative to the new commit (if
 provided) as returned by 'commit-relation'.
 
 REF is pair whose key is [branch | commit | tag | tag-or-commit ] and value

base-commit: a4bed14c438dc0cbc1c1885a38f8409c7fef7957
-- 
2.41.0

[bug#64746] [PATCH v2 2/3] pull: Tag commit argument with 'tag-or-commit.

[Maxim Cournoyer]

For compatibility with (guix git) procedures.

* guix/scripts/pull.scm (channel-list): Also accept tag-or-commit tagged
refspec.
---

New commit.

 guix/scripts/pull.scm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
index ecd264d3fa..9b78d4b5ca 100644
--- a/guix/scripts/pull.scm
+++ b/guix/scripts/pull.scm
@@ -166,7 +166,7 @@ (define %options
                                (alist-delete 'repository-url result))))
          (option '("commit") #t #f
                  (lambda (opt name arg result)
-                   (alist-cons 'ref `(commit . ,arg) result)))
+                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
          (option '("branch") #t #f
                  (lambda (opt name arg result)
                    (alist-cons 'ref `(branch . ,arg) result)))
@@ -774,7 +774,8 @@ (define (channel-list opts)
                (if (guix-channel? c)
                    (let ((url (or url (channel-url c))))
                      (match ref
-                       (('commit . commit)
+                       ((or ('commit . commit)
+                            ('tag-or-commit . commit))
                         (channel (inherit c)
                                  (url url) (commit commit) (branch #f)))
                        (('branch . branch)
-- 
2.41.0

[bug#64746] [PATCH v2 2/3] pull: Tag commit argument with 'tag-or-commit.

[Simon Tournier]

Hi Maxim,

On Tue, 15 Aug 2023 at 15:44, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
> For compatibility with (guix git) procedures.
>
> * guix/scripts/pull.scm (channel-list): Also accept tag-or-commit tagged
> refspec.
> ---

I am not sure to understand these both changes.

>
>  guix/scripts/pull.scm | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
> index ecd264d3fa..9b78d4b5ca 100644
> --- a/guix/scripts/pull.scm
> +++ b/guix/scripts/pull.scm
> @@ -166,7 +166,7 @@ (define %options
>                                 (alist-delete 'repository-url result))))
>           (option '("commit") #t #f
>                   (lambda (opt name arg result)
> -                   (alist-cons 'ref `(commit . ,arg) result)))
> +                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))

Well, why not. :-)

>           (option '("branch") #t #f
>                   (lambda (opt name arg result)
>                     (alist-cons 'ref `(branch . ,arg) result)))
> @@ -774,7 +774,8 @@ (define (channel-list opts)
>                 (if (guix-channel? c)
>                     (let ((url (or url (channel-url c))))
>                       (match ref
> -                       (('commit . commit)
> +                       ((or ('commit . commit)
> +                            ('tag-or-commit . commit))

Here, why not also add 'tag?.  Hum, I am missing why this ’tag-or-commit
would be required.

Cheers,
simon

[bug#64746] [PATCH v2 2/3] pull: Tag commit argument with 'tag-or-commit.

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Tue, 15 Aug 2023 at 15:44, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>> For compatibility with (guix git) procedures.
>>
>> * guix/scripts/pull.scm (channel-list): Also accept tag-or-commit tagged
>> refspec.
>> ---
>
> I am not sure to understand these both changes.
>
>>
>>  guix/scripts/pull.scm | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
>> index ecd264d3fa..9b78d4b5ca 100644
>> --- a/guix/scripts/pull.scm
>> +++ b/guix/scripts/pull.scm
>> @@ -166,7 +166,7 @@ (define %options
>>                                 (alist-delete 'repository-url result))))
>>           (option '("commit") #t #f
>>                   (lambda (opt name arg result)
>> -                   (alist-cons 'ref `(commit . ,arg) result)))
>> +                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
>
> Well, why not. :-)

The reason is to standardize the API of (guix pull) and (guix git),
whose procedure had a different expectation for 'ref' objects.

-- 
Thanks,
Maxim

[bug#64746] [PATCH v2 2/3] pull: Tag commit argument with 'tag-or-commit.

[Simon Tournier]

Hi Maxim,

On Wed, 16 Aug 2023 at 14:47, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>>>           (option '("commit") #t #f
>>>                   (lambda (opt name arg result)
>>> -                   (alist-cons 'ref `(commit . ,arg) result)))
>>> +                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))

[...]

>                       (match ref
> -                       (('commit . commit)
> +                       ((or ('commit . commit)
> +                            ('tag-or-commit . commit))

> The reason is to standardize the API of (guix pull) and (guix git),
> whose procedure had a different expectation for 'ref' objects.

My point is that this ’or’ is useless, IIUC.  Well, I have removed it in
the series fixing the annoyance with the network access of “guix
time-machine”.

Cheers,
simon

[bug#64746] [PATCH v2 2/3] pull: Tag commit argument with 'tag-or-commit.

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Wed, 16 Aug 2023 at 14:47, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>>>>           (option '("commit") #t #f
>>>>                   (lambda (opt name arg result)
>>>> -                   (alist-cons 'ref `(commit . ,arg) result)))
>>>> +                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
>
> [...]
>
>>                       (match ref
>> -                       (('commit . commit)
>> +                       ((or ('commit . commit)
>> +                            ('tag-or-commit . commit))
>
>> The reason is to standardize the API of (guix pull) and (guix git),
>> whose procedure had a different expectation for 'ref' objects.
>
> My point is that this ’or’ is useless, IIUC.  Well, I have removed it in
> the series fixing the annoyance with the network access of “guix
> time-machine”.

It isn't, unless you meant after applying further changes :-) You should
be able to see the problem by reverting that commit and running 'guix
time-machine --commit=v1.4.0 -- describe', for example.

-- 
Thanks,
Maxim

[bug#64746] [PATCH v2 2/3] pull: Tag commit argument with 'tag-or-commit.

[Simon Tournier]

Re Maxim,

On Thu, 17 Aug 2023 at 20:16, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> > On Wed, 16 Aug 2023 at 14:47, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
> >
> >>>>           (option '("commit") #t #f
> >>>>                   (lambda (opt name arg result)
> >>>> -                   (alist-cons 'ref `(commit . ,arg) result)))
> >>>> +                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
> >
> > [...]
> >
> >>                       (match ref
> >> -                       (('commit . commit)
> >> +                       ((or ('commit . commit)
> >> +                            ('tag-or-commit . commit))
> >
> >> The reason is to standardize the API of (guix pull) and (guix git),
> >> whose procedure had a different expectation for 'ref' objects.
> >
> > My point is that this ’or’ is useless, IIUC.  Well, I have removed it in
> > the series fixing the annoyance with the network access of “guix
> > time-machine”.
>
> It isn't, unless you meant after applying further changes :-) You should
> be able to see the problem by reverting that commit and running 'guix
> time-machine --commit=v1.4.0 -- describe', for example.

Yes for sure because you introduced this in guix/scripts/time-machine.scm,

                  (lambda (opt name arg result)
-                   (alist-cons 'ref `(commit . ,arg) result)))
+                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))

with the previous commit 79ec651a286c71a3d4c72be33a1f80e76a560031.

Well, I feel there is a misunderstanding. :-)  And maybe I am missing
something... IIUC, the option parser:

         (option '("commit") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'ref `(commit . ,arg) result)))

implemented in guix/scrtips/pull.scm provides a way to construct this
REF.  This way should be compliant with the other one in
guix/scripts/time-machine.scm -- at least for being consistent.  And I
am missing the reason why you introduced this difference.

If the both option parsers use the same way, then the 'or' is useless.

As I said initially commenting this patch v2 2/3:

--8<---------------cut here---------------start------------->8---
> For compatibility with (guix git) procedures.
>
> * guix/scripts/pull.scm (channel-list): Also accept tag-or-commit tagged
> refspec.
> ---

I am not sure to understand these both changes.
--8<---------------cut here---------------end--------------->8---

Anyway, my other message [1] in #65352 contains the two alternatives
for closing this discussion. :-)

1: https://issues.guix.gnu.org/65352#4


Cheers,
simon

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

Hi again,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Re Maxim,
>
> On Thu, 17 Aug 2023 at 20:16, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> > On Wed, 16 Aug 2023 at 14:47, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>> >
>> >>>>           (option '("commit") #t #f
>> >>>>                   (lambda (opt name arg result)
>> >>>> -                   (alist-cons 'ref `(commit . ,arg) result)))
>> >>>> +                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
>> >
>> > [...]
>> >
>> >>                       (match ref
>> >> -                       (('commit . commit)
>> >> +                       ((or ('commit . commit)
>> >> +                            ('tag-or-commit . commit))
>> >
>> >> The reason is to standardize the API of (guix pull) and (guix git),
>> >> whose procedure had a different expectation for 'ref' objects.
>> >
>> > My point is that this ’or’ is useless, IIUC.  Well, I have removed it in
>> > the series fixing the annoyance with the network access of “guix
>> > time-machine”.
>>
>> It isn't, unless you meant after applying further changes :-) You should
>> be able to see the problem by reverting that commit and running 'guix
>> time-machine --commit=v1.4.0 -- describe', for example.
>
> Yes for sure because you introduced this in guix/scripts/time-machine.scm,
>
>                   (lambda (opt name arg result)
> -                   (alist-cons 'ref `(commit . ,arg) result)))
> +                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
>
> with the previous commit 79ec651a286c71a3d4c72be33a1f80e76a560031.

It's a bit convoluted, but there are three things involved:

1. (guix scripts time-machine)
2. (guix pull)
3. (guix git)

They are involved in that order, if I remember correctly.

Now the important part is that the update-cached-checkout from (guix
git), newly used in (guix scripts time-machine), should be passed a
tag-or-commit ref and not a commit one if we want to support both tags
or commits (otherwise tags would throw an error about not respecting a
git hash format).

Since 'guix time-machine --commit' is documented as accepting a tag or
commit, it makes sense to tag it as such at that point.

I hope this clarifies it?

-- 
Thanks,
Maxim

[bug#64746] [PATCH 2/2] scripts: time-machine: Error when attempting to visit too old commits.

[Simon Tournier]

Hi Maxim,

On Tue, 22 Aug 2023 at 22:54, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> 1. (guix scripts time-machine)
> 2. (guix pull)
> 3. (guix git)

[...]

> I hope this clarifies it?

All is clear since my initial comment [1] about this patch. ;-) And my
proposal for improving the asymmetry your patch introduced is summarized
by the alternative options I wrote in [2].

Therefore, I will continue the discussion in #65352. :-)

1: https://issues.guix.gnu.org/64746#13
2: https://issues.guix.gnu.org/65352#4

Cheers,
simon

[bug#64746] [PATCH v2 3/3] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

* doc/guix.texi (Invoking guix time-machine): Document limitation.
* guix/inferior.scm (cached-channel-instance): New VALIDATE-CHANNELS
argument.  Use it to validate channels when there are no cache hit.
* guix/scripts/time-machine.scm
(%options): Tag the given reference with 'tag-or-commit instead of 'commit.
(%oldest-possible-commit): New variable.
(guix-time-machine) <validate-guix-channel>: New nested procedure.  Pass it to
the 'cached-channel-instance' call.
* tests/guix-time-machine.sh: New test.
* Makefile.am (SH_TESTS): Register it.

Suggested-by: Simon Tournier <zimon.toutoune@gmail.com>
Reviewed-by: Ludovic Courtès <ludo@gnu.org>

---

Changes in v2:
- Test it
- Delay validation work to inside cached-channel-instance, when there's no
cache hit
- Expound doc to mention possible problems and possible workarounds

 Makefile.am                   |  1 +
 doc/guix.texi                 | 14 +++++++++
 guix/inferior.scm             | 57 ++++++++++++++++++++---------------
 guix/scripts/time-machine.scm | 38 +++++++++++++++++++++--
 tests/guix-time-machine.sh    | 28 +++++++++++++++++
 5 files changed, 110 insertions(+), 28 deletions(-)
 create mode 100644 tests/guix-time-machine.sh

diff --git a/Makefile.am b/Makefile.am
index 5ffcb6a12d..4228c07803 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -615,6 +615,7 @@ SH_TESTS =					\
   tests/guix-refresh.sh				\
   tests/guix-shell.sh				\
   tests/guix-shell-export-manifest.sh		\
+  tests/guix-time-machine.sh			\
   tests/guix-graph.sh				\
   tests/guix-describe.sh			\
   tests/guix-repl.sh     			\
diff --git a/doc/guix.texi b/doc/guix.texi
index b50feed4c4..a3754b7019 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -5060,6 +5060,20 @@ Invoking guix time-machine
 large number of packages; the result is cached though and subsequent
 commands targeting the same commit are almost instantaneous.
 
+Due to @command{guix time-machine} relying on the ``inferiors''
+mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
+commit @samp{6298c3ff} (``v1.0.0''), dated May 1@sup{st}, 2019, which is
+the first release that included the inferiors mechanism.  An error is
+returned when attempting to navigate to older commits.  Note that
+although it should technically be possible to travel to such an old
+commit, the ease to do so will largely depend on the availability of
+binary substitutes.  When traveling to a distant past, some packages may
+not easily build from source anymore.  One such example are old versions
+of Python 2 which had time bombs in its test suite, in the form of
+expiring SSL certificates.  This particular problem can be worked around
+by setting the hardware clock to a value in the past before attempting
+the build.
+
 @quotation Note
 The history of Guix is immutable and @command{guix time-machine}
 provides the exact same software as they are in a specific Guix
diff --git a/guix/inferior.scm b/guix/inferior.scm
index 5dfd30a6c8..fca6fb4b22 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -871,11 +871,15 @@ (define* (cached-channel-instance store
                                   #:key
                                   (authenticate? #t)
                                   (cache-directory (%inferior-cache-directory))
-                                  (ttl (* 3600 24 30)))
+                                  (ttl (* 3600 24 30))
+                                  validate-channels)
   "Return a directory containing a guix filetree defined by CHANNELS, a list of channels.
-The directory is a subdirectory of CACHE-DIRECTORY, where entries can be reclaimed after TTL seconds.
-This procedure opens a new connection to the build daemon.  AUTHENTICATE?
-determines whether CHANNELS are authenticated."
+The directory is a subdirectory of CACHE-DIRECTORY, where entries can be
+reclaimed after TTL seconds.  This procedure opens a new connection to the
+build daemon.  AUTHENTICATE? determines whether CHANNELS are authenticated.
+VALIDATE-CHANNELS, if specified, must be a one argument procedure accepting a
+list of channels that can be used to validate the channels; it should raise an
+exception in case of problems."
   (define commits
     ;; Since computing the instances of CHANNELS is I/O-intensive, use a
     ;; cheaper way to get the commit list of CHANNELS.  This limits overhead
@@ -923,27 +927,30 @@ (define* (cached-channel-instance store
 
   (if (file-exists? cached)
       cached
-      (run-with-store store
-        (mlet* %store-monad ((instances
-                              -> (latest-channel-instances store channels
-                                                           #:authenticate?
-                                                           authenticate?))
-                             (profile
-                              (channel-instances->derivation instances)))
-          (mbegin %store-monad
-            ;; It's up to the caller to install a build handler to report
-            ;; what's going to be built.
-            (built-derivations (list profile))
-
-            ;; Cache if and only if AUTHENTICATE? is true.
-            (if authenticate?
-                (mbegin %store-monad
-                  (symlink* (derivation->output-path profile) cached)
-                  (add-indirect-root* cached)
-                  (return cached))
-                (mbegin %store-monad
-                  (add-temp-root* (derivation->output-path profile))
-                  (return (derivation->output-path profile)))))))))
+      (begin
+        (when (procedure? validate-channels)
+          (validate-channels channels))
+        (run-with-store store
+          (mlet* %store-monad ((instances
+                                -> (latest-channel-instances store channels
+                                                             #:authenticate?
+                                                             authenticate?))
+                               (profile
+                                (channel-instances->derivation instances)))
+            (mbegin %store-monad
+              ;; It's up to the caller to install a build handler to report
+              ;; what's going to be built.
+              (built-derivations (list profile))
+
+              ;; Cache if and only if AUTHENTICATE? is true.
+              (if authenticate?
+                  (mbegin %store-monad
+                    (symlink* (derivation->output-path profile) cached)
+                    (add-indirect-root* cached)
+                    (return cached))
+                  (mbegin %store-monad
+                    (add-temp-root* (derivation->output-path profile))
+                    (return (derivation->output-path profile))))))))))
 
 (define* (inferior-for-channels channels
                                 #:key
diff --git a/guix/scripts/time-machine.scm b/guix/scripts/time-machine.scm
index d7c71ef705..e4fe511382 100644
--- a/guix/scripts/time-machine.scm
+++ b/guix/scripts/time-machine.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2019 Konrad Hinsen <konrad.hinsen@fastmail.net>
 ;;; Copyright © 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -19,13 +20,15 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (guix scripts time-machine)
+  #:use-module (guix channels)
+  #:use-module (guix diagnostics)
   #:use-module (guix ui)
   #:use-module (guix scripts)
   #:use-module (guix inferior)
   #:use-module (guix store)
   #:use-module (guix status)
   #:use-module ((guix git)
-                #:select (with-git-error-handling))
+                #:select (update-cached-checkout with-git-error-handling))
   #:use-module ((guix utils)
                 #:select (%current-system))
   #:use-module ((guix scripts pull)
@@ -38,9 +41,17 @@ (define-module (guix scripts time-machine)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
+  #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-37)
+  #:use-module (srfi srfi-71)
   #:export (guix-time-machine))
 
+;;; The required inferiors mechanism relied on by 'guix time-machine' was
+;;; firmed up in v1.0.0; it is the oldest, safest commit that can be travelled
+;;; to.
+(define %oldest-possible-commit
+  "6298c3ffd9654d3231a6f25390b056483e8f407c") ;v1.0.0
+
 \f
 ;;;
 ;;; Command-line options.
@@ -81,7 +92,7 @@ (define %options
                                (alist-delete 'repository-url result))))
          (option '("commit") #t #f
                  (lambda (opt name arg result)
-                   (alist-cons 'ref `(commit . ,arg) result)))
+                   (alist-cons 'ref `(tag-or-commit . ,arg) result)))
          (option '("branch") #t #f
                  (lambda (opt name arg result)
                    (alist-cons 'ref `(branch . ,arg) result)))
@@ -140,8 +151,27 @@ (define-command (guix-time-machine . args)
      (let* ((opts         (parse-args args))
             (channels     (channel-list opts))
             (command-line (assoc-ref opts 'exec))
+            (ref          (assoc-ref opts 'ref))
             (substitutes?  (assoc-ref opts 'substitutes?))
             (authenticate? (assoc-ref opts 'authenticate-channels?)))
+
+       (define (validate-guix-channel channels)
+         "Finds the Guix channel among CHANNELS, and validates that REF as
+captured from the closure, a git reference specification such as a commit hash
+or tag associated to CHANNEL, is valid and new enough to satisfy the 'guix
+time-machine' requirements.  A `formatted-message' condition is raised
+otherwise."
+         (let* ((guix-channel (find guix-channel? channels))
+                (checkout commit relation (update-cached-checkout
+                                           (channel-url guix-channel)
+                                           #:ref (or ref '())
+                                           #:starting-commit
+                                           %oldest-possible-commit)))
+           (unless (memq relation '(ancestor self))
+             (raise (formatted-message
+                     (G_ "cannot travel past commit `~a' from May 1st, 2019")
+                     (string-take %oldest-possible-commit 12))))))
+
        (when command-line
          (let* ((directory
                  (with-store store
@@ -153,6 +183,8 @@ (define-command (guix-time-machine . args)
                                                          #:dry-run? #f)
                        (set-build-options-from-command-line store opts)
                        (cached-channel-instance store channels
-                                                #:authenticate? authenticate?)))))
+                                                #:authenticate? authenticate?
+                                                #:validate-channels
+                                                validate-guix-channel)))))
                 (executable (string-append directory "/bin/guix")))
            (apply execl (cons* executable executable command-line))))))))
diff --git a/tests/guix-time-machine.sh b/tests/guix-time-machine.sh
new file mode 100644
index 0000000000..8b62ef75ea
--- /dev/null
+++ b/tests/guix-time-machine.sh
@@ -0,0 +1,28 @@
+# GNU Guix --- Functional package management for GNU
+# Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+#
+# This file is part of GNU Guix.
+#
+# GNU Guix is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or (at
+# your option) any later version.
+#
+# GNU Guix is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+#
+# Test the 'guix time-machine' command-line utility.
+#
+
+guix time-machine --version
+
+# Visiting a commit older than v1.0.0 fails.
+! guix time-machine --commit=v0.15.0
+
+exit 0
-- 
2.41.0

[bug#64746] [PATCH v2 3/3] scripts: time-machine: Error when attempting to visit too old commits.

[Simon Tournier]

Hi Maxim,

For the record, I have documented [1] the various roadblocks when using
“guix time-machine” rebuilding all from source.  Time-bomb is one among
other annoyances – I have in mind the complete bootstrap.

Well, as Ludo pointed, the CI is currently building all the past
releases.  Waiting the fixes for all the bugs, I suggest that we retain
the substitutes for the release.  I mean that,

    guix time-machine --commit=v1.X.0 -- help

just works when substitutes are available.  If the project is lacking
disk space, the University of Montpellier (France) is proposing to store
some binary artifact outputs.  At least, they were proposing back on
past September in 10 Years of Guix event. :-)  There is discussion on
guix-sysadmin, I guess.  Maybe we could resume this discussion and
complete the last steps.  WDYT?

1: https://simon.tournier.info/posts/2023-06-23-hackathon-repro.html


On Tue, 15 Aug 2023 at 15:44, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
> * doc/guix.texi (Invoking guix time-machine): Document limitation.
> * guix/inferior.scm (cached-channel-instance): New VALIDATE-CHANNELS
> argument.  Use it to validate channels when there are no cache hit.
> * guix/scripts/time-machine.scm
> (%options): Tag the given reference with 'tag-or-commit instead of 'commit.
> (%oldest-possible-commit): New variable.
> (guix-time-machine) <validate-guix-channel>: New nested procedure.  Pass it to
> the 'cached-channel-instance' call.
> * tests/guix-time-machine.sh: New test.
> * Makefile.am (SH_TESTS): Register it.

Cool!  That’s a nice usability improvement.

> diff --git a/doc/guix.texi b/doc/guix.texi
> index b50feed4c4..a3754b7019 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -5060,6 +5060,20 @@ Invoking guix time-machine
>  large number of packages; the result is cached though and subsequent
>  commands targeting the same commit are almost instantaneous.
>  
> +Due to @command{guix time-machine} relying on the ``inferiors''
> +mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
> +commit @samp{6298c3ff} (``v1.0.0''), dated May 1@sup{st}, 2019, which is
> +the first release that included the inferiors mechanism.  An error is
> +returned when attempting to navigate to older commits.

There is also some issue with bootstrapping depending on your hardware.

About time-bomb, there are also gnutls and openssl or libgit2.  It was
probably transparent for you because there are substitutable, I guess.
While Python 2 had probably been removed for some reasons.

Well, I would move the workaround to some dedicated block and move this
comment after the note about security

--8<---------------cut here---------------start------------->8---
@quotation Note
The history of Guix is immutable and @command{guix time-machine}
provides the exact same software as they are in a specific Guix
revision.  Naturally, no security fixes are provided for old versions
of Guix or its channels.  A careless use of @command{guix time-machine}
opens the door to security vulnerabilities.  @xref{Invoking guix pull,
@option{--allow-downgrades}}.
@end quotation
 
+Due to @command{guix time-machine} relying on the ``inferiors''
+mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
+commit @samp{6298c3ff} (``v1.0.0''), dated May 1@sup{st}, 2019, which is
+the first release that included the inferiors mechanism.  An error is
+returned when attempting to navigate to older commits.
+
+@quotation Note
+Although it should technically be possible to travel to such an old
+revision, the ease to do so will largely depend on the availability of
+binary substitutes.  When traveling to a distant past, some packages may
+not easily build from source anymore.  One such example are old versions
+of Python 2 which had time bombs in its test suite, in the form of
+expiring SSL certificates.  This particular problem can be worked around
+by setting the hardware clock to a value in the past before attempting
+the build.
+@end quotation
+
 The general syntax is:
 
 @example
--8<---------------cut here---------------end--------------->8---


> new file mode 100644
> index 0000000000..8b62ef75ea
> --- /dev/null
> +++ b/tests/guix-time-machine.sh

[...]

> +# Visiting a commit older than v1.0.0 fails.
> +! guix time-machine --commit=v0.15.0

Cool to add test.  But this test needs a network access:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix time-machine --commit=v0.15.0 -- describe
guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
--8<---------------cut here---------------end--------------->8---

It’s as I said elsewhere. :-)  Well, I have not investigated more but I
guess a bug in some Git manipulation.


Cheers,
simon

bug#64746: [PATCH v2 3/3] scripts: time-machine: Error when attempting to visit too old commits.

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> For the record, I have documented [1] the various roadblocks when using
> “guix time-machine” rebuilding all from source.  Time-bomb is one among
> other annoyances – I have in mind the complete bootstrap.
>
> Well, as Ludo pointed, the CI is currently building all the past
> releases.  Waiting the fixes for all the bugs, I suggest that we retain
> the substitutes for the release.  I mean that,
>
>     guix time-machine --commit=v1.X.0 -- help
>
> just works when substitutes are available.  If the project is lacking
> disk space, the University of Montpellier (France) is proposing to store
> some binary artifact outputs.  At least, they were proposing back on
> past September in 10 Years of Guix event. :-)  There is discussion on
> guix-sysadmin, I guess.  Maybe we could resume this discussion and
> complete the last steps.  WDYT?

That make sense.  The current means this can be achieved is by having a
jobset for each release in Cuirass, as hinted in doc/release.org from
the guix-maintenance repo:

--8<---------------cut here---------------start------------->8---
** Adding a Cuirass jobset for branch =version-X.Y.Z=

This jobset will have to be kept until the next release, so that
substitutes remain available.  The easiest way to add a new jobset is
directly via the web interface of Cuirass.  To be allowed to do so,
you must authenticate with the Cuirass instance via a private TLS
certificate imported into your browser.
--8<---------------cut here---------------end--------------->8---

They should be added declaratively in the guix-maintenance repo to avoid
loosing them.  Would you like to give it a try?

[...]

>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index b50feed4c4..a3754b7019 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -5060,6 +5060,20 @@ Invoking guix time-machine
>>  large number of packages; the result is cached though and subsequent
>>  commands targeting the same commit are almost instantaneous.
>>
>> +Due to @command{guix time-machine} relying on the ``inferiors''
>> +mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
>> +commit @samp{6298c3ff} (``v1.0.0''), dated May 1@sup{st}, 2019, which is
>> +the first release that included the inferiors mechanism.  An error is
>> +returned when attempting to navigate to older commits.
>
> There is also some issue with bootstrapping depending on your hardware.
>
> About time-bomb, there are also gnutls and openssl or libgit2.  It was
> probably transparent for you because there are substitutable, I guess.
> While Python 2 had probably been removed for some reasons.
>
> Well, I would move the workaround to some dedicated block and move this
> comment after the note about security
>
> @quotation Note
> The history of Guix is immutable and @command{guix time-machine}
> provides the exact same software as they are in a specific Guix
> revision.  Naturally, no security fixes are provided for old versions
> of Guix or its channels.  A careless use of @command{guix time-machine}
> opens the door to security vulnerabilities.  @xref{Invoking guix pull,
> @option{--allow-downgrades}}.
> @end quotation
>
> +Due to @command{guix time-machine} relying on the ``inferiors''
> +mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
> +commit @samp{6298c3ff} (``v1.0.0''), dated May 1@sup{st}, 2019, which is
> +the first release that included the inferiors mechanism.  An error is
> +returned when attempting to navigate to older commits.
> +
> +@quotation Note
> +Although it should technically be possible to travel to such an old
> +revision, the ease to do so will largely depend on the availability of
> +binary substitutes.  When traveling to a distant past, some packages may
> +not easily build from source anymore.  One such example are old versions
> +of Python 2 which had time bombs in its test suite, in the form of
> +expiring SSL certificates.  This particular problem can be worked around
> +by setting the hardware clock to a value in the past before attempting
> +the build.
> +@end quotation

Good suggestion, done!

>  The general syntax is:
>
>  @example
>
>
>
>> new file mode 100644
>> index 0000000000..8b62ef75ea
>> --- /dev/null
>> +++ b/tests/guix-time-machine.sh
>
> [...]
>
>> +# Visiting a commit older than v1.0.0 fails.
>> +! guix time-machine --commit=v0.15.0
>
> Cool to add test.  But this test needs a network access:
>
> $ ./pre-inst-env guix time-machine --commit=v0.15.0 -- describe
> guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
>
> It’s as I said elsewhere. :-)  Well, I have not investigated more but I
> guess a bug in some Git manipulation.

From my testing, the test doesn't require networking, perhaps because it
doesn't use the --commit or --branch arguments.  I've tested that with
this in /etc/hosts, with the first IP being bogus:

--8<---------------cut here---------------start------------->8---
192.168.254.254         git.savannah.gnu.org    savannah
--8<---------------cut here---------------end--------------->8---

I've investigated a bit, and it seems that reaching to the network is
only done when using tags or branch names, not exact commits.  That is
expected I think, since tags or branch names are not immutable in Git,
while a commit ID is.

I've now installed this change; thanks for the review and suggestions!

--
Thanks,
Maxim