[PATCH 0/3] fetch: fix segfault, missing docs in --negotiate-only

The recently added --negotiate-only option segfaults when not combined
with --negotiation-tip=*, this fixes that, and adds documentation for
it.

1/3 is a related fix to save us some work in a codepath checking the
"--negotiate-only" and other passed-in options in send-pack.c.

Ævar Arnfjörð Bjarmason (3):
  send-pack.c: move "no refs in common" abort earlier
  fetch: document the --negotiate-only option
  fetch: fix segfault in --negotiate-only without --negotiation-tip=*

 Documentation/config/fetch.txt  |  3 ++-
 Documentation/fetch-options.txt | 13 +++++++++++--
 builtin/fetch.c                 |  3 +++
 send-pack.c                     | 11 ++++++-----
 t/t5702-protocol-v2.sh          | 17 +++++++++++++++++
 5 files changed, 39 insertions(+), 8 deletions(-)

-- 
2.32.0.619.g53a98c35da0

Move the early return if we have no remote refs in send_pack()
earlier.

When this was added in 4c353e890c0 (Warn when send-pack does nothing,
2005-12-04) one of the first things we'd do was to abort, but as of
cfee10a773b (send-pack/receive-pack: allow errors to be reported back
to pusher., 2005-12-25) we've added numerous server_supports()
conditions that are acted on later in the function, that won't be used
if we don't have remote refs.

Then as of 477673d6f39 (send-pack: support push negotiation,
2021-05-04) we started doing even more work on the assumption that we
had some remote refs to feed to --negotiation-tip=* options.

We only hit this condition if we have nothing to push, so we don't
need to consider "push.negotiate" etc. only to do nothing with that
information.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 send-pack.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 9cb9f716509..5a79e0e7110 100644
--- a/send-pack.c
+++ b/send-pack.c

@@ -486,6 +486,12 @@ int send_pack(struct send_pack_args *args,
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 
+	if (!remote_refs) {
+		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
+			"Perhaps you should specify a branch.\n");
+		return 0;
+	}
+
 	git_config_get_bool("push.negotiate", &push_negotiate);
 	if (push_negotiate)
 		get_commons_through_negotiation(args->url, remote_refs, &commons);

@@ -534,11 +540,6 @@ int send_pack(struct send_pack_args *args,
 		}
 	}
 
-	if (!remote_refs) {
-		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
-			"Perhaps you should specify a branch.\n");
-		return 0;
-	}
 	if (args->atomic && !atomic_supported)
 		die(_("the receiving end does not support --atomic push"));
 

-- 
2.32.0.619.g53a98c35da0

The recent --negotiate-only option would segfault in the call to
oid_array_for_each() in negotiate_using_fetch() unless one or more
--negotiation-tip=* options were provided.

All of the other tests for the feature combine both, but nothing was
checking this assumption, let's do that and add a test for it. Fixes a
bug in 9c1e657a8fd (fetch: teach independent negotiation (no
packfile), 2021-05-04).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/fetch.c        |  3 +++
 t/t5702-protocol-v2.sh | 17 +++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9191620e50c..25740c13df1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1990,6 +1990,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		fetch_config_from_gitmodules(sfjc, rs);
 	}
 
