[bug#65352] Fix time-machine and network

[Simon Tournier <zimon.toutoune@gmail.com>]

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