From: Matthew John Cheetham Date: Tue, 12 May 2026 11:11:15 GMT Subject: Re: [PATCH v3 2/7] fetch: add --negotiation-restrict option Message-ID: In-Reply-To: On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee > > 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! > Signed-off-by: Derrick Stolee > --- > 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=(|)`:: > `--negotiation-tip=(|)`:: > 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 :-) > 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. > @@ -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. > @@ -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. > @@ -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! > 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. > 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