+	if (negotiate_only && !negotiation_tip.nr)
+		die(_("--negotiate-only needs one or more --negotiate-tip=*"));
+
 	if (deepen_relative) {
 		if (deepen_relative < 0)
 			die(_("Negative depth in --deepen is not supported"));

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 66af411057c..920815478c7 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh

@@ -599,6 +599,23 @@ setup_negotiate_only () {
 	test_commit -C client three
 }
 
+test_expect_success 'usage: --negotiate-only without --negotiation-tip' '
+	SERVER="server" &&
+	URI="file://$(pwd)/server" &&
+
+	setup_negotiate_only "$SERVER" "$URI" &&
+
+	cat >err.expect <<-\EOF &&
+	fatal: --negotiate-only needs one or more --negotiate-tip=*
+	EOF
+
+	test_must_fail git -c protocol.version=2 -C client fetch \
+		--negotiate-only \
+		origin 2>err.actual &&
+	cat err &&
+	test_cmp err.expect err.actual
+'
+
 test_expect_success 'file:// --negotiate-only' '
 	SERVER="server" &&
 	URI="file://$(pwd)/server" &&

-- 
2.32.0.619.g53a98c35da0

There was no documentation for the --negotiate-only option added in
9c1e657a8fd (fetch: teach independent negotiation (no packfile),
2021-05-04), only documentation for the related push.negotiation
option added in the following commit in 477673d6f39 (send-pack:
support push negotiation, 2021-05-04).

Let's document it, and update the cross-linking I'd added between
--negotiation-tip=* and 'fetch.negotiationAlgorithm' in
526608284a7 (fetch doc: cross-link two new negotiation options,
2018-08-01).

I think it would be better to say "in common with the remote" here
than "...the server", but the documentation for --negotiation-tip=*
above this talks about "the server", so let's continue doing that in
this related option. See 3390e42adb3 (fetch-pack: support negotiation
tip whitelist, 2018-07-02) for that documentation.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/config/fetch.txt  |  3 ++-
 Documentation/fetch-options.txt | 13 +++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 6af6f5edb27..63748c02b72 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt

@@ -69,7 +69,8 @@ fetch.negotiationAlgorithm::
 	setting defaults to "skipping".
 	Unknown values will cause 'git fetch' to error out.
 +
-See also the `--negotiation-tip` option for linkgit:git-fetch[1].
+See also the `--negotiate-only` and `--negotiation-tip` options to
+linkgit:git-fetch[1].
 
 fetch.showForcedUpdates::
 	Set to false to enable `--no-show-forced-updates` in

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 9e7b4e189ce..e967ff1874c 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt

@@ -62,8 +62,17 @@ The argument to this option may be a glob on ref names, a ref, or the (possibly
 abbreviated) SHA-1 of a commit. Specifying a glob is equivalent to specifying
 this option multiple times, one for each matching ref name.
 +
-See also the `fetch.negotiationAlgorithm` configuration variable
-documented in linkgit:git-config[1].
+See also the `fetch.negotiationAlgorithm` and `push.negotiate`
+configuration variables documented in linkgit:git-config[1], and the
+`--negotiate-only` option below.
+
+--negotiate-only::
+	Do not fetch anything from the server, and instead print the
+	ancestors of the provided `--negotiation-tip=*` arguments,
+	which we have in common with the server.
++
+Internally this is used to implement the `push.negotiate` option, see
+linkgit:git-config[1].
 
 --dry-run::
 	Show what would be done, without making any changes.

-- 
2.32.0.619.g53a98c35da0

I think I inadvertently left a similar line in other tests in this file, but
this shouldn't be in.

Other than that, all the patches look good to me. Thanks for taking a look at this.

I have a patch set [1] that fixes related bugs (but not this one - in
particular, --negotiate-only without --negotiation-tip still fails, so we still
need this patch set). I've tried merging my set into this set; the merge is
clean and all tests still pass, so there shouldn't be any problems with that.

[1] https://lore.kernel.org/git/cover.1624486920.git.jonathantanmy@google.com/

Fix a segfault in recent code, per v1's:
https://lore.kernel.org/git/cover-0.3-00000000000-20210630T163329Z-avarab@gmail.com/

Fixes a stray "cat err" that snuck out of the lab and into the test,
spotted by Jonathan Tan.

Junio: You picked this up as ab/fetch-negotiate-segv-fix and it's
marked as "Will merge to 'next'", but it's not there yet. Hopefully
you'll see this before it's merged down. Sorry about the delay.

Ævar Arnfjörð Bjarmason (3):
  send-pack.c: move "no refs in common" abort earlier
  fetch: document the --negotiate-only option
  fetch: fix segfault in --negotiate-only without --negotiation-tip=*

 Documentation/config/fetch.txt  |  3 ++-
 Documentation/fetch-options.txt | 13 +++++++++++--
 builtin/fetch.c                 |  3 +++
 send-pack.c                     | 11 ++++++-----
 t/t5702-protocol-v2.sh          | 16 ++++++++++++++++
 5 files changed, 38 insertions(+), 8 deletions(-)

Range-diff against v1:
1:  1cd8b98d3d6 = 1:  10375a6484e send-pack.c: move "no refs in common" abort earlier
2:  7ff734ed70a = 2:  f86f4fc0e6f fetch: document the --negotiate-only option
3:  38930024d95 ! 3:  491d72c35a0 fetch: fix segfault in --negotiate-only without --negotiation-tip=*
    @@ t/t5702-protocol-v2.sh: setup_negotiate_only () {
     +	test_must_fail git -c protocol.version=2 -C client fetch \
     +		--negotiate-only \
     +		origin 2>err.actual &&
    -+	cat err &&
     +	test_cmp err.expect err.actual
     +'
     +
-- 
2.32.0.636.g43e71d69cff

Move the early return if we have no remote refs in send_pack()
earlier.

When this was added in 4c353e890c0 (Warn when send-pack does nothing,
2005-12-04) one of the first things we'd do was to abort, but as of
cfee10a773b (send-pack/receive-pack: allow errors to be reported back
to pusher., 2005-12-25) we've added numerous server_supports()
conditions that are acted on later in the function, that won't be used
if we don't have remote refs.

Then as of 477673d6f39 (send-pack: support push negotiation,
2021-05-04) we started doing even more work on the assumption that we
had some remote refs to feed to --negotiation-tip=* options.

We only hit this condition if we have nothing to push, so we don't
need to consider "push.negotiate" etc. only to do nothing with that
information.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 send-pack.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 9cb9f716509..5a79e0e7110 100644
--- a/send-pack.c
+++ b/send-pack.c

@@ -486,6 +486,12 @@ int send_pack(struct send_pack_args *args,
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 
+	if (!remote_refs) {
+		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
+			"Perhaps you should specify a branch.\n");
+		return 0;
+	}
+
 	git_config_get_bool("push.negotiate", &push_negotiate);
 	if (push_negotiate)
 		get_commons_through_negotiation(args->url, remote_refs, &commons);

@@ -534,11 +540,6 @@ int send_pack(struct send_pack_args *args,
 		}
 	}
 
-	if (!remote_refs) {
-		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
-			"Perhaps you should specify a branch.\n");
-		return 0;
-	}
 	if (args->atomic && !atomic_supported)
 		die(_("the receiving end does not support --atomic push"));
 

