Re: [PATCH] push: avoid showing false negotiation errors
- From
Jeff King <peff@peff.net>
- Date
- Jul 2, 2024, 21:11 UTC
- Message-ID
- <20240702211151.GA120950@coredump.intra.peff.net>
- In-Reply-To
- <xmqqo77fr2h0.fsf@gitster.g>
On Tue, Jul 02, 2024 at 12:57:47PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> 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.
Show 25 quoted lines
> @@ -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.
Show 18 quoted lines
> 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