{"thread":{"id":"61718","subject":"[PATCH] push: avoid showing false negotiation errors","startedAt":"2024-07-02T19:57:58Z","lastAt":"2024-07-02T21:52:02Z","messageCount":3,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497990","messageId":"xmqqo77fr2h0.fsf@gitster.g","threadId":"61718","inReplyTo":null,"subject":"[PATCH] push: avoid showing false negotiation errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-02T19:57:47Z","receivedAt":"2024-07-02T19:57:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"git push\" is configured to use the push negotiation, a push of\ndeletion of a branch (without pushing anything else) may end up not\nhaving anything to negotiate for the common ancestor discovery.\n\nIn such a case, we end up making an internal invocation of \"git\nfetch --negotiate-only\" without any \"--negotiate-tip\" parameters\nthat stops the negotiate-only fetch from being run, which by itself\nis not a bad thing (one fewer round-trip), but the end-user sees a\n\"fatal: --negotiate-only needs one or more --negotiation-tip=*\"\nmessage that the user cannot act upon.\n\nTeach \"git push\" to notice the situation and omit performing the\nnegotiate-only fetch to begin with.  One fewer process spawned, one\nfewer \"alarming\" message given the user.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n send-pack.c           | 13 +++++++++++--\n t/t5516-fetch-push.sh | 11 +++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git c/send-pack.c w/send-pack.c\nindex 713da582d7..fa2f5eec17 100644\n--- c/send-pack.c\n+++ w/send-pack.c\n@@ -427,17 +427,26 @@ static void get_commons_through_negotiation(const char *url,\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst struct ref *ref;\n \tint len = the_hash_algo->hexsz + 1; /* hash + NL */\n+\tint nr_negotiation_tip = 0;\n \n \tchild.git_cmd = 1;\n \tchild.no_stdin = 1;\n \tchild.out = -1;\n \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n+\t\tif (!is_null_oid(&ref->new_oid)) {\n+\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\",\n+\t\t\t\t     oid_to_hex(&ref->new_oid));\n+\t\t\tnr_negotiation_tip++;\n+\t\t}\n \t}\n \tstrvec_push(&child.args, url);\n \n+\tif (!nr_negotiation_tip) {\n+\t\tchild_process_clear(&child);\n+\t\treturn;\n+\t}\n+\n \tif (start_command(&child))\n \t\tdie(_(\"send-pack: unable to fork off fetch subprocess\"));\n \ndiff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh\nindex 2e7c0e1648..a3f18404d9 100755\n--- c/t/t5516-fetch-push.sh\n+++ w/t/t5516-fetch-push.sh\n@@ -230,6 +230,17 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \ttest_grep \"push negotiation failed\" err\n '\n \n+test_expect_success 'push deletion with negotiation' '\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/heads/master &&\n+\tgit ls-remote testrepo >ls-remote &&\n+\tgit -c push.negotiate=1 push testrepo \\\n+\t\t:master $the_first_commit:refs/heads/next 2>errors-2 &&\n+\ttest_grep ! \"negotiate-only needs one or \" errors-2 &&\n+\tgit -c push.negotiate=1 push testrepo :next 2>errors-1 &&\n+\ttest_grep ! \"negotiate-only needs one or \" errors-1\n+'\n+\n test_expect_success 'push with negotiation does not attempt to fetch submodules' '\n \tmk_empty submodule_upstream &&\n \ttest_commit -C submodule_upstream submodule_commit &&\n"},{"id":"497998","messageId":"20240702211151.GA120950@coredump.intra.peff.net","threadId":"61718","inReplyTo":"xmqqo77fr2h0.fsf@gitster.g","subject":"Re: [PATCH] push: avoid showing false negotiation errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-02T21:11:51Z","receivedAt":"2024-07-02T21:12:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 02, 2024 at 12:57:47PM -0700, Junio C Hamano wrote:\n\n> When \"git push\" is configured to use the push negotiation, a push of\n> deletion of a branch (without pushing anything else) may end up not\n> having anything to negotiate for the common ancestor discovery.\n> \n> In such a case, we end up making an internal invocation of \"git\n> fetch --negotiate-only\" without any \"--negotiate-tip\" parameters\n> that stops the negotiate-only fetch from being run, which by itself\n> is not a bad thing (one fewer round-trip), but the end-user sees a\n> \"fatal: --negotiate-only needs one or more --negotiation-tip=*\"\n> message that the user cannot act upon.\n> \n> Teach \"git push\" to notice the situation and omit performing the\n> negotiate-only fetch to begin with.  One fewer process spawned, one\n> fewer \"alarming\" message given the user.\n\nMakes sense.\n\n> @@ -427,17 +427,26 @@ static void get_commons_through_negotiation(const char *url,\n>  \tstruct child_process child = CHILD_PROCESS_INIT;\n>  \tconst struct ref *ref;\n>  \tint len = the_hash_algo->hexsz + 1; /* hash + NL */\n> +\tint nr_negotiation_tip = 0;\n>  \n>  \tchild.git_cmd = 1;\n>  \tchild.no_stdin = 1;\n>  \tchild.out = -1;\n>  \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n>  \tfor (ref = remote_refs; ref; ref = ref->next) {\n> -\t\tif (!is_null_oid(&ref->new_oid))\n> -\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> +\t\tif (!is_null_oid(&ref->new_oid)) {\n> +\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\",\n> +\t\t\t\t     oid_to_hex(&ref->new_oid));\n> +\t\t\tnr_negotiation_tip++;\n> +\t\t}\n>  \t}\n>  \tstrvec_push(&child.args, url);\n>  \n> +\tif (!nr_negotiation_tip) {\n> +\t\tchild_process_clear(&child);\n> +\t\treturn;\n> +\t}\n\nOK, this works as advertised. \"nr_negotiation_tip\" is really a boolean\nhere, as we never care about the actual count. I'd probably have written\nit as \"have_negotiation_tip = 1\", but I don't think there is any real\nreason to prefer one over the other.\n\nAnd we can't just check for a non-NULL remote_refs, since we are looking\nfor non-deletions.\n\n> diff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh\n> index 2e7c0e1648..a3f18404d9 100755\n> --- c/t/t5516-fetch-push.sh\n> +++ w/t/t5516-fetch-push.sh\n> @@ -230,6 +230,17 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n>  \ttest_grep \"push negotiation failed\" err\n>  '\n>  \n> +test_expect_success 'push deletion with negotiation' '\n> +\tmk_empty testrepo &&\n> +\tgit push testrepo $the_first_commit:refs/heads/master &&\n> +\tgit ls-remote testrepo >ls-remote &&\n> +\tgit -c push.negotiate=1 push testrepo \\\n> +\t\t:master $the_first_commit:refs/heads/next 2>errors-2 &&\n> +\ttest_grep ! \"negotiate-only needs one or \" errors-2 &&\n> +\tgit -c push.negotiate=1 push testrepo :next 2>errors-1 &&\n> +\ttest_grep ! \"negotiate-only needs one or \" errors-1\n> +'\n\nThe test mostly makes sense, though is the ls-remote bit leftover\ndebugging cruft?\n\n-Peff\n"},{"id":"498006","messageId":"xmqqcynvqx6r.fsf@gitster.g","threadId":"61718","inReplyTo":"20240702211151.GA120950@coredump.intra.peff.net","subject":"Re: [PATCH] push: avoid showing false negotiation errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-02T21:51:56Z","receivedAt":"2024-07-02T21:52:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> diff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh\n>> index 2e7c0e1648..a3f18404d9 100755\n>> --- c/t/t5516-fetch-push.sh\n>> +++ w/t/t5516-fetch-push.sh\n>> @@ -230,6 +230,17 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n>>  \ttest_grep \"push negotiation failed\" err\n>>  '\n>>  \n>> +test_expect_success 'push deletion with negotiation' '\n>> +\tmk_empty testrepo &&\n>> +\tgit push testrepo $the_first_commit:refs/heads/master &&\n>> +\tgit ls-remote testrepo >ls-remote &&\n>> +\tgit -c push.negotiate=1 push testrepo \\\n>> +\t\t:master $the_first_commit:refs/heads/next 2>errors-2 &&\n>> +\ttest_grep ! \"negotiate-only needs one or \" errors-2 &&\n>> +\tgit -c push.negotiate=1 push testrepo :next 2>errors-1 &&\n>> +\ttest_grep ! \"negotiate-only needs one or \" errors-1\n>> +'\n>\n> The test mostly makes sense, though is the ls-remote bit leftover\n> debugging cruft?\n\nThe ls-remote is more of \"forward-looking\" (as opposed to\n\"leftover\") debugging cruft to help future debugging when somebody\nbreaks the tests.  I can remove it, of course, as it is not required\nfor the tests to work correctly.\n\nThanks.\n\n"}]}