| Message ID | c5156f9a756c2a6a304dc789c00abf533c787fd8.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 Tournier <zimon.toutoune@gmail.com> skribis: > * guix/git/scm (reference-available?): Rely of the procedure resolve-reference > to determine if the reference belongs to the local Git checkout. LGTM too, thanks! Ludo’.
Hi again, Simon Tournier <zimon.toutoune@gmail.com> skribis: > * guix/git/scm (reference-available?): Rely of the procedure resolve-reference > to determine if the reference belongs to the local Git checkout. > --- > guix/git.scm | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/guix/git.scm b/guix/git.scm > index dbc3b7caa7..ebe2600209 100644 > --- a/guix/git.scm > +++ b/guix/git.scm > @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp) > (define (reference-available? repository ref) > "Return true if REF, a reference such as '(commit . \"cabba9e\"), is > definitely available in REPOSITORY, false otherwise." > - (match ref > - ((or ('commit . commit) > - ('tag-or-commit . (? commit-id? commit))) > - (let ((len (string-length commit)) > - (oid (string->oid commit))) > - (false-if-git-not-found > - (->bool (if (< len 40) > - (object-lookup-prefix repository oid len OBJ-COMMIT) > - (commit-lookup repository oid)))))) > - (_ > - #f))) > + (false-if-git-not-found > + (->bool (resolve-reference repository ref)))) > > (define (clone-from-swh url tag-or-commit output) > "Attempt to clone TAG-OR-COMMIT (a string), which originates from URL, using > > base-commit: 1b2d43fe016848ea2ec16ff18cbc14340944fc4e In fact, now I recall why that procedure was written that way: it’s meant to say whether a given commit (and only a commit) is already in the checkout, meaning we don’t need to pull. By definition, it’s an answer that can only be given for a specific commit; we cannot tell whether “master” or “HEAD” is available, that wouldn’t make sense. Thus, I think we need to revert a789dd58656d5f7f1b8edf790d77753fc71670af, and probably add a comment explaining why it’s written this way. Thoughts? Ludo’.
Hi, On Mon, 04 Sep 2023 at 11:32, Ludovic Courtès <ludo@gnu.org> wrote: > In fact, now I recall why that procedure was written that way: it’s > meant to say whether a given commit (and only a commit) is already in > the checkout, meaning we don’t need to pull. By definition, it’s an > answer that can only be given for a specific commit; we cannot tell > whether “master” or “HEAD” is available, that wouldn’t make sense. Yeah, that’s the job of ’reference-available?’ to say if a given reference is or not in the repository, IMHO. The patch I proposed earlier fixes the issue you reported, I guess. When debugging, I have noticed that this update-cached-checkout is called many times. For instance, 79ec651a286c71a3d4c72be33a1f80e76a560031 introduced a call. Investigating, I notice that this new procedure is incorrect: --8<---------------cut here---------------start------------->8--- (define (validate-guix-channel channels) "Finds the Guix channel among CHANNELS, and validates that REF as captured from the closure, a git reference specification such as a commit hash or tag associated to CHANNEL, is valid and new enough to satisfy the 'guix time-machine' requirements. A `formatted-message' condition is raised otherwise." (let* ((guix-channel (find guix-channel? channels)) (checkout commit relation (update-cached-checkout (channel-url guix-channel) #:ref (or ref '()) #:starting-commit %oldest-possible-commit))) --8<---------------cut here---------------end--------------->8--- Here, the symbol ’ref’ is bound by: (ref (assoc-ref opts 'ref)) which comes from: (option '("commit") #t #f (lambda (opt name arg result) (alist-cons 'ref `(tag-or-commit . ,arg) result))) (option '("branch") #t #f (lambda (opt name arg result) (alist-cons 'ref `(branch . ,arg) result))) Therefore, it means that when none of the options --commit= or --branch= is provided by the user at the CLI, this ’ref’ is bounded to #false. Therefore, it can lead to unexpected behaviour when providing a channels.scm file. Well, instead, the correct something like: (let* ((guix-channel (find guix-channel? channels)) (reference (or ref (match (channel-commit guix-channel) (#f `(branch . ,(channel-branch guix-channel))) (commit `(tag-or-commit . ,commit))))) (checkout commit relation (update-cached-checkout (channel-url guix-channel) #:ref reference #:starting-commit %oldest-possible-commit))) which works using my tests (with or without network). The remaining issue is the order when displaying messages. This ’validate-guix-channel’ happens before “Updating channel 'guix'” therefore the progress bar appears before etc. I have not investigated how to improve cached-channel-instance. Let me know if the current tiny fix tweaking resolve-interface is enough for now waiting some rework of validate-guix-channel. :-) Cheers, simon
Hi, Simon Tournier <zimon.toutoune@gmail.com> writes: > * guix/git/scm (reference-available?): Rely of the procedure resolve-reference > to determine if the reference belongs to the local Git checkout. > --- > guix/git.scm | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/guix/git.scm b/guix/git.scm > index dbc3b7caa7..ebe2600209 100644 > --- a/guix/git.scm > +++ b/guix/git.scm > @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp) > (define (reference-available? repository ref) > "Return true if REF, a reference such as '(commit . \"cabba9e\"), is > definitely available in REPOSITORY, false otherwise." > - (match ref > - ((or ('commit . commit) > - ('tag-or-commit . (? commit-id? commit))) > - (let ((len (string-length commit)) > - (oid (string->oid commit))) > - (false-if-git-not-found > - (->bool (if (< len 40) > - (object-lookup-prefix repository oid len OBJ-COMMIT) > - (commit-lookup repository oid)))))) > - (_ > - #f))) > + (false-if-git-not-found > + (->bool (resolve-reference repository ref)))) This was applied some time ago as a789dd58656d5f7f1b8edf790d77753fc71670af. Closing.
Hi Maxim, On Tue, 5 Sept 2023 at 15:24, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > This was applied some time ago as > a789dd58656d5f7f1b8edf790d77753fc71670af. Thanks for having applied it. > Closing. However, I do not think it should be closed. Maybe you have missed: [bug#65352] Fix time-machine and network Ludovic Courtès <ludo@gnu.org> Mon, 04 Sep 2023 10:49:24 +0200 id:87wmx6qq5n.fsf_-_@gnu.org https://issues.guix.gnu.org//65352 https://issues.guix.gnu.org/msgid/87wmx6qq5n.fsf_-_@gnu.org https://yhetil.org/guix/87wmx6qq5n.fsf_-_@gnu.org And this thread contains some fixes. Well, from my point of view, the discussion is still pending... Cheers, simon
Hi, Ludovic Courtès <ludo@gnu.org> writes: > Hi again, > > Simon Tournier <zimon.toutoune@gmail.com> skribis: > >> * guix/git/scm (reference-available?): Rely of the procedure resolve-reference >> to determine if the reference belongs to the local Git checkout. >> --- >> guix/git.scm | 13 ++----------- >> 1 file changed, 2 insertions(+), 11 deletions(-) >> >> diff --git a/guix/git.scm b/guix/git.scm >> index dbc3b7caa7..ebe2600209 100644 >> --- a/guix/git.scm >> +++ b/guix/git.scm >> @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp) >> (define (reference-available? repository ref) >> "Return true if REF, a reference such as '(commit . \"cabba9e\"), is >> definitely available in REPOSITORY, false otherwise." >> - (match ref >> - ((or ('commit . commit) >> - ('tag-or-commit . (? commit-id? commit))) >> - (let ((len (string-length commit)) >> - (oid (string->oid commit))) >> - (false-if-git-not-found >> - (->bool (if (< len 40) >> - (object-lookup-prefix repository oid len OBJ-COMMIT) >> - (commit-lookup repository oid)))))) >> - (_ >> - #f))) >> + (false-if-git-not-found >> + (->bool (resolve-reference repository ref)))) >> >> (define (clone-from-swh url tag-or-commit output) >> "Attempt to clone TAG-OR-COMMIT (a string), which originates from URL, using >> >> base-commit: 1b2d43fe016848ea2ec16ff18cbc14340944fc4e > > In fact, now I recall why that procedure was written that way: it’s > meant to say whether a given commit (and only a commit) is already in > the checkout, meaning we don’t need to pull. By definition, it’s an > answer that can only be given for a specific commit; we cannot tell > whether “master” or “HEAD” is available, that wouldn’t make sense. > > Thus, I think we need to revert > a789dd58656d5f7f1b8edf790d77753fc71670af, and probably add a comment > explaining why it’s written this way. > > Thoughts? I've reviewed this thread and the code, and I agree. This is a special case. I've added a comment so we aren't tempted to use 'resolve-reference' there again. Will install shortly.
Hi Maxim, On Tue, 05 Sep 2023 at 16:39, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > I've reviewed this thread and the code, and I agree. This is a special > case. I've added a comment so we aren't tempted to use > 'resolve-reference' there again. I disagree. There is no special case. The culprit is the procedure ’validate-guix-channel’ as explained in: [bug#65352] Fix time-machine and network Simon Tournier <zimon.toutoune@gmail.com> Mon, 04 Sep 2023 19:37:08 +0200 id:87wmx5on5n.fsf@gmail.com https://issues.guix.gnu.org//65352 https://issues.guix.gnu.org/msgid/87wmx5on5n.fsf@gmail.com https://yhetil.org/guix/87wmx5on5n.fsf@gmail.com > Will install shortly. I do not know what you will install shortly. The fix belong to validate-guix-channel, something like: (let* ((guix-channel (find guix-channel? channels)) (reference (or ref (match (channel-commit guix-channel) (#f `(branch . ,(channel-branch guix-channel))) (commit `(tag-or-commit . ,commit))))) (checkout commit relation (update-cached-checkout (channel-url guix-channel) #:ref reference #:starting-commit %oldest-possible-commit))) and that would avoid to break the “contract” of resolve-reference. Before committing something, I was testing. Cheers, simon
Hi Simon, Simon Tournier <zimon.toutoune@gmail.com> writes: > Hi Maxim, > > On Tue, 5 Sept 2023 at 15:24, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > >> This was applied some time ago as >> a789dd58656d5f7f1b8edf790d77753fc71670af. > > Thanks for having applied it. > >> Closing. > > However, I do not think it should be closed. Maybe you have missed: > > [bug#65352] Fix time-machine and network > Ludovic Courtès <ludo@gnu.org> > Mon, 04 Sep 2023 10:49:24 +0200 > id:87wmx6qq5n.fsf_-_@gnu.org > https://issues.guix.gnu.org//65352 > https://issues.guix.gnu.org/msgid/87wmx6qq5n.fsf_-_@gnu.org > https://yhetil.org/guix/87wmx6qq5n.fsf_-_@gnu.org > > And this thread contains some fixes. Well, from my point of view, the > discussion is still pending... 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.
Hi Simon, Simon Tournier <zimon.toutoune@gmail.com> writes: [...] > Well, instead, the correct something like: > > (let* ((guix-channel (find guix-channel? channels)) > (reference (or ref > (match (channel-commit guix-channel) > (#f `(branch . ,(channel-branch guix-channel))) > (commit `(tag-or-commit . ,commit))))) > (checkout commit relation (update-cached-checkout > (channel-url guix-channel) > #:ref reference > #:starting-commit > %oldest-possible-commit))) > > which works using my tests (with or without network). I've installed something along this with c3d48d0. If there are other issues, I think it'd be best if they are described clearly in a new issue, as that one is getting crowded :-).
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
reopen 65352 quit Hi Simon, Simon Tournier <zimon.toutoune@gmail.com> writes: > 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. OK, thank you. It's a bit hard to keep track of multiple issues and their resolutions in a longish thread. > 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. I don't know if we want to consider tags are immutable or not; the safest is to consider them an *not* immutable, which is what we had been doing. I agree it doesn't cover all the potential git refspecs; we can get there if we want (although I suppose it's uncommon for someone to try 'guix time-machine --commit=v1.3.0-47405-ge0767a24d0' or similar). > It is similar with short commit hash as "guix time-machine > --commit=4a027d2". That's what I reported. I'm not sure if short commit IDs should be treated as immutable, since in theory they can collide; the safest would be to check if there are collisions and report an error if there is; and this requires fetching new objects first. So, what is the behavior that we want?
Hi, Simon Tournier <zimon.toutoune@gmail.com> writes: > Hi Maxim, > > On Tue, 05 Sep 2023 at 16:39, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > >> I've reviewed this thread and the code, and I agree. This is a special >> case. I've added a comment so we aren't tempted to use >> 'resolve-reference' there again. > > I disagree. There is no special case. The culprit is the procedure > ’validate-guix-channel’ as explained in: I was referring to the special case of resolved-reference? (that it mustn't trust tags or branches in a git cache -- at least currently, compared to resolve-reference. Maybe we want to change that? > [bug#65352] Fix time-machine and network > Simon Tournier <zimon.toutoune@gmail.com> > Mon, 04 Sep 2023 19:37:08 +0200 > id:87wmx5on5n.fsf@gmail.com > https://issues.guix.gnu.org//65352 > https://issues.guix.gnu.org/msgid/87wmx5on5n.fsf@gmail.com > https://yhetil.org/guix/87wmx5on5n.fsf@gmail.com > > >> Will install shortly. > > I do not know what you will install shortly. The fix belong to > validate-guix-channel, something like: > > (let* ((guix-channel (find guix-channel? channels)) > (reference (or ref > (match (channel-commit guix-channel) > (#f `(branch . ,(channel-branch guix-channel))) > (commit `(tag-or-commit . ,commit))))) > (checkout commit relation (update-cached-checkout > (channel-url guix-channel) > #:ref reference > #:starting-commit > %oldest-possible-commit))) > > and that would avoid to break the “contract” of resolve-reference. > Before committing something, I was testing. That's orthogonal to the other issue discussed, right? What I was referring to about 'installing' was c3d48d02, which implements the above (with let-bound variables and 'if' instead of match, but the logic is the same). I feel like we need to agree on what reference-available? is supposed to achieve, and where it needs to differentiate from resolve-reference.
Hi, On Tue, 05 Sep 2023 at 22:00, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: >> Anyway, I will open a >> report for that order issue. > > OK, thank you. It's a bit hard to keep track of multiple issues and > their resolutions in a longish thread. For cross-referencing, done with bug#65788, bug#65788: poor information when updating using “guix time-machine” Simon Tournier <zimon.toutoune@gmail.com> Wed, 06 Sep 2023 18:57:38 +0200 id:87pm2vme7x.fsf@gmail.com https://yhetil.org/guix/87pm2vme7x.fsf@gmail.com https://issues.guix.gnu.org/msgid/87pm2vme7x.fsf@gmail.com Cheers, simon
diff --git a/guix/git.scm b/guix/git.scm index dbc3b7caa7..ebe2600209 100644 --- a/guix/git.scm +++ b/guix/git.scm @@ -360,17 +360,8 @@ (define-syntax-rule (false-if-git-not-found exp) (define (reference-available? repository ref) "Return true if REF, a reference such as '(commit . \"cabba9e\"), is definitely available in REPOSITORY, false otherwise." - (match ref - ((or ('commit . commit) - ('tag-or-commit . (? commit-id? commit))) - (let ((len (string-length commit)) - (oid (string->oid commit))) - (false-if-git-not-found - (->bool (if (< len 40) - (object-lookup-prefix repository oid len OBJ-COMMIT) - (commit-lookup repository oid)))))) - (_ - #f))) + (false-if-git-not-found + (->bool (resolve-reference repository ref)))) (define (clone-from-swh url tag-or-commit output) "Attempt to clone TAG-OR-COMMIT (a string), which originates from URL, using