Re: [PATCH v4 2/8] fetch: add --negotiation-restrict option

> 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.
> 
> 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.
> 
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>   Documentation/config/fetch.adoc  |  2 +-
>   Documentation/fetch-options.adoc |  6 +++++-
>   builtin/fetch.c                  | 13 ++++++++-----
>   builtin/pull.c                   |  3 ++-
>   send-pack.c                      |  2 +-
>   t/t5510-fetch.sh                 | 25 +++++++++++++++++++++++++
>   t/t5702-protocol-v2.sh           |  4 ++--
>   transport-helper.c               |  3 ++-
>   8 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
> index cd40db0cad..04ac90912d 100644
> --- a/Documentation/config/fetch.adoc
> +++ b/Documentation/config/fetch.adoc
> @@ -76,7 +76,7 @@
>   	default is `skipping`.  Unknown values will cause `git fetch` to
>   	error out.
>   +
> -See also the `--negotiate-only` and `--negotiation-tip` options to
> +See also the `--negotiate-only` and `--negotiation-restrict` options to
>   linkgit:git-fetch[1].

>   `fetch.showForcedUpdates`::
> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index 81a9d7f9bb..d39cecb446 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.
>   +
> @@ -71,7 +75,7 @@ configuration variables documented in linkgit:git-config[1], and the
>   
>   `--negotiate-only`::
>   	Do not fetch anything from the server, and instead print the
> -	ancestors of the provided `--negotiation-tip=` arguments,
> +	ancestors of the provided `--negotiation-restrict=` arguments,
>   	which we have in common with the server.
>   +
>   This is incompatible with `--recurse-submodules=(yes|on-demand)`.

> 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;
>   }
> @@ -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;
>   }
> @@ -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),
> @@ -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)

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 7e67fdce97..cc6ce485fc 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -996,9 +996,10 @@ int cmd_pull(int argc,
>   		OPT_PASSTHRU('6',  "ipv6", &opt_ipv6, NULL,
>   			N_("use IPv6 addresses only"),
>   			PARSE_OPT_NOARG),
> -		OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
> +		OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
>   			N_("report that we have only objects reachable from this object"),
>   			0),
> +		OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
>   		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,

> diff --git a/send-pack.c b/send-pack.c
> index 67d6987b1c..3d5d36ba3b 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -447,7 +447,7 @@ static void get_commons_through_negotiation(struct repository *r,
>   	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",
> +			strvec_pushf(&child.args, "--negotiation-restrict=%s",
>   				     oid_to_hex(&ref->new_oid));
>   			nr_negotiation_tip++;
>   		}

> 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 \

> diff --git a/transport-helper.c b/transport-helper.c
> index 4d95d84f9e..dd78d40668 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -755,7 +755,8 @@ static int fetch_refs(struct transport *transport,
>   	}
>   
>   	if (data->transport_options.negotiation_tips)
> -		warning("Ignoring --negotiation-tip because the protocol does not support it.");
> +		warning(_("ignoring %s because the protocol does not support it."),
> +			"--negotiation-restrict");