From: Matthew John Cheetham Date: Tue, 12 May 2026 12:29:32 GMT Subject: Re: [PATCH v3 4/7] remote: add remote.*.negotiationRestrict config Message-ID: In-Reply-To: On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee > > In a previous change, the --negotiation-restrict command-line option of > 'git fetch' was added as a synonym of --negotiation-tips. Both of these > options restrict the set of 'haves' the client can send as part of > negotiation. s/tips/tip/ as per the previous patch comments. Not important either way. > This was previously not available via a configuration option. Add a new > 'remote..negotiationRestrict' multi-valued config option that > updates 'git fetch ' to use these restrictions by default. > > If the user provides even one --negotiation-restrict argument, then the > config is ignored. > > An empty value resets the value list to allow ignoring earlier config > values, such as those that might be set in system or global config. > > Signed-off-by: Derrick Stolee > --- > Documentation/config/remote.adoc | 19 +++++++++++++++++++ > builtin/fetch.c | 21 +++++++++++++++++---- > remote.c | 8 ++++++++ > remote.h | 1 + > t/t5510-fetch.sh | 26 ++++++++++++++++++++++++++ > 5 files changed, 71 insertions(+), 4 deletions(-) > > diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc > index 91e46f66f5..f1d889d03e 100644 > --- a/Documentation/config/remote.adoc > +++ b/Documentation/config/remote.adoc > @@ -107,6 +107,25 @@ priority configuration file (e.g. `.git/config` in a repository) to clear > the values inherited from a lower priority configuration files (e.g. > `$HOME/.gitconfig`). > > +remote..negotiationRestrict:: > + When negotiating with this remote during `git fetch` and `git push`, > + restrict the commits advertised as "have" lines to only those > + reachable from refs matching the given patterns. This multi-valued > + config option behaves like `--negotiation-restrict` on the command > + line. > ++ > +Each value is either an exact ref name (e.g. `refs/heads/release`) or a > +glob pattern (e.g. `refs/heads/release/*`). The pattern syntax is the > +same as for `--negotiation-restrict`. > ++ > +These config values are used as defaults for the `--negotiation-restrict` > +command-line option. If `--negotiation-restrict` (or its synonym > +`--negotiation-tip`) is specified on the command line, then the config > +values are not used. > ++ > +Blank values signal to ignore all previous values, allowing a reset of > +the list from broader config scenarios. > + > remote..followRemoteHEAD:: > How linkgit:git-fetch[1] should handle updates to `remotes//HEAD` > when fetching using the configured refspecs of a remote. You say "during `git fetch` and `git push`", but does `push` actually honour the new config? When the `push.negotiate` config is on then `get_commons_through_negotiation()` from send-pack.c shells out to `git fetch --negotiate-only` with one `--negotiation-tip=` arg per ref being pushed, then the URL. This means the CLI restrict list is always non-empty in the subprocess so in `prepare_transport()` (in the below hunk) the `if (negotiation_restrict.nr)` arm is always taken and the new `else if (remote->negotiation_restrict.nr)` arm is never taken. BUT.. reading ahead I see that patch 7 actually wires up negotiation config for push - so my commentary here will be moot! Do we want to drop the "and `git push`" part from this until patch 7, when it is wired up appropriately? One other suggestion: perhaps we should clarify that `push.negotiate` needs to be set for `remote..negotiationRestrict` to be honoured during pushes? > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 2ba0051d52..a1960e3e0c 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1601,6 +1601,19 @@ static struct transport *prepare_transport(struct remote *remote, int deepen, > else > warning(_("ignoring %s because the protocol does not support it"), > "--negotiation-restrict"); > + } else if (remote->negotiation_restrict.nr) { > + struct string_list_item *item; > + for_each_string_list_item(item, &remote->negotiation_restrict) > + string_list_append(&negotiation_restrict, item->string); > + if (transport->smart_options) > + add_negotiation_restrict_tips(transport->smart_options); > + else { > + struct strbuf config_name = STRBUF_INIT; > + strbuf_addf(&config_name, "remote.%s.negotiationRestrict", remote->name); > + warning(_("ignoring %s because the protocol does not support it"), > + config_name.buf); > + strbuf_release(&config_name); > + } > } > return transport; > } See above - this new arm is not reachable on the push.negotiate=true path until patch 7 wires send-pack up. > @@ -2658,10 +2671,6 @@ int cmd_fetch(int argc, > config.display_format = DISPLAY_FORMAT_PORCELAIN; > } > > - if (negotiate_only && !negotiation_restrict.nr) > - die(_("%s needs one or more %s"), "--negotiate-only", > - "--negotiation-restrict=*"); > - > if (deepen_relative) { > if (deepen_relative < 0) > die(_("negative depth in --deepen is not supported")); > @@ -2749,6 +2758,10 @@ int cmd_fetch(int argc, > if (!remote) > die(_("must supply remote when using --negotiate-only")); > gtransport = prepare_transport(remote, 1, &filter_options); > + if (!gtransport->smart_options || > + !gtransport->smart_options->negotiation_restrict_tips) > + die(_("%s needs one or more %s"), "--negotiate-only", > + "--negotiation-restrict=*"); > if (gtransport->smart_options) { > gtransport->smart_options->acked_commits = &acked_commits; > } else { This new condition fires whenever `gtransport->smart_options` is NULL, i.e. the transport doesn't support smart options. Before this case was handled three lines after this hunk by: } else { warning(_("protocol does not support --negotiate-only, exiting")); result = 1; trace2_region_leave("fetch", "negotiate-only", the_repository); goto cleanup; } What happens now if a user runs --negotiate-only against a non-smart transport is they see an odd message: fatal: --negotiate-only needs one or more --negotiation-restrict=* ..but they may have specified --negotiation-restrict options. Do we instead want &&? if (gtransport->smart_options && !gtransport->smart_options->negotiation_restrict_tips) die(_("%s needs one or more %s"), "--negotiate-only", "--negotiation-restrict=*"); > diff --git a/remote.c b/remote.c > index 7ca2a6501b..166a56408a 100644 > --- a/remote.c > +++ b/remote.c > @@ -152,6 +152,7 @@ static struct remote *make_remote(struct remote_state *remote_state, > refspec_init_push(&ret->push); > refspec_init_fetch(&ret->fetch); > string_list_init_dup(&ret->server_options); > + string_list_init_dup(&ret->negotiation_restrict); > > ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1, > remote_state->remotes_alloc); > @@ -179,6 +180,7 @@ static void remote_clear(struct remote *remote) > FREE_AND_NULL(remote->http_proxy); > FREE_AND_NULL(remote->http_proxy_authmethod); > string_list_clear(&remote->server_options, 0); > + string_list_clear(&remote->negotiation_restrict, 0); > } > > static void add_merge(struct branch *branch, const char *name) > @@ -562,6 +564,12 @@ static int handle_config(const char *key, const char *value, > } else if (!strcmp(subkey, "serveroption")) { > return parse_transport_option(key, value, > &remote->server_options); > + } else if (!strcmp(subkey, "negotiationrestrict")) { > + /* reset list on empty value. */ > + if (!value || !*value) > + string_list_clear(&remote->negotiation_restrict, 0); > + else > + string_list_append(&remote->negotiation_restrict, value); > } else if (!strcmp(subkey, "followremotehead")) { > const char *no_warn_branch; > if (!strcmp(value, "never")) Here we use the 'empty value means reset the list' pattern, but I notice that the `parse_transport_option()` function already supports this reset pattern (and used by serveroption above), with a small difference: if (!value) return config_error_nonbool(var); if (!*value) string_list_clear(transport_options, 0); So NULL is an error, but empty string is 'reset'. Is it worth being consistent with other options that use `parse_transport_options`? > diff --git a/remote.h b/remote.h > index fc052945ee..e6ec37c393 100644 > --- a/remote.h > +++ b/remote.h > @@ -117,6 +117,7 @@ struct remote { > char *http_proxy_authmethod; > > struct string_list server_options; > + struct string_list negotiation_restrict; > > enum follow_remote_head_settings follow_remote_head; > const char *no_warn_branch; > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index dc3ce56d84..eff3ce8e2d 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -1485,6 +1485,32 @@ test_expect_success '--negotiation-restrict and --negotiation-tip can be mixed' > check_negotiation_tip > ' > > +test_expect_success 'remote..negotiationRestrict used as default' ' > + setup_negotiation_tip server server 0 && > + > + # test the reset of the list on an empty value > + git -C client config --add remote.origin.negotiationRestrict alpha_2 && > + git -C client config --add remote.origin.negotiationRestrict "" && > + git -C client config --add remote.origin.negotiationRestrict alpha_1 && > + git -C client config --add remote.origin.negotiationRestrict beta_1 && > + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \ > + origin alpha_s beta_s && > + check_negotiation_tip > +' > + > +test_expect_success 'CLI --negotiation-restrict overrides remote config' ' > + setup_negotiation_tip server server 0 && > + git -C client config --add remote.origin.negotiationRestrict alpha_1 && > + git -C client config --add remote.origin.negotiationRestrict beta_1 && > + ALPHA_1=$(git -C client rev-parse alpha_1) && > + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \ > + --negotiation-restrict=alpha_1 \ > + origin alpha_s beta_s && > + test_grep "fetch> have $ALPHA_1" trace && > + BETA_1=$(git -C client rev-parse beta_1) && > + test_grep ! "fetch> have $BETA_1" trace > +' > + > test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' ' > git init df-conflict && > ( > -- gitgitgadget > General shape of this patch is good. The main thing that tripped me up when reading this patch is the doc claim about push, which only becomes true after patch 7 lands. Thanks, Matthew