Fix time-machine and network

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxim Cournoyer
  • Simon Tournier
Owner
unassigned
Submitted by
Simon Tournier
Severity
normal
S
S
Simon Tournier wrote on 17 Aug 2023 16:06
(address . guix-patches@gnu.org)
87fs4h4vb9.fsf@gmail.com
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:

Toggle quote (34 lines)
> 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.

Toggle snippet (14 lines)
(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)

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.



Let me know if I am not missing something.


Cheers,
simon
S
S
Simon Tournier wrote on 17 Aug 2023 16:09
[PATCH 1/2] guix: git: Fix the procedure reference-available?.
(address . 65352@debbugs.gnu.org)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
c5156f9a756c2a6a304dc789c00abf533c787fd8.1692281315.git.zimon.toutoune@gmail.com
* 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(-)

Toggle diff (28 lines)
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
S
S
Simon Tournier wrote on 17 Aug 2023 16:09
[PATCH 2/2] scripts: pull: Remove unused reference pair.
(address . 65352@debbugs.gnu.org)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
444e7e32d49b56ba6cb0a132e97d63560d8de437.1692281315.git.zimon.toutoune@gmail.com
* guix/scripts/pull.scm (channel-list): Remove commit pair reference
specification.
---
guix/scripts/pull.scm | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

Toggle diff (16 lines)
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
M
M
Maxim Cournoyer wrote on 17 Aug 2023 17:41
Re: bug#65352: Fix time-machine and network
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352@debbugs.gnu.org)
87o7j5fzg8.fsf_-_@gmail.com
Hi Simon,

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

Toggle quote (21 lines)
> * 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
S
S
Simon Tournier wrote on 17 Aug 2023 18:08
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 65352@debbugs.gnu.org)
CAJ3okZ0NHQ0z1-+VB+bONKL2y85rb00dc9zjj98MeC9o9C86TQ@mail.gmail.com
Hi Maxim,

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

Toggle quote (8 lines)
> > (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
L
L
Ludovic Courtès wrote on 21 Aug 2023 15:57
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352@debbugs.gnu.org)
87350cbirk.fsf_-_@gnu.org
Hi!

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

Toggle quote (3 lines)
> * 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’.
L
L
Ludovic Courtès wrote on 21 Aug 2023 16:00
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87y1i4a42h.fsf_-_@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (26 lines)
> 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’.
M
M
Maxim Cournoyer wrote on 21 Aug 2023 17:58
(name . Ludovic Courtès)(address . ludo@gnu.org)
87350ce6b4.fsf@gmail.com
Hi,

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

Toggle quote (31 lines)
> 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
L
L
Ludovic Courtès wrote on 22 Aug 2023 18:27
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87fs4b3uvg.fsf@gnu.org
Hi,

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

Toggle quote (2 lines)
> Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (10 lines)
>>> 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’.
M
M
Maxim Cournoyer wrote on 23 Aug 2023 04:14
(name . Ludovic Courtès)(address . ludo@gnu.org)
87350abj3d.fsf@gmail.com
Hi,

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

Toggle quote (24 lines)
> 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
M
M
Maxim Cournoyer wrote on 23 Aug 2023 04:56
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352@debbugs.gnu.org)
87ttsqa2km.fsf@gmail.com
Hi Simon,

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

Toggle quote (20 lines)
> 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.

Toggle quote (32 lines)
> 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
S
S
Simon Tournier wrote on 23 Aug 2023 10:32
Re: [bug#65352] Fix time-machine and network
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 65352@debbugs.gnu.org)
86jztm40r2.fsf@gmail.com
Hi Maxim,

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

Toggle quote (19 lines)
>> 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


M
M
Maxim Cournoyer wrote on 23 Aug 2023 22:25
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352@debbugs.gnu.org)
87lee1a4lp.fsf@gmail.com
Hi Simon,

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

Toggle quote (28 lines)
> 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
L
L
Ludovic Courtès wrote on 4 Sep 2023 10:49
Re: bug#65352: Fix time-machine and network
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352@debbugs.gnu.org)
87wmx6qq5n.fsf_-_@gnu.org
Hi,

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

Toggle quote (28 lines)
> * 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:

Toggle snippet (44 lines)
$ 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

I’m testing the fix below:
Toggle diff (24 lines)
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
Ludo’.
L
L
Ludovic Courtès wrote on 4 Sep 2023 11:32
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352@debbugs.gnu.org)
87o7iiqo6d.fsf_-_@gnu.org
Hi again,

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

Toggle quote (33 lines)
> * 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’.
S
S
Simon Tournier wrote on 4 Sep 2023 13:34
Re: [bug#65352] Fix time-machine and network
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 65352@debbugs.gnu.org)
87edjep3xq.fsf@gmail.com
Hi Ludo,

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

Toggle quote (2 lines)
> Houston, we have a problem:

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


Toggle quote (9 lines)
> 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
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(-)

