| Message ID | 444e7e32d49b56ba6cb0a132e97d63560d8de437.1692281315.git.zimon.toutoune@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#65352,1/2] guix: git: Fix the procedure reference-available?. | expand |
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!
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)