Re: [PATCH v3 2/7] fetch: add --negotiation-restrict option

From: Matthew John Cheetham <mjcheetham@outlook.com>
Date: May 12, 2026, 11:11 UTC
Message-ID: <VI0PR03MB116340D42554FA1D08E51910BC0392@VI0PR03MB11634.eurprd03.prod.outlook.com>
In-Reply-To: <fe875399a851ba27ab193463cb6a1faf62aa6835.1776871546.git.gitgitgadget@gmail.com>

On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:

Show 19 quoted lines

> From: Derrick Stolee <stolee@gmail.com>
> 
> The --negotiation-tip option to 'git fetch' and 'git pull' allows users
> to specify that they want to focus negotiation on a small set of
> references. This is a _restriction_ on the negotiation set, helping to
> focus the negotiation when the ref count is high. However, it doesn't
> allow for the ability to opportunistically select references beyond that
> list.
> 
> This subtle detail that this is a 'maximum set' and not a 'minimum set'
> is not immediately clear from the option name. This makes it more
> complicated to add a new option that provides the complementary behavior
> of a minimum set.
> 
> For now, create a new synonym option, --negotiation-restrict, that
> behaves identically to --negotiation-tip. Update the documentation to
> make it clear that this new name is the preferred option, but we keep
> the old name for compatibility. Mark --negotiation-tip as an alias of the
> new, preferred option.

This motivation reads well. Preparing the new naming convention before introducing the new, complementary option, is the right order to do this in, IMO.

> Update a few warning messages with the new option, but also make them
> translatable with the option name inserted by formatting. At least one
> of these messages will be reused later for a new option.

Nice extra win!

Show 31 quoted lines

> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>   Documentation/fetch-options.adoc |  4 ++++
>   builtin/fetch.c                  | 13 ++++++++-----
>   builtin/pull.c                   |  3 +++
>   t/t5510-fetch.sh                 | 25 +++++++++++++++++++++++++
>   t/t5702-protocol-v2.sh           |  4 ++--
>   5 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index 81a9d7f9bb..c07b85499f 100644
> --- a/Documentation/fetch-options.adoc
> +++ b/Documentation/fetch-options.adoc
> @@ -49,6 +49,7 @@ the current repository has the same history as the source repository.
>   	`.git/shallow`. This option updates `.git/shallow` and accepts such
>   	refs.
>   
> +`--negotiation-restrict=(<commit>|<glob>)`::
>   `--negotiation-tip=(<commit>|<glob>)`::
>   	By default, Git will report, to the server, commits reachable
>   	from all local refs to find common commits in an attempt to
> @@ -58,6 +59,9 @@ the current repository has the same history as the source repository.
>   	local ref is likely to have commits in common with the
>   	upstream ref being fetched.
>   +
> +`--negotiation-restrict` is the preferred name for this option;
> +`--negotiation-tip` is accepted as a synonym.
> ++
>   This option may be specified more than once; if so, Git will report
>   commits reachable from any of the given commits.
>   +

By my eyes it looks like two other references to the old name remain and could also be updated for consistency (since --negotiation-restrict is now the preferred name):

  1. Documentation/fetch-options.adoc, under `--negotiate-only`:
       "ancestors of the provided `--negotiation-tip=` arguments"

  2. Documentation/config/fetch.adoc:
       "See also the `--negotiate-only` and `--negotiation-tip` options"

Of course the old name will still work, so this is more a nit-pick :-)

Show 15 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 4795b2a13c..fc950fe35b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1558,8 +1558,8 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
>   		refs_for_each_ref_ext(get_main_ref_store(the_repository),
>   				      add_oid, oids, &opts);
>   		if (old_nr == oids->nr)
> -			warning("ignoring --negotiation-tip=%s because it does not match any refs",
> -				s);
> +			warning(_("ignoring %s=%s because it does not match any refs"),
> +				"--negotiation-restrict", s);
>   	}
>   	smart_options->negotiation_tips = oids;
>   }

Keeping the option name out of the translation string prevents accidental translation of a fixed symbol - good.

Show 10 quoted lines