Toggle diff (18 lines)
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
S
S
Simon Tournier wrote on 4 Sep 2023 19:37
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 65352@debbugs.gnu.org)
87wmx5on5n.fsf@gmail.com
Hi,

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

Toggle quote (6 lines)
> 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:

Toggle snippet (14 lines)
(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)))

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
M
M
Maxim Cournoyer wrote on 5 Sep 2023 15:24
Re: bug#65352: Fix time-machine and network
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352-done@debbugs.gnu.org)
87fs3slplq.fsf_-_@gmail.com
Hi,

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

Toggle quote (28 lines)
> * 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
Closed
S
S
Simon Tournier wrote on 5 Sep 2023 15:43
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 65352-done@debbugs.gnu.org)
CAJ3okZ3xkUMZnGvUcM8WSxDu0dkOZ0Cfp+qtauYJc7wTsR2xyQ@mail.gmail.com
Hi Maxim,

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

Toggle quote (3 lines)
> This was applied some time ago as
> a789dd58656d5f7f1b8edf790d77753fc71670af.

Thanks for having applied it.

Toggle quote (2 lines)
> 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

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

Cheers,
simon
Closed
M
M
Maxim Cournoyer wrote on 5 Sep 2023 22:33
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
8734zsgy2k.fsf_-_@gmail.com
Hi Simon,

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

[...]

Toggle quote (29 lines)
>>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
M
M
Maxim Cournoyer wrote on 5 Sep 2023 22:39
(name . Ludovic Courtès)(address . ludo@gnu.org)
87y1hkfj7j.fsf_-_@gmail.com
Hi,

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

Toggle quote (49 lines)
> 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
S
S
Simon Tournier wrote on 5 Sep 2023 22:48
Re: [bug#65352] Fix time-machine and network
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
86a5u0qrbd.fsf@gmail.com
Hi Maxim,

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

Toggle quote (8 lines)
>> - 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

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

Cheers,
simon
S
S
Simon Tournier wrote on 5 Sep 2023 22:56
(address . 65352@debbugs.gnu.org)
864jk8qqz8.fsf@gmail.com
Hi Maxim,

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

Toggle quote (4 lines)
> 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


Toggle quote (2 lines)
> 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
M
M
Maxim Cournoyer wrote on 6 Sep 2023 02:04
Re: bug#65352: Fix time-machine and network
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 65352-done@debbugs.gnu.org)
87tts8f9p7.fsf@gmail.com
Hi Simon,

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

Toggle quote (24 lines)
> 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
Closed
M
M
Maxim Cournoyer wrote on 6 Sep 2023 02:22
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87pm2wf8wd.fsf_-_@gmail.com
Hi Simon,

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

[...]

Toggle quote (15 lines)
> 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
Closed
S
S
Simon Tournier wrote on 6 Sep 2023 02:58
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 65352-done@debbugs.gnu.org)
CAJ3okZ17LnExcT32HwW1i86tS7fUD3WJqUURwMi=X3AXZ-1YZA@mail.gmail.com
Hi Maxim,

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

Toggle quote (4 lines)
> 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.


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
Closed
M
M
Maxim Cournoyer wrote on 6 Sep 2023 04:00
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
878r9kdpsk.fsf@gmail.com
reopen 65352
quit

Hi Simon,

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

Toggle quote (17 lines)
> 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.

Toggle quote (10 lines)
> 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).

