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

For compatibility with (guix git) procedures.

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

New commit.

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

Hi Maxim,

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

I am not sure to understand these both changes.

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

Well, why not. :-)

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

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

Cheers,
simon
  

Hi Simon,

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

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

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

Hi Maxim,

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

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

[...]

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

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

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

Cheers,
simon
  

Hi Simon,

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

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

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

Re Maxim,

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

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

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

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

with the previous commit 79ec651a286c71a3d4c72be33a1f80e76a560031.

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

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

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

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

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

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

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

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

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


Cheers,
simon
  

Hi again,

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

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

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

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

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

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

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

I hope this clarifies it?
  

Hi Maxim,

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

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

[...]

> I hope this clarifies it?

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

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

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

Cheers,
simon