[bug#65352,2/2] scripts: pull: Remove unused reference pair.

* guix/scripts/pull.scm (channel-list): Remove commit pair reference
specification.
---
 guix/scripts/pull.scm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

Hi Simon,

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?

Otherwise, the series LGTM, thank you!
  

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.

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
  

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.

Thanks,
Ludo’.
  

Hi,

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

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

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.

Thanks,
Ludo’.
  

Hi,

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

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

Hi Simon,

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

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

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

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

Cheers,
simon


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

Hi Simon,

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

> 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!