[PATCH] push: avoid showing false negotiation errors

[PATCH] push: avoid showing false negotiation errors

[Junio C Hamano]

When "git push" is configured to use the push negotiation, a push of
deletion of a branch (without pushing anything else) may end up not
having anything to negotiate for the common ancestor discovery.

In such a case, we end up making an internal invocation of "git
fetch --negotiate-only" without any "--negotiate-tip" parameters
that stops the negotiate-only fetch from being run, which by itself
is not a bad thing (one fewer round-trip), but the end-user sees a
"fatal: --negotiate-only needs one or more --negotiation-tip=*"
message that the user cannot act upon.

Teach "git push" to notice the situation and omit performing the
negotiate-only fetch to begin with.  One fewer process spawned, one
fewer "alarming" message given the user.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c           | 13 +++++++++++--
 t/t5516-fetch-push.sh | 11 +++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git c/send-pack.c w/send-pack.c
index 713da582d7..fa2f5eec17 100644
--- c/send-pack.c
+++ w/send-pack.c
@@ -427,17 +427,26 @@ static void get_commons_through_negotiation(const char *url,
 	struct child_process child = CHILD_PROCESS_INIT;
 	const struct ref *ref;
 	int len = the_hash_algo->hexsz + 1; /* hash + NL */
+	int nr_negotiation_tip = 0;
 
 	child.git_cmd = 1;
 	child.no_stdin = 1;
 	child.out = -1;
 	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
 	for (ref = remote_refs; ref; ref = ref->next) {
-		if (!is_null_oid(&ref->new_oid))
-			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
+		if (!is_null_oid(&ref->new_oid)) {
+			strvec_pushf(&child.args, "--negotiation-tip=%s",
+				     oid_to_hex(&ref->new_oid));
+			nr_negotiation_tip++;
+		}
 	}
 	strvec_push(&child.args, url);
 
+	if (!nr_negotiation_tip) {
+		child_process_clear(&child);
+		return;
+	}
+
 	if (start_command(&child))
 		die(_("send-pack: unable to fork off fetch subprocess"));
 
diff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh
index 2e7c0e1648..a3f18404d9 100755
--- c/t/t5516-fetch-push.sh
+++ w/t/t5516-fetch-push.sh
@@ -230,6 +230,17 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
 	test_grep "push negotiation failed" err
 '
 
+test_expect_success 'push deletion with negotiation' '
+	mk_empty testrepo &&
+	git push testrepo $the_first_commit:refs/heads/master &&
+	git ls-remote testrepo >ls-remote &&
+	git -c push.negotiate=1 push testrepo \
+		:master $the_first_commit:refs/heads/next 2>errors-2 &&
+	test_grep ! "negotiate-only needs one or " errors-2 &&
+	git -c push.negotiate=1 push testrepo :next 2>errors-1 &&
+	test_grep ! "negotiate-only needs one or " errors-1
+'
+
 test_expect_success 'push with negotiation does not attempt to fetch submodules' '
 	mk_empty submodule_upstream &&
 	test_commit -C submodule_upstream submodule_commit &&

Re: [PATCH] push: avoid showing false negotiation errors

[Jeff King]

On Tue, Jul 02, 2024 at 12:57:47PM -0700, Junio C Hamano wrote:

> When "git push" is configured to use the push negotiation, a push of
> deletion of a branch (without pushing anything else) may end up not
> having anything to negotiate for the common ancestor discovery.
> 
> In such a case, we end up making an internal invocation of "git
> fetch --negotiate-only" without any "--negotiate-tip" parameters
> that stops the negotiate-only fetch from being run, which by itself
> is not a bad thing (one fewer round-trip), but the end-user sees a
> "fatal: --negotiate-only needs one or more --negotiation-tip=*"
> message that the user cannot act upon.
> 
> Teach "git push" to notice the situation and omit performing the
> negotiate-only fetch to begin with.  One fewer process spawned, one
> fewer "alarming" message given the user.

Makes sense.

> @@ -427,17 +427,26 @@ static void get_commons_through_negotiation(const char *url,
>  	struct child_process child = CHILD_PROCESS_INIT;
>  	const struct ref *ref;
>  	int len = the_hash_algo->hexsz + 1; /* hash + NL */
> +	int nr_negotiation_tip = 0;
>  
>  	child.git_cmd = 1;
>  	child.no_stdin = 1;
>  	child.out = -1;
>  	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
>  	for (ref = remote_refs; ref; ref = ref->next) {
> -		if (!is_null_oid(&ref->new_oid))
> -			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
> +		if (!is_null_oid(&ref->new_oid)) {
> +			strvec_pushf(&child.args, "--negotiation-tip=%s",
> +				     oid_to_hex(&ref->new_oid));
> +			nr_negotiation_tip++;
> +		}
>  	}
>  	strvec_push(&child.args, url);
>  
> +	if (!nr_negotiation_tip) {
> +		child_process_clear(&child);
> +		return;
> +	}

OK, this works as advertised. "nr_negotiation_tip" is really a boolean
here, as we never care about the actual count. I'd probably have written
it as "have_negotiation_tip = 1", but I don't think there is any real
reason to prefer one over the other.

And we can't just check for a non-NULL remote_refs, since we are looking
for non-deletions.

> diff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh
> index 2e7c0e1648..a3f18404d9 100755
> --- c/t/t5516-fetch-push.sh
> +++ w/t/t5516-fetch-push.sh
> @@ -230,6 +230,17 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
>  	test_grep "push negotiation failed" err
>  '
>  
> +test_expect_success 'push deletion with negotiation' '
> +	mk_empty testrepo &&
> +	git push testrepo $the_first_commit:refs/heads/master &&
> +	git ls-remote testrepo >ls-remote &&
> +	git -c push.negotiate=1 push testrepo \
> +		:master $the_first_commit:refs/heads/next 2>errors-2 &&
> +	test_grep ! "negotiate-only needs one or " errors-2 &&
> +	git -c push.negotiate=1 push testrepo :next 2>errors-1 &&
> +	test_grep ! "negotiate-only needs one or " errors-1
> +'

The test mostly makes sense, though is the ls-remote bit leftover
debugging cruft?

-Peff

Re: [PATCH] push: avoid showing false negotiation errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> diff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh
>> index 2e7c0e1648..a3f18404d9 100755
>> --- c/t/t5516-fetch-push.sh
>> +++ w/t/t5516-fetch-push.sh
>> @@ -230,6 +230,17 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
>>  	test_grep "push negotiation failed" err
>>  '
>>  
>> +test_expect_success 'push deletion with negotiation' '
>> +	mk_empty testrepo &&
>> +	git push testrepo $the_first_commit:refs/heads/master &&
>> +	git ls-remote testrepo >ls-remote &&
>> +	git -c push.negotiate=1 push testrepo \
>> +		:master $the_first_commit:refs/heads/next 2>errors-2 &&
>> +	test_grep ! "negotiate-only needs one or " errors-2 &&
>> +	git -c push.negotiate=1 push testrepo :next 2>errors-1 &&
>> +	test_grep ! "negotiate-only needs one or " errors-1
>> +'
>
> The test mostly makes sense, though is the ls-remote bit leftover
> debugging cruft?

The ls-remote is more of "forward-looking" (as opposed to
"leftover") debugging cruft to help future debugging when somebody
breaks the tests.  I can remove it, of course, as it is not required
for the tests to work correctly.

Thanks.