[bug#65352] Fix time-machine and network

Hi Ludo,

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

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

No, or I am missing some details.

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

It’s already the case because of that:

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

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

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

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

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

Before the proposed patch:

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

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

;;; (reference-available? #f)

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

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

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

;;; (reference-available? #f)

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

After the proposed patch:

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

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

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

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

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

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


Cheers,
simon

--8<---------------cut here---------------start------------->8---
--8<---------------cut here---------------end--------------->8---
  

Hi,

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

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

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

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

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

Thanks,
Ludo’.
  

Hi again,

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

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

Yes this seems to confirm what I thought.

So anyway, go for it!

Great that you’re improving performance here.

Ludo’.
  

Hi,

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

> Yes this seems to confirm what I thought.

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

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

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

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

Hence my previous answer. :-)

> So anyway, go for it!

Cool!  I will proceed.

> Great that you’re improving performance here.

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

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

Cheers,
simon
  

Hi,

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

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

Done with 6d33c1f8061e86d63ab5c9ec75df9c58130c7264.

Cheers,
simon
  

Hi,

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

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

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

There are two things we missed here:

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

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

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

Thanks,
Ludo’.
  

Hi,

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

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

About "guix system"?

Yes, for sure let reintroduce it.

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

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

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

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

Cheers,
simon
  

Re

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

Done with 94f3831e5bb1e04eeb3a0e7d31a0675208ce6f4c.

Cheers,
simon
  

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

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

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

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

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

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

Thanks for your quick response!

Ludo’.
  

Hi,

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

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

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

The story starts with 79ec651a286c71a3d4c72be33a1f80e76a560031 and
ecab937897385fce3e3ce0c5f128afba4304187c:

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

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

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

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

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

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

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

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

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

Cheers,
simon