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

* 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(-)


base-commit: 1b2d43fe016848ea2ec16ff18cbc14340944fc4e
  

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’.
  

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’.
  

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
  

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.
  

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
  

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.
  

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
  

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.
  

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 :-).
  

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
  

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?
  

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.
  

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