-- 
2.32.0.636.g43e71d69cff

There was no documentation for the --negotiate-only option added in
9c1e657a8fd (fetch: teach independent negotiation (no packfile),
2021-05-04), only documentation for the related push.negotiation
option added in the following commit in 477673d6f39 (send-pack:
support push negotiation, 2021-05-04).

Let's document it, and update the cross-linking I'd added between
--negotiation-tip=* and 'fetch.negotiationAlgorithm' in
526608284a7 (fetch doc: cross-link two new negotiation options,
2018-08-01).

I think it would be better to say "in common with the remote" here
than "...the server", but the documentation for --negotiation-tip=*
above this talks about "the server", so let's continue doing that in
this related option. See 3390e42adb3 (fetch-pack: support negotiation
tip whitelist, 2018-07-02) for that documentation.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/config/fetch.txt  |  3 ++-
 Documentation/fetch-options.txt | 13 +++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 6af6f5edb27..63748c02b72 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt

@@ -69,7 +69,8 @@ fetch.negotiationAlgorithm::
 	setting defaults to "skipping".
 	Unknown values will cause 'git fetch' to error out.
 +
-See also the `--negotiation-tip` option for linkgit:git-fetch[1].
+See also the `--negotiate-only` and `--negotiation-tip` options to
+linkgit:git-fetch[1].
 
 fetch.showForcedUpdates::
 	Set to false to enable `--no-show-forced-updates` in

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 9e7b4e189ce..e967ff1874c 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt

@@ -62,8 +62,17 @@ The argument to this option may be a glob on ref names, a ref, or the (possibly
 abbreviated) SHA-1 of a commit. Specifying a glob is equivalent to specifying
 this option multiple times, one for each matching ref name.
 +
-See also the `fetch.negotiationAlgorithm` configuration variable
-documented in linkgit:git-config[1].
+See also the `fetch.negotiationAlgorithm` and `push.negotiate`
+configuration variables documented in linkgit:git-config[1], and the
+`--negotiate-only` option below.
+
+--negotiate-only::
+	Do not fetch anything from the server, and instead print the
+	ancestors of the provided `--negotiation-tip=*` arguments,
+	which we have in common with the server.
++
+Internally this is used to implement the `push.negotiate` option, see
+linkgit:git-config[1].
 
 --dry-run::
 	Show what would be done, without making any changes.

-- 
2.32.0.636.g43e71d69cff

The recent --negotiate-only option would segfault in the call to
oid_array_for_each() in negotiate_using_fetch() unless one or more
--negotiation-tip=* options were provided.

All of the other tests for the feature combine both, but nothing was
checking this assumption, let's do that and add a test for it. Fixes a
bug in 9c1e657a8fd (fetch: teach independent negotiation (no
packfile), 2021-05-04).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/fetch.c        |  3 +++
 t/t5702-protocol-v2.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9191620e50c..25740c13df1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1990,6 +1990,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		fetch_config_from_gitmodules(sfjc, rs);
 	}
 
+	if (negotiate_only && !negotiation_tip.nr)
+		die(_("--negotiate-only needs one or more --negotiate-tip=*"));
+
 	if (deepen_relative) {
 		if (deepen_relative < 0)
 			die(_("Negative depth in --deepen is not supported"));

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 66af411057c..78de1ff2ad5 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh

@@ -599,6 +599,22 @@ setup_negotiate_only () {
 	test_commit -C client three
 }
 
+test_expect_success 'usage: --negotiate-only without --negotiation-tip' '
+	SERVER="server" &&
+	URI="file://$(pwd)/server" &&
+
+	setup_negotiate_only "$SERVER" "$URI" &&
+
+	cat >err.expect <<-\EOF &&
+	fatal: --negotiate-only needs one or more --negotiate-tip=*
+	EOF
+
+	test_must_fail git -c protocol.version=2 -C client fetch \
+		--negotiate-only \
+		origin 2>err.actual &&
+	test_cmp err.expect err.actual
+'
+
 test_expect_success 'file:// --negotiate-only' '
 	SERVER="server" &&
 	URI="file://$(pwd)/server" &&

-- 
2.32.0.636.g43e71d69cff