> @@ -1599,7 +1599,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
>   		if (transport->smart_options)
>   			add_negotiation_tips(transport->smart_options);
>   		else
> -			warning("ignoring --negotiation-tip because the protocol does not support it");
> +			warning(_("ignoring %s because the protocol does not support it"),
> +				"--negotiation-restrict");
>   	}
>   	return transport;
>   }

Same as above - good.

Show 11 quoted lines

> @@ -2565,8 +2566,9 @@ int cmd_fetch(int argc,
>   			       N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
>   		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
>   		OPT_IPVERSION(&family),
> -		OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
> +		OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
>   				N_("report that we have only objects reachable from this object")),
> +		OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
>   		OPT_BOOL(0, "negotiate-only", &negotiate_only,
>   			 N_("do not fetch a packfile; instead, print ancestors of negotiation tips")),
>   		OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),

Good. Makes the --negotiate-restrict name the primary and the 'tip' the alias, matching the docs' preference.

Keeping the variable named `negotiate_tip` helps reduce churn in this patch (and has no outwardly visible impact anyway). I see a future patch renames the variable - nice choice for reviewability.

Show 10 quoted lines

> @@ -2657,7 +2659,8 @@ int cmd_fetch(int argc,
>   	}
>   
>   	if (negotiate_only && !negotiation_tip.nr)
> -		die(_("--negotiate-only needs one or more --negotiation-tip=*"));
> +		die(_("%s needs one or more %s"), "--negotiate-only",
> +		    "--negotiation-restrict=*");
>   
>   	if (deepen_relative) {
>   		if (deepen_relative < 0)

Much love for i18n!

Show 14 quoted lines

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 7e67fdce97..821cc6699a 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -999,6 +999,9 @@ int cmd_pull(int argc,
>   		OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
>   			N_("report that we have only objects reachable from this object"),
>   			0),
> +		OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
> +			N_("report that we have only objects reachable from this object"),
> +			0),
>   		OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
>   			 N_("check for forced-updates on all updated branches")),
>   		OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,

It's a shame we don't have a nice way to combine the `OPT_ALIAS` and `OPT_PASSTHRU_ARGV` functionality, but it's only a small duplication cost of the repeated definition.

Show 57 quoted lines

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 5dcb4b51a4..dc3ce56d84 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1460,6 +1460,31 @@ EOF
>   	test_cmp fatal-expect fatal-actual
>   '
>   
> +test_expect_success '--negotiation-restrict limits "have" lines sent' '
> +	setup_negotiation_tip server server 0 &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> +		--negotiation-restrict=alpha_1 --negotiation-restrict=beta_1 \
> +		origin alpha_s beta_s &&
> +	check_negotiation_tip
> +'
> +
> +test_expect_success '--negotiation-restrict understands globs' '
> +	setup_negotiation_tip server server 0 &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> +		--negotiation-restrict=*_1 \
> +		origin alpha_s beta_s &&
> +	check_negotiation_tip
> +'
> +
> +test_expect_success '--negotiation-restrict and --negotiation-tip can be mixed' '
> +	setup_negotiation_tip server server 0 &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> +		--negotiation-restrict=alpha_1 \
> +		--negotiation-tip=beta_1 \
> +		origin alpha_s beta_s &&
> +	check_negotiation_tip
> +'
> +
>   test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
>   	git init df-conflict &&
>   	(
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index f826ac46a5..9f6cf4142d 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -869,14 +869,14 @@ setup_negotiate_only () {
>   	test_commit -C client three
>   }
>   
> -test_expect_success 'usage: --negotiate-only without --negotiation-tip' '
> +test_expect_success 'usage: --negotiate-only without --negotiation-restrict' '
>   	SERVER="server" &&
>   	URI="file://$(pwd)/server" &&
>   
>   	setup_negotiate_only "$SERVER" "$URI" &&
>   
>   	cat >err.expect <<-\EOF &&
> -	fatal: --negotiate-only needs one or more --negotiation-tip=*
> +	fatal: --negotiate-only needs one or more --negotiation-restrict=*
>   	EOF
>   
>   	test_must_fail git -c protocol.version=2 -C client fetch \

Looks like this test is the only place asserting the '--negotiate-tip' string literal in the tree - good, no others to update.

Except the two doc cross-references above (nits) this looks good to me.

Thanks, Matthew