| Message ID | 20211217000235.68996-1-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only | expand |
Glen Choo <chooglen@google.com> writes: > cmd_fetch() performs certain tasks with the assumption that objects are > fetched, but `git fetch --negotiate-only` does not fetch objects, as its > name implies. This is results in behavior that is unnecessary at best, > and incorrect at worst: > > * Submodules are updated if enabled by recurse.submodules=true, but > negotiation fetch doesn't actually update the repo, so this doesn't > make sense (introduced in [1]). Hmph. > * Commit graphs will be written if enabled by > fetch.writeCommitGraph=true. But according to > Documentation/config/fetch.txt [2], this should only be done if a > pack-file is downloaded. Makes sense, as we haven't changed any reachability, and we have no need to rewrite the graph file. > * gc is run, but according to [3], we only do this because we expect > `git fetch` to introduce objects. Makes sense. As we haven't added any new objects, there is nothing (other than the passage of time) that adds to the need to collect garbage. It makes me wonder if we need to do anything upon "fetch --dry-run". I know we add to the object store without making anything reachable, so that the user can do pre-flight checks with the real objects. We do not change the reachability so there is no reason to rewrite the graph file, but we do add cruft to the object store. Doing something about "--dry-run" is obviously outside the scope of this topic, but it may make sense to think about it while we are thinking about "fetch". > diff --git a/builtin/fetch.c b/builtin/fetch.c > index f7abbc31ff..85091af99b 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > > argc = parse_options(argc, argv, prefix, > builtin_fetch_options, builtin_fetch_usage, 0); > + > + if (negotiate_only) { > + /* > + * --negotiate-only should never recurse into > + * submodules, so there is no need to read .gitmodules. > + */ > + recurse_submodules = RECURSE_SUBMODULES_OFF; > + if (!negotiation_tip.nr) > + die(_("--negotiate-only needs one or more --negotiate-tip=*")); > + } > + This means "fetch --negotiate-only --recurse-submodules" silently ignores an explicit wish by the user. I suspect that this part should be more like this. if (negitiate_only && recurse_submodules != RECURSE_SUBMODULES_OFF) { if (recurse_submodules came from the parse_options) die(_("'--%s' cannot be used with '--%s'", "recurse-submodules", "negotiate-only")); recurse_submodules = RECURSE_SUBMODULES_OFF; } That is, we complain if user gives us a combination we do not support, but we are OK if the configuration is set to do so and silently ignore (because we declare that the combination does not make sense). By the way, do not move the check about the number of negotiation tips from the original location. That check, or its location, have nothing to do with what you want to do in this patch, which is "do not gc or update the graph file if we are not fetching". It is better to leave unrelated changes out of the patch. In order to tell if recurse_submodules that is not OFF came from the call to parse_options(), you may need to capture the value of the variable before calling parse_options() and compare it with the current value in the above illustration code snippet I gave. Having said all that, is it true that recurse-submodules should not be combined with negotiate-only? I naively think it would not be surprising if users expect negotiate-only fetches are done also in the submodules. Whatever we decide the right behaviour should be, we should document it. With your patch without any of my above input, I would expect at least something like diff --git i/Documentation/fetch-options.txt w/Documentation/fetch-options.txt index e967ff1874..baf2e9c50d 100644 --- i/Documentation/fetch-options.txt +++ w/Documentation/fetch-options.txt @@ -73,6 +73,9 @@ configuration variables documented in linkgit:git-config[1], and the + Internally this is used to implement the `push.negotiate` option, see linkgit:git-config[1]. ++ +Note that this option silently makes various options that do not make +sense to be used together with it (e.g. `--recurse-submodules`) ignored. --dry-run:: Show what would be done, without making any changes. to leave wiggling room for us to silently ignore more. We may know about --recurse-submodules today, but I would not be surprised if we find more. > @@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > result = fetch_multiple(&list, max_children); > } > > + string_list_clear(&list, 0); > + > + /* > + * Skip irrelevant tasks because we know objects were not > + * fetched. > + * > + * NEEDSWORK: as a future optimization, we can return early > + * whenever objects were not fetched e.g. if we already have all > + * of them. > + */ > + if (negotiate_only) > + return result; > + I find it somewhat misleading to have the early return before the block for recurse_submodules, as we _are_ already forcing it to not to recurse. It would be more readable if it went before the place where we start doing the post-action clean-ups like reachability graphs and garbage collection. > if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { > struct strvec options = STRVEC_INIT; > int max_children = max_jobs; > @@ -2132,8 +2153,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > strvec_clear(&options); > } > > - string_list_clear(&list, 0); > - Namely, here. > prepare_repo_settings(the_repository); This is existing code, but I wonder why it can be done _SO_ late in the sequence. We've already called the transport API for the negotiate-only communication at this point, but a call to this function is the only thing that gives fetch_negotiation_algorithm member in the_repository its default value, isn't it? Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> * gc is run, but according to [3], we only do this because we expect >> `git fetch` to introduce objects. > > Makes sense. As we haven't added any new objects, there is nothing > (other than the passage of time) that adds to the need to collect > garbage. > > It makes me wonder if we need to do anything upon "fetch --dry-run". > I know we add to the object store without making anything reachable, > so that the user can do pre-flight checks with the real objects. We > do not change the reachability so there is no reason to rewrite the > graph file, but we do add cruft to the object store. > > Doing something about "--dry-run" is obviously outside the scope of > this topic, but it may make sense to think about it while we are > thinking about "fetch". I hadn't considered "--dry-run". I'll think about that while I structure this patch. >> diff --git a/builtin/fetch.c b/builtin/fetch.c >> index f7abbc31ff..85091af99b 100644 >> --- a/builtin/fetch.c >> +++ b/builtin/fetch.c >> @@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) >> >> argc = parse_options(argc, argv, prefix, >> builtin_fetch_options, builtin_fetch_usage, 0); >> + >> + if (negotiate_only) { >> + /* >> + * --negotiate-only should never recurse into >> + * submodules, so there is no need to read .gitmodules. >> + */ >> + recurse_submodules = RECURSE_SUBMODULES_OFF; >> + if (!negotiation_tip.nr) >> + die(_("--negotiate-only needs one or more --negotiate-tip=*")); >> + } >> + > > This means "fetch --negotiate-only --recurse-submodules" silently > ignores an explicit wish by the user. > > I suspect that this part should be more like this. > > if (negitiate_only && > recurse_submodules != RECURSE_SUBMODULES_OFF) { > if (recurse_submodules came from the parse_options) > die(_("'--%s' cannot be used with '--%s'", > "recurse-submodules", "negotiate-only")); > recurse_submodules = RECURSE_SUBMODULES_OFF; > } > > That is, we complain if user gives us a combination we do not > support, but we are OK if the configuration is set to do so and > silently ignore (because we declare that the combination does not > make sense). Yes, Jonathan proposed this as well. This is identical to the approach in gc/branch-recurse-submodules, which as I noted in [1], is a bit inconsistent with submodule parsing in general. I decided *against* this because I thought that "--negotiate-only" is internal-only. I don't see why a user would use "--negotiate-only", but if this is a user journey we want to care about, then adding the explicit check sounds ok. > By the way, do not move the check about the number of negotiation > tips from the original location. That check, or its location, have > nothing to do with what you want to do in this patch, which is "do > not gc or update the graph file if we are not fetching". It is > better to leave unrelated changes out of the patch. Ah, I see that it's not easy to tell whether or not the behavior is correct after that line is moved. I'll avoid doing this in the future. I still think that it is cleaner to move the negotiation_tip.nr check. Should I do this in a follow-up patch? > In order to tell if recurse_submodules that is not OFF came from the > call to parse_options(), you may need to capture the value of the > variable before calling parse_options() and compare it with the > current value in the above illustration code snippet I gave. > > Having said all that, is it true that recurse-submodules should not > be combined with negotiate-only? I naively think it would not be > surprising if users expect negotiate-only fetches are done also in > the submodules. In the current form of "--negotiate-only", no it does not make sense for them to be combined. I think users would have the same expectations as you if they were invoking "git fetch --negotiate-only" directly, but I'm not convinced that that they will ever do so, except to debug push negotiation. I hope Jonathan can chime in to confirm whether or not users want/need to invoke "--negotiate-only". > Whatever we decide the right behaviour should be, we should document > it. With your patch without any of my above input, I would expect at > least something like > > diff --git i/Documentation/fetch-options.txt w/Documentation/fetch-options.txt > index e967ff1874..baf2e9c50d 100644 > --- i/Documentation/fetch-options.txt > +++ w/Documentation/fetch-options.txt > @@ -73,6 +73,9 @@ configuration variables documented in linkgit:git-config[1], and the > + > Internally this is used to implement the `push.negotiate` option, see > linkgit:git-config[1]. > ++ > +Note that this option silently makes various options that do not make > +sense to be used together with it (e.g. `--recurse-submodules`) ignored. > > --dry-run:: > Show what would be done, without making any changes. > > to leave wiggling room for us to silently ignore more. We may know > about --recurse-submodules today, but I would not be surprised if we > find more. Sounds good. >> @@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) >> result = fetch_multiple(&list, max_children); >> } >> >> + string_list_clear(&list, 0); >> + >> + /* >> + * Skip irrelevant tasks because we know objects were not >> + * fetched. >> + * >> + * NEEDSWORK: as a future optimization, we can return early >> + * whenever objects were not fetched e.g. if we already have all >> + * of them. >> + */ >> + if (negotiate_only) >> + return result; >> + > > I find it somewhat misleading to have the early return before the > block for recurse_submodules, as we _are_ already forcing it to not > to recurse. It would be more readable if it went before the place > where we start doing the post-action clean-ups like reachability > graphs and garbage collection. Your feedback is valid, but I think it is true because negotiate_only and recurse_submodules just happen to have a specifal interaction. As indicated by the comments, this early return can apply to any situation where objects were not fetched at all, but we only consider negotiate_only right now. This early return should also apply to submodules because if no new objects were found in the superproject, all of the superproject commits are referencing known submodule commits. If the submodule commits are not known, they should be updated with "git submodule update", not "git fetch". >> if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { >> struct strvec options = STRVEC_INIT; >> int max_children = max_jobs; >> @@ -2132,8 +2153,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) >> strvec_clear(&options); >> } >> >> - string_list_clear(&list, 0); >> - > > Namely, here. Hm, I realize that I could have used a goto instead of moving string_list_clear().. > >> prepare_repo_settings(the_repository); > > This is existing code, but I wonder why it can be done _SO_ late in > the sequence. We've already called the transport API for the > negotiate-only communication at this point, but a call to this > function is the only thing that gives fetch_negotiation_algorithm > member in the_repository its default value, isn't it? That's right, this looks like it could be a bug. Maybe Jonathan knows more. [1] https://lore.kernel.org/git/kl6lwnk4to5x.fsf@chooglen-macbookpro.roam.corp.google.com
Glen Choo <chooglen@google.com> writes: >> By the way, do not move the check about the number of negotiation >> tips from the original location. That check, or its location, have >> nothing to do with what you want to do in this patch, which is "do >> not gc or update the graph file if we are not fetching". It is >> better to leave unrelated changes out of the patch. > > Ah, I see that it's not easy to tell whether or not the behavior is > correct after that line is moved. I'll avoid doing this in the future. > > I still think that it is cleaner to move the negotiation_tip.nr check. > Should I do this in a follow-up patch? I am not seeing what makes it cleaner to have it early or at the current position, but with "It is cleaner to do tip.nr check early because ...", in a separate patch, it may become obvious. But let's not do it in this patch. > I hope Jonathan can chime in to confirm whether or not users want/need > to invoke "--negotiate-only". Yeah, I knew that this was "internal implementation detail", but then perhaps we know that a sane developer who knows what they are doing will never write combination of it with recurse-submodule option? If so, we'd catch a notice developer breaking the system by having a sanity check by detecting it as an error, no?
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >>> By the way, do not move the check about the number of negotiation >>> tips from the original location. That check, or its location, have >>> nothing to do with what you want to do in this patch, which is "do >>> not gc or update the graph file if we are not fetching". It is >>> better to leave unrelated changes out of the patch. >> >> Ah, I see that it's not easy to tell whether or not the behavior is >> correct after that line is moved. I'll avoid doing this in the future. >> >> I still think that it is cleaner to move the negotiation_tip.nr check. >> Should I do this in a follow-up patch? > > I am not seeing what makes it cleaner to have it early or at the > current position, but with "It is cleaner to do tip.nr check early > because ...", in a separate patch, it may become obvious. But let's > not do it in this patch. Noted, thanks! >> I hope Jonathan can chime in to confirm whether or not users want/need >> to invoke "--negotiate-only". > > Yeah, I knew that this was "internal implementation detail", but > then perhaps we know that a sane developer who knows what they are > doing will never write combination of it with recurse-submodule > option? If so, we'd catch a notice developer breaking the system by > having a sanity check by detecting it as an error, no? Hm, I suppose that is true, though I have some reservations. In particular, I'm not sure if the argument you are making is intended to be specific to this combination of options, or all incompatible options in general. I think it might make sense to check for this specific combination of "--negotiate-only" and "--recurse-submodules" because where the stakes are low and behavior is reasonably simple to understand, though the payoff is also pretty low. In the more general case of "Does it *always* make sense to check for an invalid combination of options?", I find your argument a bit troubling because it seems to imply that anything we add to the Git CLI should be treated as if users will depend on it. This seems like a needless burden (or even an antipattern) if we need to fork a Git process purely as an implementation detail, but we have to treat these internal CLI options as user-facing. But maybe I am misunderstanding how Git treats CLI options in general? We don't ever really hide CLI options, even if they are only internal. There is PARSE_OPT_HIDDEN, but those still show up in usage and documentation [1]. So we actually do want users to know about these implementation details? Presumably, the thought process goes something like this: if we add an option, a user *could* find a use for it (or it might accidentally conflict with a user's flags), even if we never intended it for end user consumption. Thus we need to treat all CLI options with care. Is this a more accurate description of how we think about CLI options? [1] This statement doesn't apply to the -- commands (like submodule--helper). Those are 'truly' hidden because they don't have public docs AFAIK.
Glen Choo <chooglen@google.com> writes: > But maybe I am misunderstanding how Git treats CLI options in general? > We don't ever really hide CLI options, even if they are only > internal. There is PARSE_OPT_HIDDEN, but those still show up in usage > and documentation [1]. So we actually do want users to know about these > implementation details? At least they should be told not to use from the command line when they have no business triggering whatever feature these options trigger. > Presumably, the thought process goes something like this: if we add an > option, a user *could* find a use for it (or it might accidentally > conflict with a user's flags), even if we never intended it for end user > consumption. Thus we need to treat all CLI options with care. Rather, they can keep both halves ;-)
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >> But maybe I am misunderstanding how Git treats CLI options in general? >> We don't ever really hide CLI options, even if they are only >> internal. There is PARSE_OPT_HIDDEN, but those still show up in usage >> and documentation [1]. So we actually do want users to know about these >> implementation details? > > At least they should be told not to use from the command line when > they have no business triggering whatever feature these options > trigger. Makes sense. I can add this to the docs if we think --negotiate-only isn't meant for end users. >> Presumably, the thought process goes something like this: if we add an >> option, a user *could* find a use for it (or it might accidentally >> conflict with a user's flags), even if we never intended it for end user >> consumption. Thus we need to treat all CLI options with care. > > Rather, they can keep both halves ;-) Hm, do you mean "both halves" as in, they should be able to choose whether or not they want to use an internal option?
Glen Choo <chooglen@google.com> writes: >>> prepare_repo_settings(the_repository); >> >> This is existing code, but I wonder why it can be done _SO_ late in >> the sequence. We've already called the transport API for the >> negotiate-only communication at this point, but a call to this >> function is the only thing that gives fetch_negotiation_algorithm >> member in the_repository its default value, isn't it? > > That's right, this looks like it could be a bug. Maybe Jonathan knows > more. It seems that fetch negotiation always calls prepare_repo_settings(). Fetch negotiation uses negotiate_using_fetch(), which calls fetch_negotiator_init(), which calls prepare_repo_settings(): void fetch_negotiator_init(struct repository *r, struct fetch_negotiator *negotiator) { prepare_repo_settings(r); switch(r->settings.fetch_negotiation_algorithm) { case FETCH_NEGOTIATION_SKIPPING: skipping_negotiator_init(negotiator); return; case FETCH_NEGOTIATION_NOOP: noop_negotiator_init(negotiator); return; case FETCH_NEGOTIATION_DEFAULT: default_negotiator_init(negotiator); return; } } If anything, this seems safer than calling prepare_repo_settings() in cmd_fetch() :)
diff --git a/builtin/fetch.c b/builtin/fetch.c index f7abbc31ff..85091af99b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, builtin_fetch_options, builtin_fetch_usage, 0); + + if (negotiate_only) { + /* + * --negotiate-only should never recurse into + * submodules, so there is no need to read .gitmodules. + */ + recurse_submodules = RECURSE_SUBMODULES_OFF; + if (!negotiation_tip.nr) + die(_("--negotiate-only needs one or more --negotiate-tip=*")); + } + if (recurse_submodules != RECURSE_SUBMODULES_OFF) { int *sfjc = submodule_fetch_jobs_config == -1 ? &submodule_fetch_jobs_config : NULL; @@ -2005,9 +2016,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) fetch_config_from_gitmodules(sfjc, rs); } - if (negotiate_only && !negotiation_tip.nr) - die(_("--negotiate-only needs one or more --negotiate-tip=*")); - if (deepen_relative) { if (deepen_relative < 0) die(_("Negative depth in --deepen is not supported")); @@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) result = fetch_multiple(&list, max_children); } + string_list_clear(&list, 0); + + /* + * Skip irrelevant tasks because we know objects were not + * fetched. + * + * NEEDSWORK: as a future optimization, we can return early + * whenever objects were not fetched e.g. if we already have all + * of them. + */ + if (negotiate_only) + return result; + if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { struct strvec options = STRVEC_INIT; int max_children = max_jobs; @@ -2132,8 +2153,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) strvec_clear(&options); } - string_list_clear(&list, 0); - prepare_repo_settings(the_repository); if (fetch_write_commit_graph > 0 || (fetch_write_commit_graph < 0 && diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 8212ca56dc..732031085e 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f test_i18ngrep "push negotiation failed" err ' +test_expect_success 'push with negotiation does not attempt to fetch submodules' ' + mk_empty submodule_upstream && + test_commit -C submodule_upstream submodule_commit && + git submodule add ./submodule_upstream submodule && + mk_empty testrepo && + git push testrepo $the_first_commit:refs/remotes/origin/first_commit && + test_commit -C testrepo unrelated_commit && + git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit && + git -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err && + ! grep "Fetching submodule" err +' + test_expect_success 'push without wildcard' ' mk_empty testrepo &&
cmd_fetch() performs certain tasks with the assumption that objects are fetched, but `git fetch --negotiate-only` does not fetch objects, as its name implies. This is results in behavior that is unnecessary at best, and incorrect at worst: * Submodules are updated if enabled by recurse.submodules=true, but negotiation fetch doesn't actually update the repo, so this doesn't make sense (introduced in [1]). * Commit graphs will be written if enabled by fetch.writeCommitGraph=true. But according to Documentation/config/fetch.txt [2], this should only be done if a pack-file is downloaded. * gc is run, but according to [3], we only do this because we expect `git fetch` to introduce objects. Instead of disabling these tasks piecemeal, make cmd_fetch() return early if we know for certain that objects will not be fetched. We can return early whenever objects are not fetched, but for now this only considers --negotiate-only. [1] 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12) [2] 50f26bd035 (fetch: add fetch.writeCommitGraph config setting, 2019-09-02) [3] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26) Signed-off-by: Glen Choo <chooglen@google.com> --- Thanks for the review, Jonathan! Changes since v1: * added more context to commit message * added a NEEDSWORK comment Interdiff against v1: diff --git a/builtin/fetch.c b/builtin/fetch.c index 01865b5c09..85091af99b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2122,7 +2122,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) string_list_clear(&list, 0); - /* skip irrelevant tasks if objects were not fetched */ + /* + * Skip irrelevant tasks because we know objects were not + * fetched. + * + * NEEDSWORK: as a future optimization, we can return early + * whenever objects were not fetched e.g. if we already have all + * of them. + */ if (negotiate_only) return result; builtin/fetch.c | 29 ++++++++++++++++++++++++----- t/t5516-fetch-push.sh | 12 ++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-)