| Message ID | 86a5tqzze6.fsf@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#65352] Fix time-machine and network | expand |
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
diff --git a/guix/git.scm b/guix/git.scm index 1cb87a45607b..c927555cce18 100644 --- a/guix/git.scm +++ b/guix/git.scm @@ -481,6 +481,8 @@ (define* (update-cached-checkout url (repository-open cache-directory) (clone/swh-fallback url ref cache-directory)))) ;; Only fetch remote if it has not been cloned just before. + (pk 'ref ref) + (pk 'reference-available? (reference-available? repository ref)) (when (and cache-exists? (not (reference-available? repository ref))) (remote-fetch (remote-lookup repository "origin")