| Message ID | 20210806214612.1501980-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | transport: no warning if no server wait-for-done | expand |
On Fri, Aug 06 2021, Jonathan Tan wrote: > When the push.negotiate configuration variable was implemented in > 477673d6f3 ("send-pack: support push negotiation", 2021-05-05), Git was > taught to print warning messages whenever that variable was set to true > but push negotiation didn't happen. However, the lack of push > negotiation is more similar to things like the usage of protocol v2 - in > which the new feature is desired for performance, but if the feature > is not there, the user does not intend to take any action - than to > things like the usage of --filter - in which the new feature is desired > for a certain outcome, and if the outcome does not happen, there is a > high chance that the user would need to do something else (in this case, > for example, reclone with "--depth" if the user needs the disk space). > > Therefore, when pushing with push.negotiate set to true, do not warn if > wait-for-done is not supported for any reason (e.g. if the server is > using an older version of Git or if protocol v2 is deactivated on the > server). This is done by using an internal-use-only parameter to "git > fetch". Tangentally related (the alternative was to start a thread on some 2018-era patch of yours): Is it intentional that when you supply a gibberish OID or a nonexisting one as an explicit negotiation tip we don't even warn about it? Looking at the history of fetch-pack.c I suspect not. It goes back to ec06283844a (fetch-pack: introduce negotiator API, 2018-06-14), i.e. the "o && o->type == OBJ_COMMIT" check, now "if (c)" (as in could we look up a commit) on "master". That in turn seems to go back as far as 9534f40bc42 (Be careful when dereferencing tags., 2005-11-02). > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > builtin/fetch.c | 8 +++++++- > send-pack.c | 11 +++-------- > t/t5516-fetch-push.sh | 3 +-- > transport.c | 6 ++++-- > transport.h | 6 ++++++ > 5 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 25740c13df..940d650aba 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -84,6 +84,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP; > static int fetch_write_commit_graph = -1; > static int stdin_refspecs = 0; > static int negotiate_only; > +static int negotiate_only_failure_ok; > > static int git_fetch_config(const char *k, const char *v, void *cb) > { > @@ -208,6 +209,8 @@ static struct option builtin_fetch_options[] = { > N_("report that we have only objects reachable from this object")), > OPT_BOOL(0, "negotiate-only", &negotiate_only, > N_("do not fetch a packfile; instead, print ancestors of negotiation tips")), > + OPT_BOOL(0, "negotiate-only-failure-ok", &negotiate_only_failure_ok, > + N_("for internal use only: if --negotiate-only fails, do not print a warning message")), > OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options), > OPT_BOOL(0, "auto-maintenance", &enable_auto_gc, > N_("run 'maintenance --auto' after fetching")), > @@ -2059,8 +2062,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > gtransport = prepare_transport(remote, 1); > if (gtransport->smart_options) { > gtransport->smart_options->acked_commits = &acked_commits; > + gtransport->smart_options->negotiate_only_failure_ok = > + negotiate_only_failure_ok; > } else { > - warning(_("Protocol does not support --negotiate-only, exiting.")); > + if (!negotiate_only_failure_ok) > + warning(_("Protocol does not support --negotiate-only, exiting.")); > return 1; But we still want to "return 1" here and not proceed with the fetch?, ah yes, because we run this via send-pack.c below... > } > if (server_options.nr) > diff --git a/send-pack.c b/send-pack.c > index 5a79e0e711..020fd0b265 100644 > --- a/send-pack.c > +++ b/send-pack.c > @@ -424,7 +424,8 @@ static void get_commons_through_negotiation(const char *url, > child.git_cmd = 1; > child.no_stdin = 1; > child.out = -1; > - strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL); > + strvec_pushl(&child.args, "fetch", "--negotiate-only", > + "--negotiate-only-failure-ok", NULL); > for (ref = remote_refs; ref; ref = ref->next) > strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid)); > strvec_push(&child.args, url); > @@ -447,13 +448,7 @@ static void get_commons_through_negotiation(const char *url, > oid_array_append(commons, &oid); > } while (1); > > - if (finish_command(&child)) { > - /* > - * The information that push negotiation provides is useful but > - * not mandatory. > - */ > - warning(_("push negotiation failed; proceeding anyway with push")); > - } > + finish_command(&child); > } > > int send_pack(struct send_pack_args *args, > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 0916f76302..60b377edf2 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -222,8 +222,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f > git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit && > GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \ > git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err && > - grep_wrote 5 event && # 2 commits, 2 trees, 1 blob > - test_i18ngrep "push negotiation failed" err > + grep_wrote 5 event # 2 commits, 2 trees, 1 blob > ' > > test_expect_success 'push without wildcard' ' > diff --git a/transport.c b/transport.c > index 17e9629710..913fc0f8e4 100644 > --- a/transport.c > +++ b/transport.c > @@ -397,10 +397,12 @@ static int fetch_refs_via_pack(struct transport *transport, > > if (data->options.acked_commits) { > if (data->version < protocol_v2) { > - warning(_("--negotiate-only requires protocol v2")); > + if (!data->options.negotiate_only_failure_ok) > + warning(_("--negotiate-only requires protocol v2")); > ret = -1; > } else if (!server_supports_feature("fetch", "wait-for-done", 0)) { > - warning(_("server does not support wait-for-done")); > + if (!data->options.negotiate_only_failure_ok) > + warning(_("server does not support wait-for-done")); > ret = -1; > } else { > negotiate_using_fetch(data->options.negotiation_tips, > diff --git a/transport.h b/transport.h > index 1cbab11373..98c90b46df 100644 > --- a/transport.h > +++ b/transport.h > @@ -53,6 +53,12 @@ struct git_transport_options { > * common commits to this oidset instead of fetching any packfiles. > */ > struct oidset *acked_commits; > + > + /* > + * If the server does not support wait-for-done, do not print any > + * warning messages. > + */ > + unsigned negotiate_only_failure_ok : 1; > }; > > enum transport_family { I find myself wondering if a new option for this, or if --negotiate-only shouldn't just pay attention to the existing --quiet and --verbose options, depending. We already pass that down to this level through the transport struct you're changing. So since we're running a one-off special command here why not just pass --quiet and check args.quiet in fetch_refs_via_pack() before emitting the warning()? Ditto for fetch itself...
Jonathan Tan <jonathantanmy@google.com> writes: > When the push.negotiate configuration variable was implemented in > 477673d6f3 ("send-pack: support push negotiation", 2021-05-05), Git was > taught to print warning messages whenever that variable was set to true > but push negotiation didn't happen. However, the lack of push > negotiation is more similar to things like the usage of protocol v2 - in > which the new feature is desired for performance, but if the feature > is not there, the user does not intend to take any action - than to > things like the usage of --filter - in which the new feature is desired > for a certain outcome, and if the outcome does not happen, there is a > high chance that the user would need to do something else (in this case, > for example, reclone with "--depth" if the user needs the disk space). > > Therefore, when pushing with push.negotiate set to true, do not warn if > wait-for-done is not supported for any reason (e.g. if the server is > using an older version of Git or if protocol v2 is deactivated on the > server). This is done by using an internal-use-only parameter to "git > fetch". Hmph, if this were a hard error, instead of "print warning messages", the above discussion is entirely reasonable. But I am not sure if squelching the warning unconditionally, especially given that the feature is relatively young, is a good idea. I suspect that you are trying to hide the "failed" message that may sound more alarming then it should be from the end users, but if that is the case, wouldn't it be a better solution to reword the message to tone it down? > - if (finish_command(&child)) { > - /* > - * The information that push negotiation provides is useful but > - * not mandatory. > - */ > - warning(_("push negotiation failed; proceeding anyway with push")); > - } Perhaps like "optional ancestry negotiation failed---pushing normally" or some phrasing that assures the users that pushing without negotiation is perfectly normal?
Hi, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: >> - if (finish_command(&child)) { >> - /* >> - * The information that push negotiation provides is useful but >> - * not mandatory. >> - */ >> - warning(_("push negotiation failed; proceeding anyway with push")); >> - } > > Perhaps like "optional ancestry negotiation failed---pushing > normally" or some phrasing that assures the users that pushing > without negotiation is perfectly normal? The question is what the user will do with this information. Will they contact the service provider to ask them to turn on push negotiation? Will they turn off push negotiation because they don't want to waste a round trip? Does what they will do depend on _why_ push negotiation failed? If it failed because the server didn't declare the capability and the user has set push.negotate to true to represent "I want to live in the future by using push negotiation wherever it's available", then the message is noise. If it failed due to a bug, then the message is more relevant to the user --- e.g., should we use a different exit status to distinguish between these two cases? Thanks, Jonathan
Jonathan Nieder <jrnieder@gmail.com> writes: >>> - if (finish_command(&child)) { >>> - /* >>> - * The information that push negotiation provides is useful but >>> - * not mandatory. >>> - */ >>> - warning(_("push negotiation failed; proceeding anyway with push")); >>> - } >> >> Perhaps like "optional ancestry negotiation failed---pushing >> normally" or some phrasing that assures the users that pushing >> without negotiation is perfectly normal? > > The question is what the user will do with this information. > > Will they contact the service provider to ask them to turn on push > negotiation? > > Will they turn off push negotiation because they don't want to waste a > round trip? > > Does what they will do depend on _why_ push negotiation failed? If it > failed because the server didn't declare the capability and the user > has set push.negotate to true to represent "I want to live in the > future by using push negotiation wherever it's available", then the > message is noise. If it failed due to a bug, then the message is more > relevant to the user --- e.g., should we use a different exit status > to distinguish between these two cases? I was hoping that the "child" command being run there gives enough clue before the warning message so that the user would already know (that is where my "assures the users" primarily comes from---even we cannot hide that the negotiation poll has failed, the users would be helped by being told that it is OK). If "child" does not give enough clue to this codepath (via the exit code) or to the end user (via error messages), we probably would want to fix that for that approach to work.
> Tangentally related (the alternative was to start a thread on some > 2018-era patch of yours): Is it intentional that when you supply a > gibberish OID or a nonexisting one as an explicit negotiation tip we > don't even warn about it? > > Looking at the history of fetch-pack.c I suspect not. It goes back to > ec06283844a (fetch-pack: introduce negotiator API, 2018-06-14), i.e. the > "o && o->type == OBJ_COMMIT" check, now "if (c)" (as in could we look up > a commit) on "master". That in turn seems to go back as far as > 9534f40bc42 (Be careful when dereferencing tags., 2005-11-02). This sounds unintentional, yes. Other than replying to an old email, you could also write a top-level patch :-)
> Jonathan Nieder <jrnieder@gmail.com> writes: > > >>> - if (finish_command(&child)) { > >>> - /* > >>> - * The information that push negotiation provides is useful but > >>> - * not mandatory. > >>> - */ > >>> - warning(_("push negotiation failed; proceeding anyway with push")); > >>> - } > >> > >> Perhaps like "optional ancestry negotiation failed---pushing > >> normally" or some phrasing that assures the users that pushing > >> without negotiation is perfectly normal? > > > > The question is what the user will do with this information. > > > > Will they contact the service provider to ask them to turn on push > > negotiation? > > > > Will they turn off push negotiation because they don't want to waste a > > round trip? > > > > Does what they will do depend on _why_ push negotiation failed? If it > > failed because the server didn't declare the capability and the user > > has set push.negotate to true to represent "I want to live in the > > future by using push negotiation wherever it's available", then the > > message is noise. If it failed due to a bug, then the message is more > > relevant to the user --- e.g., should we use a different exit status > > to distinguish between these two cases? > > I was hoping that the "child" command being run there gives enough > clue before the warning message so that the user would already know > (that is where my "assures the users" primarily comes from---even we > cannot hide that the negotiation poll has failed, the users would be > helped by being told that it is OK). > > If "child" does not give enough clue to this codepath (via the exit > code) or to the end user (via error messages), we probably would > want to fix that for that approach to work. The idea of this patch is to treat it like a user wanting to use protocol v2 - if the remote doesn't support it, then the user is perfectly fine falling back, and any extra message would be noise because the user wouldn't take action on it anyway. If we think that the user needs to know when push negotiation fails (for example, if we think "the user enabled it so the user expects it to happen"), then I agree that we should leave the warning messages in and also print a message saying that push is still happening anyway (which was the approach in the original patch set), and maybe update the message to something like Junio suggested [1] to clarify what's going on. But I would think that the user doesn't care - for example, I could imagine someone globally enabling it and having pushes become automatically faster as more and more remotes support it. [1] https://lore.kernel.org/git/xmqqh7fyfrtl.fsf@gitster.g/
diff --git a/builtin/fetch.c b/builtin/fetch.c index 25740c13df..940d650aba 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -84,6 +84,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP; static int fetch_write_commit_graph = -1; static int stdin_refspecs = 0; static int negotiate_only; +static int negotiate_only_failure_ok; static int git_fetch_config(const char *k, const char *v, void *cb) { @@ -208,6 +209,8 @@ static struct option builtin_fetch_options[] = { N_("report that we have only objects reachable from this object")), OPT_BOOL(0, "negotiate-only", &negotiate_only, N_("do not fetch a packfile; instead, print ancestors of negotiation tips")), + OPT_BOOL(0, "negotiate-only-failure-ok", &negotiate_only_failure_ok, + N_("for internal use only: if --negotiate-only fails, do not print a warning message")), OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options), OPT_BOOL(0, "auto-maintenance", &enable_auto_gc, N_("run 'maintenance --auto' after fetching")), @@ -2059,8 +2062,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) gtransport = prepare_transport(remote, 1); if (gtransport->smart_options) { gtransport->smart_options->acked_commits = &acked_commits; + gtransport->smart_options->negotiate_only_failure_ok = + negotiate_only_failure_ok; } else { - warning(_("Protocol does not support --negotiate-only, exiting.")); + if (!negotiate_only_failure_ok) + warning(_("Protocol does not support --negotiate-only, exiting.")); return 1; } if (server_options.nr) diff --git a/send-pack.c b/send-pack.c index 5a79e0e711..020fd0b265 100644 --- a/send-pack.c +++ b/send-pack.c @@ -424,7 +424,8 @@ static void get_commons_through_negotiation(const char *url, child.git_cmd = 1; child.no_stdin = 1; child.out = -1; - strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL); + strvec_pushl(&child.args, "fetch", "--negotiate-only", + "--negotiate-only-failure-ok", NULL); for (ref = remote_refs; ref; ref = ref->next) strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid)); strvec_push(&child.args, url); @@ -447,13 +448,7 @@ static void get_commons_through_negotiation(const char *url, oid_array_append(commons, &oid); } while (1); - if (finish_command(&child)) { - /* - * The information that push negotiation provides is useful but - * not mandatory. - */ - warning(_("push negotiation failed; proceeding anyway with push")); - } + finish_command(&child); } int send_pack(struct send_pack_args *args, diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 0916f76302..60b377edf2 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -222,8 +222,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit && GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \ git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err && - grep_wrote 5 event && # 2 commits, 2 trees, 1 blob - test_i18ngrep "push negotiation failed" err + grep_wrote 5 event # 2 commits, 2 trees, 1 blob ' test_expect_success 'push without wildcard' ' diff --git a/transport.c b/transport.c index 17e9629710..913fc0f8e4 100644 --- a/transport.c +++ b/transport.c @@ -397,10 +397,12 @@ static int fetch_refs_via_pack(struct transport *transport, if (data->options.acked_commits) { if (data->version < protocol_v2) { - warning(_("--negotiate-only requires protocol v2")); + if (!data->options.negotiate_only_failure_ok) + warning(_("--negotiate-only requires protocol v2")); ret = -1; } else if (!server_supports_feature("fetch", "wait-for-done", 0)) { - warning(_("server does not support wait-for-done")); + if (!data->options.negotiate_only_failure_ok) + warning(_("server does not support wait-for-done")); ret = -1; } else { negotiate_using_fetch(data->options.negotiation_tips, diff --git a/transport.h b/transport.h index 1cbab11373..98c90b46df 100644 --- a/transport.h +++ b/transport.h @@ -53,6 +53,12 @@ struct git_transport_options { * common commits to this oidset instead of fetching any packfiles. */ struct oidset *acked_commits; + + /* + * If the server does not support wait-for-done, do not print any + * warning messages. + */ + unsigned negotiate_only_failure_ok : 1; }; enum transport_family {
When the push.negotiate configuration variable was implemented in 477673d6f3 ("send-pack: support push negotiation", 2021-05-05), Git was taught to print warning messages whenever that variable was set to true but push negotiation didn't happen. However, the lack of push negotiation is more similar to things like the usage of protocol v2 - in which the new feature is desired for performance, but if the feature is not there, the user does not intend to take any action - than to things like the usage of --filter - in which the new feature is desired for a certain outcome, and if the outcome does not happen, there is a high chance that the user would need to do something else (in this case, for example, reclone with "--depth" if the user needs the disk space). Therefore, when pushing with push.negotiate set to true, do not warn if wait-for-done is not supported for any reason (e.g. if the server is using an older version of Git or if protocol v2 is deactivated on the server). This is done by using an internal-use-only parameter to "git fetch". Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- builtin/fetch.c | 8 +++++++- send-pack.c | 11 +++-------- t/t5516-fetch-push.sh | 3 +-- transport.c | 6 ++++-- transport.h | 6 ++++++ 5 files changed, 21 insertions(+), 13 deletions(-)