{"thread":{"id":"56228","subject":"[PATCH] transport: no warning if no server wait-for-done","startedAt":"2021-08-06T21:46:19Z","lastAt":"2021-08-16T23:06:51Z","messageCount":7,"participants":["Jonathan Tan","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"432213","messageId":"20210806214612.1501980-1-jonathantanmy@google.com","threadId":"56228","inReplyTo":null,"subject":"[PATCH] transport: no warning if no server wait-for-done","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-06T21:46:12Z","receivedAt":"2021-08-06T21:46:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When the push.negotiate configuration variable was implemented in\n477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05), Git was\ntaught to print warning messages whenever that variable was set to true\nbut push negotiation didn't happen. However, the lack of push\nnegotiation is more similar to things like the usage of protocol v2 - in\nwhich the new feature is desired for performance, but if the feature\nis not there, the user does not intend to take any action - than to\nthings like the usage of --filter - in which the new feature is desired\nfor a certain outcome, and if the outcome does not happen, there is a\nhigh chance that the user would need to do something else (in this case,\nfor example, reclone with \"--depth\" if the user needs the disk space).\n\nTherefore, when pushing with push.negotiate set to true, do not warn if\nwait-for-done is not supported for any reason (e.g. if the server is\nusing an older version of Git or if protocol v2 is deactivated on the\nserver). This is done by using an internal-use-only parameter to \"git\nfetch\".\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/fetch.c       |  8 +++++++-\n send-pack.c           | 11 +++--------\n t/t5516-fetch-push.sh |  3 +--\n transport.c           |  6 ++++--\n transport.h           |  6 ++++++\n 5 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 25740c13df..940d650aba 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -84,6 +84,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\n static int stdin_refspecs = 0;\n static int negotiate_only;\n+static int negotiate_only_failure_ok;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -208,6 +209,8 @@ static struct option builtin_fetch_options[] = {\n \t\t\tN_(\"report that we have only objects reachable from this object\")),\n \tOPT_BOOL(0, \"negotiate-only\", &negotiate_only,\n \t\t N_(\"do not fetch a packfile; instead, print ancestors of negotiation tips\")),\n+\tOPT_BOOL(0, \"negotiate-only-failure-ok\", &negotiate_only_failure_ok,\n+\t\t N_(\"for internal use only: if --negotiate-only fails, do not print a warning message\")),\n \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \tOPT_BOOL(0, \"auto-maintenance\", &enable_auto_gc,\n \t\t N_(\"run 'maintenance --auto' after fetching\")),\n@@ -2059,8 +2062,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tgtransport = prepare_transport(remote, 1);\n \t\tif (gtransport->smart_options) {\n \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n+\t\t\tgtransport->smart_options->negotiate_only_failure_ok =\n+\t\t\t\tnegotiate_only_failure_ok;\n \t\t} else {\n-\t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n+\t\t\tif (!negotiate_only_failure_ok)\n+\t\t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n \t\t\treturn 1;\n \t\t}\n \t\tif (server_options.nr)\ndiff --git a/send-pack.c b/send-pack.c\nindex 5a79e0e711..020fd0b265 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -424,7 +424,8 @@ static void get_commons_through_negotiation(const char *url,\n \tchild.git_cmd = 1;\n \tchild.no_stdin = 1;\n \tchild.out = -1;\n-\tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n+\tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\",\n+\t\t     \"--negotiate-only-failure-ok\", NULL);\n \tfor (ref = remote_refs; ref; ref = ref->next)\n \t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n \tstrvec_push(&child.args, url);\n@@ -447,13 +448,7 @@ static void get_commons_through_negotiation(const char *url,\n \t\toid_array_append(commons, &oid);\n \t} while (1);\n \n-\tif (finish_command(&child)) {\n-\t\t/*\n-\t\t * The information that push negotiation provides is useful but\n-\t\t * not mandatory.\n-\t\t */\n-\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n-\t}\n+\tfinish_command(&child);\n }\n \n int send_pack(struct send_pack_args *args,\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 0916f76302..60b377edf2 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -222,8 +222,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n \t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n-\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n-\ttest_i18ngrep \"push negotiation failed\" err\n+\tgrep_wrote 5 event # 2 commits, 2 trees, 1 blob\n '\n \n test_expect_success 'push without wildcard' '\ndiff --git a/transport.c b/transport.c\nindex 17e9629710..913fc0f8e4 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -397,10 +397,12 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tif (data->options.acked_commits) {\n \t\tif (data->version < protocol_v2) {\n-\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n+\t\t\tif (!data->options.negotiate_only_failure_ok)\n+\t\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n \t\t\tret = -1;\n \t\t} else if (!server_supports_feature(\"fetch\", \"wait-for-done\", 0)) {\n-\t\t\twarning(_(\"server does not support wait-for-done\"));\n+\t\t\tif (!data->options.negotiate_only_failure_ok)\n+\t\t\t\twarning(_(\"server does not support wait-for-done\"));\n \t\t\tret = -1;\n \t\t} else {\n \t\t\tnegotiate_using_fetch(data->options.negotiation_tips,\ndiff --git a/transport.h b/transport.h\nindex 1cbab11373..98c90b46df 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -53,6 +53,12 @@ struct git_transport_options {\n \t * common commits to this oidset instead of fetching any packfiles.\n \t */\n \tstruct oidset *acked_commits;\n+\n+\t/*\n+\t * If the server does not support wait-for-done, do not print any\n+\t * warning messages.\n+\t */\n+\tunsigned negotiate_only_failure_ok : 1;\n };\n \n enum transport_family {\n-- \n2.32.0.605.g8dce9f2422-goog\n\n"},{"id":"432218","messageId":"87eeb6yr0m.fsf@evledraar.gmail.com","threadId":"56228","inReplyTo":"20210806214612.1501980-1-jonathantanmy@google.com","subject":"Re: [PATCH] transport: no warning if no server wait-for-done","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-07T01:31:01Z","receivedAt":"2021-08-07T01:37:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 06 2021, Jonathan Tan wrote:\n\n> When the push.negotiate configuration variable was implemented in\n> 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05), Git was\n> taught to print warning messages whenever that variable was set to true\n> but push negotiation didn't happen. However, the lack of push\n> negotiation is more similar to things like the usage of protocol v2 - in\n> which the new feature is desired for performance, but if the feature\n> is not there, the user does not intend to take any action - than to\n> things like the usage of --filter - in which the new feature is desired\n> for a certain outcome, and if the outcome does not happen, there is a\n> high chance that the user would need to do something else (in this case,\n> for example, reclone with \"--depth\" if the user needs the disk space).\n>\n> Therefore, when pushing with push.negotiate set to true, do not warn if\n> wait-for-done is not supported for any reason (e.g. if the server is\n> using an older version of Git or if protocol v2 is deactivated on the\n> server). This is done by using an internal-use-only parameter to \"git\n> fetch\".\n\nTangentally related (the alternative was to start a thread on some\n2018-era patch of yours): Is it intentional that when you supply a\ngibberish OID or a nonexisting one as an explicit negotiation tip we\ndon't even warn about it?\n\nLooking at the history of fetch-pack.c I suspect not. It goes back to\nec06283844a (fetch-pack: introduce negotiator API, 2018-06-14), i.e. the\n\"o && o->type == OBJ_COMMIT\" check, now \"if (c)\" (as in could we look up\na commit) on \"master\". That in turn seems to go back as far as\n9534f40bc42 (Be careful when dereferencing tags., 2005-11-02).\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  builtin/fetch.c       |  8 +++++++-\n>  send-pack.c           | 11 +++--------\n>  t/t5516-fetch-push.sh |  3 +--\n>  transport.c           |  6 ++++--\n>  transport.h           |  6 ++++++\n>  5 files changed, 21 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 25740c13df..940d650aba 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -84,6 +84,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n>  static int fetch_write_commit_graph = -1;\n>  static int stdin_refspecs = 0;\n>  static int negotiate_only;\n> +static int negotiate_only_failure_ok;\n>  \n>  static int git_fetch_config(const char *k, const char *v, void *cb)\n>  {\n> @@ -208,6 +209,8 @@ static struct option builtin_fetch_options[] = {\n>  \t\t\tN_(\"report that we have only objects reachable from this object\")),\n>  \tOPT_BOOL(0, \"negotiate-only\", &negotiate_only,\n>  \t\t N_(\"do not fetch a packfile; instead, print ancestors of negotiation tips\")),\n> +\tOPT_BOOL(0, \"negotiate-only-failure-ok\", &negotiate_only_failure_ok,\n> +\t\t N_(\"for internal use only: if --negotiate-only fails, do not print a warning message\")),\n>  \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n>  \tOPT_BOOL(0, \"auto-maintenance\", &enable_auto_gc,\n>  \t\t N_(\"run 'maintenance --auto' after fetching\")),\n> @@ -2059,8 +2062,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \t\tgtransport = prepare_transport(remote, 1);\n>  \t\tif (gtransport->smart_options) {\n>  \t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n> +\t\t\tgtransport->smart_options->negotiate_only_failure_ok =\n> +\t\t\t\tnegotiate_only_failure_ok;\n>  \t\t} else {\n> -\t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n> +\t\t\tif (!negotiate_only_failure_ok)\n> +\t\t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n>  \t\t\treturn 1;\n\nBut we still want to \"return 1\" here and not proceed with the fetch?, ah\nyes, because we run this via send-pack.c below...\n\n>  \t\t}\n>  \t\tif (server_options.nr)\n> diff --git a/send-pack.c b/send-pack.c\n> index 5a79e0e711..020fd0b265 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -424,7 +424,8 @@ static void get_commons_through_negotiation(const char *url,\n>  \tchild.git_cmd = 1;\n>  \tchild.no_stdin = 1;\n>  \tchild.out = -1;\n> -\tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n> +\tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\",\n> +\t\t     \"--negotiate-only-failure-ok\", NULL);\n>  \tfor (ref = remote_refs; ref; ref = ref->next)\n>  \t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n>  \tstrvec_push(&child.args, url);\n> @@ -447,13 +448,7 @@ static void get_commons_through_negotiation(const char *url,\n>  \t\toid_array_append(commons, &oid);\n>  \t} while (1);\n>  \n> -\tif (finish_command(&child)) {\n> -\t\t/*\n> -\t\t * The information that push negotiation provides is useful but\n> -\t\t * not mandatory.\n> -\t\t */\n> -\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n> -\t}\n> +\tfinish_command(&child);\n>  }\n>  \n>  int send_pack(struct send_pack_args *args,\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 0916f76302..60b377edf2 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -222,8 +222,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n>  \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n>  \tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n>  \t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n> -\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n> -\ttest_i18ngrep \"push negotiation failed\" err\n> +\tgrep_wrote 5 event # 2 commits, 2 trees, 1 blob\n>  '\n>  \n>  test_expect_success 'push without wildcard' '\n> diff --git a/transport.c b/transport.c\n> index 17e9629710..913fc0f8e4 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -397,10 +397,12 @@ static int fetch_refs_via_pack(struct transport *transport,\n>  \n>  \tif (data->options.acked_commits) {\n>  \t\tif (data->version < protocol_v2) {\n> -\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n> +\t\t\tif (!data->options.negotiate_only_failure_ok)\n> +\t\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n>  \t\t\tret = -1;\n>  \t\t} else if (!server_supports_feature(\"fetch\", \"wait-for-done\", 0)) {\n> -\t\t\twarning(_(\"server does not support wait-for-done\"));\n> +\t\t\tif (!data->options.negotiate_only_failure_ok)\n> +\t\t\t\twarning(_(\"server does not support wait-for-done\"));\n>  \t\t\tret = -1;\n>  \t\t} else {\n>  \t\t\tnegotiate_using_fetch(data->options.negotiation_tips,\n> diff --git a/transport.h b/transport.h\n> index 1cbab11373..98c90b46df 100644\n> --- a/transport.h\n> +++ b/transport.h\n> @@ -53,6 +53,12 @@ struct git_transport_options {\n>  \t * common commits to this oidset instead of fetching any packfiles.\n>  \t */\n>  \tstruct oidset *acked_commits;\n> +\n> +\t/*\n> +\t * If the server does not support wait-for-done, do not print any\n> +\t * warning messages.\n> +\t */\n> +\tunsigned negotiate_only_failure_ok : 1;\n>  };\n>  \n>  enum transport_family {\n\nI find myself wondering if a new option for this, or if --negotiate-only\nshouldn't just pay attention to the existing --quiet and --verbose\noptions, depending. We already pass that down to this level through the\ntransport struct you're changing.\n\nSo since we're running a one-off special command here why not just pass\n--quiet and check args.quiet in fetch_refs_via_pack() before emitting\nthe warning()? Ditto for fetch itself...\n"},{"id":"432314","messageId":"xmqqh7fyfrtl.fsf@gitster.g","threadId":"56228","inReplyTo":"20210806214612.1501980-1-jonathantanmy@google.com","subject":"Re: [PATCH] transport: no warning if no server wait-for-done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-09T17:31:18Z","receivedAt":"2021-08-09T17:31:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When the push.negotiate configuration variable was implemented in\n> 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05), Git was\n> taught to print warning messages whenever that variable was set to true\n> but push negotiation didn't happen. However, the lack of push\n> negotiation is more similar to things like the usage of protocol v2 - in\n> which the new feature is desired for performance, but if the feature\n> is not there, the user does not intend to take any action - than to\n> things like the usage of --filter - in which the new feature is desired\n> for a certain outcome, and if the outcome does not happen, there is a\n> high chance that the user would need to do something else (in this case,\n> for example, reclone with \"--depth\" if the user needs the disk space).\n>\n> Therefore, when pushing with push.negotiate set to true, do not warn if\n> wait-for-done is not supported for any reason (e.g. if the server is\n> using an older version of Git or if protocol v2 is deactivated on the\n> server). This is done by using an internal-use-only parameter to \"git\n> fetch\".\n\nHmph, if this were a hard error, instead of \"print warning\nmessages\", the above discussion is entirely reasonable.  But I am\nnot sure if squelching the warning unconditionally, especially given\nthat the feature is relatively young, is a good idea.\n\nI suspect that you are trying to hide the \"failed\" message that may\nsound more alarming then it should be from the end users, but if\nthat is the case, wouldn't it be a better solution to reword the\nmessage to tone it down?\n\n> -\tif (finish_command(&child)) {\n> -\t\t/*\n> -\t\t * The information that push negotiation provides is useful but\n> -\t\t * not mandatory.\n> -\t\t */\n> -\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n> -\t}\n\nPerhaps like \"optional ancestry negotiation failed---pushing\nnormally\" or some phrasing that assures the users that pushing\nwithout negotiation is perfectly normal?\n"},{"id":"432326","messageId":"YRF91qCi+c7csUxM@google.com","threadId":"56228","inReplyTo":"xmqqh7fyfrtl.fsf@gitster.g","subject":"Re: [PATCH] transport: no warning if no server wait-for-done","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-08-09T19:11:18Z","receivedAt":"2021-08-09T19:11:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> -\tif (finish_command(&child)) {\n>> -\t\t/*\n>> -\t\t * The information that push negotiation provides is useful but\n>> -\t\t * not mandatory.\n>> -\t\t */\n>> -\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n>> -\t}\n>\n> Perhaps like \"optional ancestry negotiation failed---pushing\n> normally\" or some phrasing that assures the users that pushing\n> without negotiation is perfectly normal?\n\nThe question is what the user will do with this information.\n\nWill they contact the service provider to ask them to turn on push\nnegotiation?\n\nWill they turn off push negotiation because they don't want to waste a\nround trip?\n\nDoes what they will do depend on _why_ push negotiation failed?  If it\nfailed because the server didn't declare the capability and the user\nhas set push.negotate to true to represent \"I want to live in the\nfuture by using push negotiation wherever it's available\", then the\nmessage is noise.  If it failed due to a bug, then the message is more\nrelevant to the user --- e.g., should we use a different exit status\nto distinguish between these two cases?\n\nThanks,\nJonathan\n"},{"id":"432330","messageId":"xmqq4kbyfkq4.fsf@gitster.g","threadId":"56228","inReplyTo":"YRF91qCi+c7csUxM@google.com","subject":"Re: [PATCH] transport: no warning if no server wait-for-done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-09T20:04:35Z","receivedAt":"2021-08-09T20:04:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>>> -\tif (finish_command(&child)) {\n>>> -\t\t/*\n>>> -\t\t * The information that push negotiation provides is useful but\n>>> -\t\t * not mandatory.\n>>> -\t\t */\n>>> -\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n>>> -\t}\n>>\n>> Perhaps like \"optional ancestry negotiation failed---pushing\n>> normally\" or some phrasing that assures the users that pushing\n>> without negotiation is perfectly normal?\n>\n> The question is what the user will do with this information.\n>\n> Will they contact the service provider to ask them to turn on push\n> negotiation?\n>\n> Will they turn off push negotiation because they don't want to waste a\n> round trip?\n>\n> Does what they will do depend on _why_ push negotiation failed?  If it\n> failed because the server didn't declare the capability and the user\n> has set push.negotate to true to represent \"I want to live in the\n> future by using push negotiation wherever it's available\", then the\n> message is noise.  If it failed due to a bug, then the message is more\n> relevant to the user --- e.g., should we use a different exit status\n> to distinguish between these two cases?\n\nI was hoping that the \"child\" command being run there gives enough\nclue before the warning message so that the user would already know\n(that is where my \"assures the users\" primarily comes from---even we\ncannot hide that the negotiation poll has failed, the users would be\nhelped by being told that it is OK).\n\nIf \"child\" does not give enough clue to this codepath (via the exit\ncode) or to the end user (via error messages), we probably would\nwant to fix that for that approach to work.\n\n\n\n"},{"id":"432888","messageId":"20210816222604.3503037-1-jonathantanmy@google.com","threadId":"56228","inReplyTo":"87eeb6yr0m.fsf@evledraar.gmail.com","subject":"Re: [PATCH] transport: no warning if no server wait-for-done","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T22:26:04Z","receivedAt":"2021-08-16T22:26:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Tangentally related (the alternative was to start a thread on some\n> 2018-era patch of yours): Is it intentional that when you supply a\n> gibberish OID or a nonexisting one as an explicit negotiation tip we\n> don't even warn about it?\n> \n> Looking at the history of fetch-pack.c I suspect not. It goes back to\n> ec06283844a (fetch-pack: introduce negotiator API, 2018-06-14), i.e. the\n> \"o && o->type == OBJ_COMMIT\" check, now \"if (c)\" (as in could we look up\n> a commit) on \"master\". That in turn seems to go back as far as\n> 9534f40bc42 (Be careful when dereferencing tags., 2005-11-02).\n\nThis sounds unintentional, yes. Other than replying to an old email, you\ncould also write a top-level patch :-)\n"},{"id":"432895","messageId":"20210816230620.3507080-1-jonathantanmy@google.com","threadId":"56228","inReplyTo":"xmqq4kbyfkq4.fsf@gitster.g","subject":"Re: [PATCH] transport: no warning if no server wait-for-done","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T23:06:20Z","receivedAt":"2021-08-16T23:06:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Nieder <jrnieder@gmail.com> writes:\n> \n> >>> -\tif (finish_command(&child)) {\n> >>> -\t\t/*\n> >>> -\t\t * The information that push negotiation provides is useful but\n> >>> -\t\t * not mandatory.\n> >>> -\t\t */\n> >>> -\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n> >>> -\t}\n> >>\n> >> Perhaps like \"optional ancestry negotiation failed---pushing\n> >> normally\" or some phrasing that assures the users that pushing\n> >> without negotiation is perfectly normal?\n> >\n> > The question is what the user will do with this information.\n> >\n> > Will they contact the service provider to ask them to turn on push\n> > negotiation?\n> >\n> > Will they turn off push negotiation because they don't want to waste a\n> > round trip?\n> >\n> > Does what they will do depend on _why_ push negotiation failed?  If it\n> > failed because the server didn't declare the capability and the user\n> > has set push.negotate to true to represent \"I want to live in the\n> > future by using push negotiation wherever it's available\", then the\n> > message is noise.  If it failed due to a bug, then the message is more\n> > relevant to the user --- e.g., should we use a different exit status\n> > to distinguish between these two cases?\n> \n> I was hoping that the \"child\" command being run there gives enough\n> clue before the warning message so that the user would already know\n> (that is where my \"assures the users\" primarily comes from---even we\n> cannot hide that the negotiation poll has failed, the users would be\n> helped by being told that it is OK).\n> \n> If \"child\" does not give enough clue to this codepath (via the exit\n> code) or to the end user (via error messages), we probably would\n> want to fix that for that approach to work.\n\nThe idea of this patch is to treat it like a user wanting to use\nprotocol v2 - if the remote doesn't support it, then the user is\nperfectly fine falling back, and any extra message would be noise\nbecause the user wouldn't take action on it anyway.\n\nIf we think that the user needs to know when push negotiation fails (for\nexample, if we think \"the user enabled it so the user expects it to\nhappen\"), then I agree that we should leave the warning messages in and\nalso print a message saying that push is still happening anyway (which\nwas the approach in the original patch set), and maybe update the\nmessage to something like Junio suggested [1] to clarify what's going\non. But I would think that the user doesn't care - for example, I could\nimagine someone globally enabling it and having pushes become\nautomatically faster as more and more remotes support it.\n\n[1] https://lore.kernel.org/git/xmqqh7fyfrtl.fsf@gitster.g/\n"}]}