Toggle quote (3 lines)
> 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
M
M
Maxim Cournoyer wrote on 6 Sep 2023 04:39
Re: [bug#65352] Fix time-machine and network
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
874jk8dnzm.fsf@gmail.com
Hi,

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

Toggle quote (11 lines)
> 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?

Toggle quote (28 lines)
> [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
S
S
Simon Tournier wrote on 6 Sep 2023 12:32
[bug#65352] time-machine, unavailable network or Savannah down
(name . Ludovic Courtès)(address . ludo@gnu.org)
86ledjoaly.fsf@gmail.com
Hi Maxim,

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

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

Toggle snippet (23 lines)
$ 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,

Toggle snippet (4 lines)
$ 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?

It is the same when specifying a release tag,

Toggle snippet (4 lines)
$ 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

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:

Toggle quote (6 lines)
> 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).

[...]

Toggle quote (5 lines)
> 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
S
S
Simon Tournier wrote on 6 Sep 2023 16:17
[PATCH v2] DRAFT git: Avoid touching the network unless needed in 'reference-available?'.
(name . Ludovic Courtès)(address . ludo@gnu.org)
32d3fb5066e0b20e200dabef0fba897634e21dda.1694009405.git.zimon.toutoune@gmail.com
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


Toggle diff (46 lines)
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
M
M
Maxim Cournoyer wrote on 6 Sep 2023 19:41
Re: [bug#65352] time-machine, unavailable network or Savannah down
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
871qfb5hcq.fsf@gmail.com
Hi Simon, Ludovic,

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

Toggle quote (4 lines)
> Hi Maxim,
>
> Let start another branch in that thread of #65352. :-)

Alright :-).

Toggle quote (34 lines)
> 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.

[...]

Toggle quote (22 lines)
> 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.

Toggle quote (6 lines)
> 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!

Toggle quote (18 lines)
> 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 ;-).

Toggle quote (13 lines)
>
> 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
S
S
Simon Tournier wrote on 7 Sep 2023 01:21
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
86a5tyopkl.fsf@gmail.com
Hi,

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

Toggle quote (3 lines)
> 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


Toggle quote (5 lines)
> 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
S
S
Simon Tournier wrote on 7 Sep 2023 13:15
Re: [bug#65352] Fix time-machine and network
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87y1hikzef.fsf@gmail.com
Hi,

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

Toggle quote (6 lines)
>> 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

Cheers,
simon
L
L
Ludovic Courtès wrote on 13 Sep 2023 22:16
Re: bug#65352: Fix time-machine and network
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
875y4dltgm.fsf_-_@gnu.org
Hi,

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

Toggle quote (5 lines)
> 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.

[...]

Toggle quote (25 lines)
> (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.

Toggle quote (3 lines)
> (_
> + ;; 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’.
S
S
Simon Tournier wrote on 13 Sep 2023 02:32
Re: [bug#65352] Fix time-machine and network
(name . Ludovic Courtès)(address . ludo@gnu.org)
86a5tqzze6.fsf@gmail.com
Hi Ludo,

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

Toggle quote (18 lines)
>> + (('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.

Toggle quote (2 lines)
> 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:

Toggle snippet (19 lines)
$ ./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

After the proposed patch:

Toggle snippet (20 lines)
$ ./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


Cheers,
simon

Toggle snippet (14 lines)
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")
L
L
Ludovic Courtès wrote on 14 Sep 2023 10:50
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
874jjxi1e5.fsf@gnu.org
Hi,

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

Toggle quote (22 lines)
> 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?


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

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 14 Sep 2023 11:04
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87zg1pgm7v.fsf@gnu.org
Hi again,

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

Toggle quote (22 lines)
> 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’.
S
S
Simon Tournier wrote on 14 Sep 2023 11:42
(name . Ludovic Courtès)(address . ludo@gnu.org)
CAJ3okZ1ccutZ+jjW9Fzk5HC=fm7HEj5D9RSbhxMZA-2OUrO2BA@mail.gmail.com
Hi,

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

Toggle quote (2 lines)
> 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. :-)

Toggle quote (2 lines)
> So anyway, go for it!

Cool! I will proceed.

Toggle quote (2 lines)
> 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

Cheers,
simon
S
S
Simon Tournier wrote on 22 Sep 2023 15:54
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ttrm8gar.fsf@gmail.com
Hi,

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

Toggle quote (4 lines)
>> So anyway, go for it!
>
> Cool! I will proceed.

Done with 6d33c1f8061e86d63ab5c9ec75df9c58130c7264.

Cheers,
simon
Closed
L
L
Ludovic Courtès wrote on 25 Sep 2023 11:32
Re: bug#65352: Fix time-machine and network
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87pm267g4h.fsf_-_@gnu.org
Hi,

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

Toggle quote (4 lines)
> 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’.
S
S
Simon Tournier wrote on 25 Sep 2023 11:57
(name . Ludovic Courtès)(address . ludo@gnu.org)
CAJ3okZ3KZmYrCePBOCGXCooa1J6o0dT5yxiTJ61t83_wJ4nLjA@mail.gmail.com
Hi,

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

Toggle quote (4 lines)
> 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'.

Toggle quote (4 lines)
> 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
S
S
Simon Tournier wrote on 25 Sep 2023 13:21
(name . Ludovic Courtès)(address . ludo@gnu.org)
CAJ3okZ1kfQ5iMrQmSYKC5LGEa5vjE5918AOZBCcPwL3v9zzojQ@mail.gmail.com
Re

On Mon, 25 Sept 2023 at 11:57, Simon Tournier <zimon.toutoune@gmail.com> wrote:
Toggle quote (5 lines)
> 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?
[...]
Toggle quote (2 lines)
> Yes, for sure let reintroduce it.

Done with 94f3831e5bb1e04eeb3a0e7d31a0675208ce6f4c.

Cheers,
simon
L
L
Ludovic Courtès wrote on 25 Sep 2023 17:01
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
877coe5mbq.fsf@gnu.org
Simon Tournier <zimon.toutoune@gmail.com> skribis:

Toggle quote (13 lines)
> 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.

Toggle quote (13 lines)
>> 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’.
S
S
Simon Tournier wrote on 25 Sep 2023 17:58
Re: [bug#65352] Fix time-machine and network
(name . Ludovic Courtès)(address . ludo@gnu.org)
87il7y454i.fsf@gmail.com
Hi,

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

Toggle quote (19 lines)
>>> 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
?
Your comment

This issue is archived.

To comment on this conversation send an email to 65352@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 65352
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch