fetch: rework negotiation tip options #2085
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,57 @@ priority configuration file (e.g. `.git/config` in a repository) to clear | |
| the values inherited from a lower priority configuration files (e.g. | ||
|
There was a problem hiding this comment. Matthew John Cheetham wrote on the Git mailing list (how to reply to this email): On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee<stolee@gmail.com>
> > 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.<name>.negotiationRestrict' multi-valued config option that
> updates 'git fetch <name>' 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<stolee@gmail.com>
> ---
> 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.<name>.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.<name>.followRemoteHEAD::
> How linkgit:git-fetch[1] should handle updates to `remotes/<name>/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=<oid>` 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.<name>.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.<name>.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,
MatthewThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/12/26 8:29 AM, Matthew John Cheetham wrote:
> On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
> >> From: Derrick Stolee<stolee@gmail.com>
>>
>> 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.
Thanks.
>> +remote.<name>.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.<name>.followRemoteHEAD::
>> How linkgit:git-fetch[1] should handle updates to `remotes/<name>/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=<oid>` 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?
You're right that this documentation is premature about 'git push'.
> One other suggestion: perhaps we should clarify that `push.negotiate`
> needs to be set for `remote.<name>.negotiationRestrict` to be honoured
> during pushes?
Yes. I'll rewrite this to focus on 'git fetch'. Then in patch 7 I can
add a new detail about how to make this behavior be respected in 'git push'.
>> 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=*");
You are right that we want to say "we have smart options but haven't
specified restrict arguments" so we can leave the later if/else to
handle the null smart_options case. But actually, I think that it
would be better to reorganize the conditions altogether:
if (!gtransport->smart_options) {
warning(_("protocol does not support --negotiate-only, "exiting"));
result = 1;
trace2_region_leave("fetch", "negotiate-only", the_repository);
goto cleanup;
}
if (!gtransport->smart_options->negotiation_restrict_tips)
die(_("%s needs one or more %s"), "--negotiate-only",
"--negotiation-restrict=*");
gtransport->smart_options->acked_commits = &acked_commits;
This is easier to reason about:
* If we don't have smart options, then skip out of the negotiation logic.
* If we don't have restrict tips, then die().
* Do the negotiation logic only if the previous two conditions didn't hold.
>> @@ -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`?
Thanks for catching this! Let's be consistent. NULL is likely
impossible in this case, but let's be consistent. It also needs
to return.
Thanks,
-Stolee |
||
| `$HOME/.gitconfig`). | ||
|
|
||
| remote.<name>.negotiationRestrict:: | ||
| When negotiating with this remote during `git fetch`, 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. | ||
| + | ||
| These values also influence negotiation during `git push` if | ||
| `push.negotiate` is enabled. | ||
| + | ||
| Blank values signal to ignore all previous values, allowing a reset of | ||
|
There was a problem hiding this comment. Matthew John Cheetham wrote on the Git mailing list (how to reply to this email): On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> > Add a new 'remote.<name>.negotiationInclude' multi-valued config option that
> provides default values for --negotiation-include when no
> --negotiation-include arguments are specified over the command line. This
> is a mirror of how 'remote.<name>.negotiationRestrict' specifies defaults
> for the --negotiation-restrict arguments.
> > Each value is either an exact ref name or a glob pattern whose tips should
> always be sent as 'have' lines during negotiation. The config values are
> resolved through the same resolve_negotiation_include() codepath as the CLI
> options.
> > This option is additive with the normal negotiation process: the negotiation
> algorithm still runs and advertises its own selected commits, but the refs
> matching the config are sent unconditionally on top of those heuristically
> selected commits.
> > Similar to the negotiationRestrict config, 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 <stolee@gmail.com>
> ---
> Documentation/config/remote.adoc | 27 ++++++++++++++++++
> Documentation/fetch-options.adoc | 4 +++
> builtin/fetch.c | 10 +++++++
> remote.c | 8 ++++++
> remote.h | 1 +
> t/t5510-fetch.sh | 49 ++++++++++++++++++++++++++++++++
> 6 files changed, 99 insertions(+)
This patch is a mirror of patch 4 that added the remote config for
negotiateRestrict. Some of the same comments apply here too:
- reusing `parse_transport_option()` vs inline resetting the list
- values could be commit SHAs as well as refs/globs
> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
> index f1d889d03e..44de6d3c1f 100644
> --- a/Documentation/config/remote.adoc
> +++ b/Documentation/config/remote.adoc
> @@ -126,6 +126,33 @@ values are not used.
> Blank values signal to ignore all previous values, allowing a reset of
> the list from broader config scenarios.
> > +remote.<name>.negotiationInclude::
> + When negotiating with this remote during `git fetch` and `git push`,
> + the client advertises a list of commits that exist locally. In
> + repos with many references, this list of "haves" can be truncated.
> + Depending on data shape, dropping certain references may be
> + expensive. This multi-valued config option specifies ref patterns
> + whose tips should always be sent as "have" commits during fetch
> + negotiation with this remote.
> ++
> +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`.
Should this say "..same as for `--negotiation-include`"?
This way each `remote.<name>.negotiationX` doc cross-references the
corresponding `--negotiation-X` command line option.
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 4316f8d4ea..db73ed5379 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1577,6 +1577,55 @@ test_expect_success '--negotiation-include avoids duplicates with negotiator' '
> test_line_count = 1 matches
> '
> > +test_expect_success 'remote.<name>.negotiationInclude used as default for --negotiation-include' '
> + test_when_finished rm -f trace &&
> + setup_negotiation_tip server server 0 &&
> +
> + # test the reset of the list on an empty value
> + git -C client config --add remote.origin.negotiationInclude refs/tags/alpha_1 &&
> + git -C client config --add remote.origin.negotiationInclude "" &&
> + git -C client config --add remote.origin.negotiationInclude refs/tags/beta_1 &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --negotiation-restrict=alpha_1 \
> + origin alpha_s beta_s &&
> +
> + ALPHA_1=$(git -C client rev-parse alpha_1) &&
> + test_grep "fetch> have $ALPHA_1" trace &&
> + BETA_1=$(git -C client rev-parse beta_1) &&
> + test_grep "fetch> have $BETA_1" trace
> +'
This test sets up the include list as [alpha_1, "", beta_1] which after
the reset should become [beta_1], but the assertions in the test only
check that alpha_1 (sent via the --negotiation-restrict option) and
beta_1 (sent via the include) appear. If the reset of the list didn't
work then the test still passes because alpha_1 is sent via the CLI
option.
> +test_expect_success 'remote.<name>.negotiationInclude works with glob patterns' '
> + test_when_finished rm -f trace &&
> + setup_negotiation_tip server server 0 &&
> +
> + git -C client config --add remote.origin.negotiationInclude "refs/tags/beta_*" &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --negotiation-restrict=alpha_1 \
> + origin alpha_s beta_s &&
> +
> + BETA_1=$(git -C client rev-parse beta_1) &&
> + test_grep "fetch> have $BETA_1" trace &&
> + BETA_2=$(git -C client rev-parse beta_2) &&
> + test_grep "fetch> have $BETA_2" trace
> +'
> +
> +test_expect_success 'CLI --negotiation-include overrides remote.<name>.negotiationInclude' '
> + test_when_finished rm -f trace &&
> + setup_negotiation_tip server server 0 &&
> +
> + git -C client config --add remote.origin.negotiationInclude refs/tags/beta_2 &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --negotiation-restrict=alpha_1 \
> + --negotiation-include=refs/tags/beta_1 \
> + origin alpha_s beta_s &&
> +
> + BETA_1=$(git -C client rev-parse beta_1) &&
> + test_grep "fetch> have $BETA_1" trace &&
> + BETA_2=$(git -C client rev-parse beta_2) &&
> + test_grep ! "fetch> have $BETA_2" trace
> +'
> +
> test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
> git init df-conflict &&
> (
Thanks,
MatthewThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/12/26 10:54 AM, Matthew John Cheetham wrote:
> On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
> This patch is a mirror of patch 4 that added the remote config for
> negotiateRestrict. Some of the same comments apply here too:
> > - reusing `parse_transport_option()` vs inline resetting the list
> > - values could be commit SHAs as well as refs/globs
Will do. Thanks.
>> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
>> index f1d889d03e..44de6d3c1f 100644
>> --- a/Documentation/config/remote.adoc
>> +++ b/Documentation/config/remote.adoc
>> @@ -126,6 +126,33 @@ values are not used.
>> Blank values signal to ignore all previous values, allowing a reset of
>> the list from broader config scenarios.
>> +remote.<name>.negotiationInclude::
>> + When negotiating with this remote during `git fetch` and `git push`,
>> + the client advertises a list of commits that exist locally. In
>> + repos with many references, this list of "haves" can be truncated.
>> + Depending on data shape, dropping certain references may be
>> + expensive. This multi-valued config option specifies ref patterns
>> + whose tips should always be sent as "have" commits during fetch
>> + negotiation with this remote.
>> ++
>> +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`.
> > Should this say "..same as for `--negotiation-include`"?
> > This way each `remote.<name>.negotiationX` doc cross-references the
> corresponding `--negotiation-X` command line option.
Good find. The rest of the description uses *-include.
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 4316f8d4ea..db73ed5379 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -1577,6 +1577,55 @@ test_expect_success '--negotiation-include avoids >> duplicates with negotiator' '
>> test_line_count = 1 matches
>> '
>> +test_expect_success 'remote.<name>.negotiationInclude used as default for -- >> negotiation-include' '
>> + test_when_finished rm -f trace &&
>> + setup_negotiation_tip server server 0 &&
>> +
>> + # test the reset of the list on an empty value
>> + git -C client config --add remote.origin.negotiationInclude refs/tags/ >> alpha_1 &&
>> + git -C client config --add remote.origin.negotiationInclude "" &&
>> + git -C client config --add remote.origin.negotiationInclude refs/tags/ >> beta_1 &&
>> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
>> + --negotiation-restrict=alpha_1 \
>> + origin alpha_s beta_s &&
>> +
>> + ALPHA_1=$(git -C client rev-parse alpha_1) &&
>> + test_grep "fetch> have $ALPHA_1" trace &&
>> + BETA_1=$(git -C client rev-parse beta_1) &&
>> + test_grep "fetch> have $BETA_1" trace
>> +'
> > This test sets up the include list as [alpha_1, "", beta_1] which after
> the reset should become [beta_1], but the assertions in the test only
> check that alpha_1 (sent via the --negotiation-restrict option) and
> beta_1 (sent via the include) appear. If the reset of the list didn't
> work then the test still passes because alpha_1 is sent via the CLI
> option.
Good point. the negotiation-restrict option is making this less clear.
If I point the restrict option at alpha_2, then I think it exercises
things correctly.
Thanks,
-Stolee |
||
| the list from broader config scenarios. | ||
|
|
||
| remote.<name>.negotiationInclude:: | ||
| When negotiating with this remote during `git fetch`, the client | ||
| advertises a list of commits that exist locally. In repos with | ||
| many references, this list of "haves" can be truncated. Depending | ||
| on data shape, dropping certain references may be expensive. This | ||
| multi-valued config option specifies references, commit hashes, | ||
| or ref pattern globs whose tips should always be sent as "have" | ||
| commits during fetch negotiation with this remote. | ||
| + | ||
| Each value is either an exact ref name (e.g. `refs/heads/release`), a | ||
| commit hash, or a glob pattern (e.g. `refs/heads/release/*`). The | ||
| pattern syntax is the same as for `--negotiation-include`. | ||
| + | ||
| These config values are used as defaults for the `--negotiation-include` | ||
| command-line option. If `--negotiation-include` is specified on the | ||
| command line, then the config values are not used. | ||
| + | ||
| This option is additive with the normal negotiation process: the | ||
| negotiation algorithm still runs and advertises its own selected commits, | ||
| but the refs matching `remote.<name>.negotiationInclude` are sent | ||
| unconditionally on top of those heuristically selected commits. This | ||
| option is also used during push negotiation when `push.negotiate` is | ||
| enabled. | ||
| + | ||
| These values also influence negotiation during `git push` if | ||
| `push.negotiate` is enabled. | ||
| + | ||
| Blank values signal to ignore all previous values, allowing a reset of | ||
| the list from broader config scenarios. | ||
|
|
||
| remote.<name>.followRemoteHEAD:: | ||
| How linkgit:git-fetch[1] should handle updates to `remotes/<name>/HEAD` | ||
| when fetching using the configured refspecs of a remote. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
There was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> - 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);
> - warning("ignoring --negotiation-tip because the protocol does not support it");
> + warning(_("ignoring %s because the protocol does not support it"),
> + "--negotiation-restrict");
These are nice touches to make sure translators cannot possibly
botch these option names that must be given verbatim.
> }
> return transport;
> }
> @@ -2567,6 +2568,8 @@ int cmd_fetch(int argc,
> OPT_IPVERSION(&family),
> OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
> N_("report that we have only objects reachable from this object")),
> + OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
> + N_("report that we have only objects reachable from this object")),
Is OPT_ALIAS() suitable for this?
> @@ -2657,7 +2660,7 @@ int cmd_fetch(int argc,
> }
>
> if (negotiate_only && !negotiation_tip.nr)
> - die(_("--negotiate-only needs one or more --negotiation-tip=*"));
> + die(_("--negotiate-only needs one or more --negotiation-restrict=*"));
OK. Shouldn't this also do the "%s" thing?
There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 4/15/26 5:57 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> - 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);
>> - warning("ignoring --negotiation-tip because the protocol does not support it");
>> + warning(_("ignoring %s because the protocol does not support it"),
>> + "--negotiation-restrict");
> > These are nice touches to make sure translators cannot possibly
> botch these option names that must be given verbatim.
>> @@ -2657,7 +2660,7 @@ int cmd_fetch(int argc,
>> }
>>
>> if (negotiate_only && !negotiation_tip.nr)
>> - die(_("--negotiate-only needs one or more --negotiation-tip=*"));
>> + die(_("--negotiate-only needs one or more --negotiation-restrict=*"));
>
> OK. Shouldn't this also do the "%s" thing?
I think I had focused on adding "%s" to strings that were not
previously translated, but adjusting the string under translation
is enough to require retranslation. I should make it easier to
translate, too.
>> }
>> return transport;
>> }
>> @@ -2567,6 +2568,8 @@ int cmd_fetch(int argc,
>> OPT_IPVERSION(&family),
>> OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>> N_("report that we have only objects reachable from this object")),
>> + OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
>> + N_("report that we have only objects reachable from this object")),
> > Is OPT_ALIAS() suitable for this?
I was not aware of this. Thanks for the pointer!
I do plan to make "negotiation-tip" an alias for "negotiation-restrict"
based on the new preference for *-restrict as the "real" option now. Is
that the right way to do this?
Thanks,
-StoleeThere was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Derrick Stolee <stolee@gmail.com> writes:
>>> OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>>> N_("report that we have only objects reachable from this object")),
>>> + OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
>>> + N_("report that we have only objects reachable from this object")),
>>
>> Is OPT_ALIAS() suitable for this?
>
> I was not aware of this. Thanks for the pointer!
>
> I do plan to make "negotiation-tip" an alias for "negotiation-restrict"
> based on the new preference for *-restrict as the "real" option now. Is
> that the right way to do this?
Let's see.
$ git grep OPT_ALIAS builtin/clone.c
builtin/clone.c: OPT_ALIAS(0, "recursive", "recurse-submodules"),
$ git clone -h
usage: git clone [<options>] [--] <repo> [<dir>]
-v, --[no-]verbose be more verbose
-q, --[no-]quiet be more quiet
...
--[no-]recurse-submodules[=<pathspec>]
initialize submodules in the clone
--[no-]recursive[=<pathspec>]
alias of --recurse-submodules
...
I think we gave the operation the name "recursive", with a common
short sightedness that anything we are adding "recursive" for is the
only kind of recursiveness, and then prepared for a future where
things other than submodules can also be sources of recursiveness by
making "recurse-submodules" the official name, while still allowing
historical name as the synonym.
In this case, if "-restrict" will become the official name, it
should be listed first, and then the historical name should be made
its alias.
So
OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, ...),
OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
would be the right combination in the correct order, I think.
Mention the official thing first, and then tell that another thing
is an alias to what the readers have already seen after that (e.g.,
c28b036f (clone: reorder --recursive/--recurse-submodules,
2020-03-16)).
There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 4/20/2026 6:32 AM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
>>>> OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>>>> N_("report that we have only objects reachable from this object")),
>>>> + OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, N_("revision"),
>>>> + N_("report that we have only objects reachable from this object")),
>>>
>>> Is OPT_ALIAS() suitable for this?
>>
>> I was not aware of this. Thanks for the pointer!
>>
>> I do plan to make "negotiation-tip" an alias for "negotiation-restrict"
>> based on the new preference for *-restrict as the "real" option now. Is
>> that the right way to do this?
>
> Let's see.
...
> So
>
> OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_tip, ...),
> OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
>
> would be the right combination in the correct order, I think.
> Mention the official thing first, and then tell that another thing
> is an alias to what the readers have already seen after that (e.g.,
> c28b036f (clone: reorder --recursive/--recurse-submodules,
> 2020-03-16)).
Thanks! This is indeed what I have in my local copy in preparation
for v3. It helps to have early confirmation about this.
-Stolee
There was a problem hiding this comment. Matthew John Cheetham wrote on the Git mailing list (how to reply to this email): On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
> 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!
> 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 :-)
> 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,
MatthewThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/12/26 7:11 AM, Matthew John Cheetham wrote:
> On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>> --- 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 :-)
Thanks for catching these! I will make the correct updates in the
next version.
>> 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.
Actually, I just missed that I should use OPT_ALIAS in 'pull' as well as
how it's used in 'fetch'. Will fix.
Thanks,
-Stolee |
||
| 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. | ||
| + | ||
|
|
@@ -69,9 +73,32 @@ See also the `fetch.negotiationAlgorithm` and `push.negotiate` | |
| configuration variables documented in linkgit:git-config[1], and the | ||
|
There was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +`--negotiation-require=<revision>`::
> + Ensure that the given ref tip is always sent as a "have" line
> + during fetch negotiation, regardless of what the negotiation
> + algorithm selects. This is useful to guarantee that common
> + history reachable from specific refs is always considered, even
> + when `--negotiation-restrict` restricts the set of tips or when
> + the negotiation algorithm would otherwise skip them.
> ++
> +This option may be specified more than once; if so, each ref is sent
> +unconditionally.
> ++
> +The argument may be an exact ref name (e.g. `refs/heads/release`) or a
> +glob pattern (e.g. `refs/heads/release/{asterisk}`). The pattern syntax
> +is the same as for `--negotiation-restrict`.
> ++
> +If `--negotiation-restrict` is used, the have set is first restricted by
> +that option and then increased to include the tips specified by
> +`--negotiation-require`.
Very readable. Nice.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 57b2b667ff..b60652e6b1 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -99,6 +99,7 @@ static struct transport *gsecondary;
> static struct refspec refmap = REFSPEC_INIT_FETCH;
> static struct string_list server_options = STRING_LIST_INIT_DUP;
> static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
> +static struct string_list negotiation_require = STRING_LIST_INIT_NODUP;
I thought _tip was renamed to _restrict in an earlier step, but that
was only in the transport in [3/7]. Perhaps we want to rename the
file-scope static variable negotiation_tip to negotiation_restrict
in an earlier step, like in [2/7]?
> + for_each_string_list_item(item, negotiation_require) {
> + if (!has_glob_specials(item->string)) {
> + struct object_id oid;
> + if (repo_get_oid(the_repository, item->string, &oid))
> + continue;
The configuration (or command line) says --nego-require=refs/heads/main
but this old repository only has refs/heads/master; we do not want
to error out in such a case.
Is it true, though? nego-{require,restrict} feels quite tied to
each project and unless the configuration or command line options
are applied blindly regardless of the project, such an error should
not happen. Perhaps the user who gives a command line option
"--nego-require=refs/heads/naster" may want to be reminded of a
possible typo?
> + if (!odb_has_object(the_repository->objects, &oid, 0))
> + continue;
This is a bit curious. When does the first condition holds but not
the second? A lazy clone whose ref-tip contains a missing commit
promised by somebody else?
In the presense of "promised objects are allowed to be missing"
rule, silently skipping a missing object here is certainly
conservative, but this is not an object that is buried deep in a
tree hierarchy, but the top-level commit or tag that is directly
pointed at by a ref, isn't it? I am a bit uneasy that we ignore
such potential repository corruption (i.e., a missing object may not
be something a promisor remote promised but simply missing).
> @@ -474,7 +511,25 @@ static int find_common(struct fetch_negotiator *negotiator,
> trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
> flushes = 0;
> retval = -1;
> +
> + /* Send unconditional haves from --negotiation-require */
> + resolve_negotiation_require(args->negotiation_require,
> + &negotiation_require_oids);
> + if (oidset_size(&negotiation_require_oids)) {
> + struct oidset_iter iter;
> + oidset_iter_init(&negotiation_require_oids, &iter);
> +
> + while ((oid = oidset_iter_next(&iter))) {
> + packet_buf_write(&req_buf, "have %s\n",
> + oid_to_hex(oid));
> + print_verbose(args, "have %s", oid_to_hex(oid));
> + }
> + }
OK. I think it makes sense to send these early. We have already
dealt with the usual "tips" by calling mark_tips() way earlier, but
that hasn't produced any "have" yet, and these will go before the
ones from traversal. We do not traverse from these "require" and
that may be why these are not called "_tips"?
And sending these early means the other side has much less chance to
say "we've heard enough, stop!", so in a sense they are of much
higher priority "have"s (I wonder what happens when they do want to
say "stop!" while we are giving a lot of "have" from this loop,
though).
> while ((oid = negotiator->next(negotiator))) {
> + /* avoid duplicate oids from --negotiation-require */
> + if (oidset_contains(&negotiation_require_oids, oid))
> + continue;
If objects rechable from "require" are traversed like others, then
this "avoid duplicate" would become unnecessary, right?
There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 4/15/26 3:50 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
...
>> +static struct string_list negotiation_require = STRING_LIST_INIT_NODUP;
> > I thought _tip was renamed to _restrict in an earlier step, but that
> was only in the transport in [3/7]. Perhaps we want to rename the
> file-scope static variable negotiation_tip to negotiation_restrict
> in an earlier step, like in [2/7]?
This one was missed but will be fixed in v3.
>> + for_each_string_list_item(item, negotiation_require) {
>> + if (!has_glob_specials(item->string)) {
>> + struct object_id oid;
>> + if (repo_get_oid(the_repository, item->string, &oid))
>> + continue;
> > The configuration (or command line) says --nego-require=refs/heads/main
> but this old repository only has refs/heads/master; we do not want
> to error out in such a case.
> > Is it true, though? nego-{require,restrict} feels quite tied to
> each project and unless the configuration or command line options
> are applied blindly regardless of the project, such an error should
> not happen. Perhaps the user who gives a command line option
> "--nego-require=refs/heads/naster" may want to be reminded of a
> possible typo?
You're right here. We shouldn't die() on a bad ref passed this way.
This should be a best-effort attempt to include a "have" and continue
normally if it isn't found.
>> + if (!odb_has_object(the_repository->objects, &oid, 0))
>> + continue;
> > This is a bit curious. When does the first condition holds but not
> the second? A lazy clone whose ref-tip contains a missing commit
> promised by somebody else?
Good point. This would occur if a ref exists but points to a missing
object, which should mean the repo is corrupt and we can't trust that
the fetch will succeed (or should).
> In the presense of "promised objects are allowed to be missing"
> rule, silently skipping a missing object here is certainly
> conservative, but this is not an object that is buried deep in a
> tree hierarchy, but the top-level commit or tag that is directly
> pointed at by a ref, isn't it? I am a bit uneasy that we ignore
> such potential repository corruption (i.e., a missing object may not
> be something a promisor remote promised but simply missing).
I'll update this to be an error.
>> @@ -474,7 +511,25 @@ static int find_common(struct fetch_negotiator *negotiator,
>> trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
>> flushes = 0;
>> retval = -1;
>> +
>> + /* Send unconditional haves from --negotiation-require */
>> + resolve_negotiation_require(args->negotiation_require,
>> + &negotiation_require_oids);
>> + if (oidset_size(&negotiation_require_oids)) {
>> + struct oidset_iter iter;
>> + oidset_iter_init(&negotiation_require_oids, &iter);
>> +
>> + while ((oid = oidset_iter_next(&iter))) {
>> + packet_buf_write(&req_buf, "have %s\n",
>> + oid_to_hex(oid));
>> + print_verbose(args, "have %s", oid_to_hex(oid));
>> + }
>> + }
> > OK. I think it makes sense to send these early. We have already
> dealt with the usual "tips" by calling mark_tips() way earlier, but
> that hasn't produced any "have" yet, and these will go before the
> ones from traversal. We do not traverse from these "require" and
> that may be why these are not called "_tips"?
It is correct that these required haves are not plugged into the
walk.
> And sending these early means the other side has much less chance to
> say "we've heard enough, stop!", so in a sense they are of much
> higher priority "have"s (I wonder what happens when they do want to
> say "stop!" while we are giving a lot of "have" from this loop,
> though).
I believe that we don't give the server an opportunity to say "stop"
until we've completed a "round" (see the 'if (flush_at <= ++count)'
case).
With this in mind, I should be incrementing 'count' while sending
the required haves.
>> while ((oid = negotiator->next(negotiator))) {
>> + /* avoid duplicate oids from --negotiation-require */
>> + if (oidset_contains(&negotiation_require_oids, oid))
>> + continue;
> > If objects rechable from "require" are traversed like others, then
> this "avoid duplicate" would become unnecessary, right?
Yes, if we add the required things to the traversal, then we wouldn't
worry about duplicates. We'd also need to do things in a different
way:
1. The negotiator has a next() method that could do a number of
things, but is responsible for walking history and ignoring IDs
that are reachable from already-emitted haves.
2. This _could_ help us avoid sending required ID if we have
emitted a have that could reach that ID.
3. However, this requires waiting until we flush a round of haves
before determining if we should send the required IDs based on
the walk to this point.
Perhaps the better way to incorporate things together would be to
mark the required IDs as COMMON as we emit them, which would signal
to the negotiation walks that they should not re-emit it as a have
(but since we don't add SEEN, they still walk through the commit to
its later ancestors).
Thanks,
-Stolee
There was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Apr 15, 2026 at 03:14:24PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> Add a new --negotiation-require option to 'git fetch', which ensures
> that certain ref tips are always sent as 'have' lines during fetch
> negotiation, regardless of what the negotiation algorithm selects.
When reading "--negotiation-require" my mind immediately shifts towards
a mode where we require the remote to have a specific reference, and if
not we'll abort. That's of course not what you're proposing here, but I
would think that I may not be the only person making that connection.
Would an alternative like "--negotiation-include" or
"--negotiation-expand" be better?
> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index c07b85499f..85ffc5b32b 100644
> --- a/Documentation/fetch-options.adoc
> +++ b/Documentation/fetch-options.adoc
> @@ -73,6 +73,25 @@ See also the `fetch.negotiationAlgorithm` and `push.negotiate`
> configuration variables documented in linkgit:git-config[1], and the
> `--negotiate-only` option below.
>
> +`--negotiation-require=<revision>`::
> + Ensure that the given ref tip is always sent as a "have" line
> + during fetch negotiation, regardless of what the negotiation
> + algorithm selects. This is useful to guarantee that common
> + history reachable from specific refs is always considered, even
> + when `--negotiation-restrict` restricts the set of tips or when
> + the negotiation algorithm would otherwise skip them.
> ++
> +This option may be specified more than once; if so, each ref is sent
> +unconditionally.
> ++
> +The argument may be an exact ref name (e.g. `refs/heads/release`) or a
> +glob pattern (e.g. `refs/heads/release/{asterisk}`). The pattern syntax
> +is the same as for `--negotiation-restrict`.
> ++
> +If `--negotiation-restrict` is used, the have set is first restricted by
> +that option and then increased to include the tips specified by
> +`--negotiation-require`.
This interaction makes sense. You can basically say "send only local
branches, but please _also_ send that one particular ref over there".
> diff --git a/fetch-pack.c b/fetch-pack.c
> index baf239adf9..a0029253f1 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -474,7 +511,25 @@ static int find_common(struct fetch_negotiator *negotiator,
> trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
> flushes = 0;
> retval = -1;
> +
> + /* Send unconditional haves from --negotiation-require */
> + resolve_negotiation_require(args->negotiation_require,
> + &negotiation_require_oids);
> + if (oidset_size(&negotiation_require_oids)) {
> + struct oidset_iter iter;
> + oidset_iter_init(&negotiation_require_oids, &iter);
> +
> + while ((oid = oidset_iter_next(&iter))) {
> + packet_buf_write(&req_buf, "have %s\n",
> + oid_to_hex(oid));
> + print_verbose(args, "have %s", oid_to_hex(oid));
> + }
> + }
Okay, so here we now unconditionally send our requested object IDs.
One thing I was wondering is whether we need to flush eventually. It can
happen that the user specifies millions of refs, either intentionally or
by accident. But I guess the answer might be "no", as the intent of the
feature is that we indeed want to send all of those to the remote side,
and the remote is being asked to consider all of those OIDs.
PatrickThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 4/20/2026 4:11 AM, Patrick Steinhardt wrote:
> On Wed, Apr 15, 2026 at 03:14:24PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> Add a new --negotiation-require option to 'git fetch', which ensures
>> that certain ref tips are always sent as 'have' lines during fetch
>> negotiation, regardless of what the negotiation algorithm selects.
>
> When reading "--negotiation-require" my mind immediately shifts towards
> a mode where we require the remote to have a specific reference, and if
> not we'll abort. That's of course not what you're proposing here, but I
> would think that I may not be the only person making that connection.
>
> Would an alternative like "--negotiation-include" or
> "--negotiation-expand" be better?
"include" does sound good to me. I'm open to it. I'll let this idea
stew and try prepping my local branch in this direction.
>> + /* Send unconditional haves from --negotiation-require */
>> + resolve_negotiation_require(args->negotiation_require,
>> + &negotiation_require_oids);
>> + if (oidset_size(&negotiation_require_oids)) {
>> + struct oidset_iter iter;
>> + oidset_iter_init(&negotiation_require_oids, &iter);
>> +
>> + while ((oid = oidset_iter_next(&iter))) {
>> + packet_buf_write(&req_buf, "have %s\n",
>> + oid_to_hex(oid));
>> + print_verbose(args, "have %s", oid_to_hex(oid));
>> + }
>> + }
>
> Okay, so here we now unconditionally send our requested object IDs.
>
> One thing I was wondering is whether we need to flush eventually. It can
> happen that the user specifies millions of refs, either intentionally or
> by accident. But I guess the answer might be "no", as the intent of the
> feature is that we indeed want to send all of those to the remote side,
> and the remote is being asked to consider all of those OIDs.
The idea is indeed to send all of the requested OIDs, but this does
present an interesting behavior where the Git client can allow the
user to misconfigure themselves to send larger-than-normal negotiation
requests. Previously, the client would protect the negotiation with a
maximum set of haves.
Is there any concern about this becoming a vector for increased load
on servers?
Would it be good to have some kind of advice message when the config
matches a set of haves that we think is too large? That would maybe
be a way to help users get out of a self-made problem.
Thanks,
-Stolee
There was a problem hiding this comment. Matthew John Cheetham wrote on the Git mailing list (how to reply to this email): On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> > Add a new --negotiation-include option to 'git fetch', which ensures
> that certain ref tips are always sent as 'have' lines during fetch
> negotiation, regardless of what the negotiation algorithm selects.
> > This is useful when the repository has a large number of references, so
> the normal negotiation algorithm truncates the list. This is especially
> important in repositories with long parallel commit histories. For
> example, a repo could have a 'dev' branch for development and a
> 'release' branch for released versions. If the 'dev' branch isn't
> selected for negotiation, then it's not a big deal because there are
> many in-progress development branches with a shared history. However, if
> 'release' is not selected for negotiation, then the server may think
> that this is the first time the client has asked for that reference,
> causing a full download of its parallel commit history (and any extra
> data that may be unique to that branch). This is based on a real example
> where certain fetches would grow to 60+ GB when a release branch
> updated.
> > This option is a complement to --negotiation-restrict, which reduces the
> negotiation ref set to a specific list. In the earlier example, using
> --negotiation-restrict to focus the negotiation to 'dev' and 'release'
> would avoid those problematic downloads, but would still not allow
> advertising potentially-relevant user brances. In this way, the
> 'include' version solves the problem I mention while allowing
> negotiation to pick other references opportunistically. The two options
> can also be combined to allow the best of both worlds.
Nice explanation and motivation for the need of such as feature.
One small typo: s/brances/branches/
> The argument may be an exact ref name or a glob pattern. Non-existent
> refs are silently ignored. This behavior is also updated in the ref matching
> logic for the related --negotiation-restrict option to match.
Calling out the intent for the behaviour change (non-existent refs are
silently ignored). This is an important point.
> The implementation outputs the requested objects as haves before the
> negotiation algorithm kicks in and performs a priority-queue walk from the
> tip commits. In order to avoid duplicates, we mark the requested objects as
> COMMON so they (and their descendants) are not output by the negotiator. The
> negotiator still outputs at least one have before a round is flushed, when
> the server could ACK to stop the negotiation.
> > Also add --negotiation-include to 'git pull' passthrough options.
> > Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> Documentation/fetch-options.adoc | 19 ++++++
> builtin/fetch.c | 16 ++++-
> builtin/pull.c | 3 +
> fetch-pack.c | 112 +++++++++++++++++++++++++++++--
> fetch-pack.h | 10 ++-
> t/t5510-fetch.sh | 66 ++++++++++++++++++
> transport.c | 4 +-
> transport.h | 6 ++
> 8 files changed, 227 insertions(+), 9 deletions(-)
> > diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index c07b85499f..decc7f6abd 100644
> --- a/Documentation/fetch-options.adoc
> +++ b/Documentation/fetch-options.adoc
> @@ -73,6 +73,25 @@ See also the `fetch.negotiationAlgorithm` and `push.negotiate`
> configuration variables documented in linkgit:git-config[1], and the
> `--negotiate-only` option below.
> > +`--negotiation-include=<revision>`::
> + Ensure that the given ref tip is always sent as a "have" line
> + during fetch negotiation, regardless of what the negotiation
> + algorithm selects. This is useful to guarantee that common
> + history reachable from specific refs is always considered, even
> + when `--negotiation-restrict` restricts the set of tips or when
> + the negotiation algorithm would otherwise skip them.
> ++
> +This option may be specified more than once; if so, each ref is sent
> +unconditionally.
> ++
> +The argument may be an exact ref name (e.g. `refs/heads/release`) or a
> +glob pattern (e.g. `refs/heads/release/{asterisk}`). The pattern syntax
> +is the same as for `--negotiation-restrict`.
> ++
> +If `--negotiation-restrict` is used, the have set is first restricted by
> +that option and then increased to include the tips specified by
> +`--negotiation-include`.
> +
The placeholder `<revision>` and the description in the body of "ref
name or glob" slightly disagree with each other. The `--negotiation-restrict` docs use `(<commit>|<glob>)` in the syntax definition and
"a glob on ref names, a ref, or .. SHA-1 of a commit".
`resolve_negotiation_include()` calls `repo_get_oid()` for non-globs
so bare OIDs and abbreviated SHAs work too. Perhaps consider aligning the syntaxes, and mention that OIDs work too.
> `--negotiate-only`::
> Do not fetch anything from the server, and instead print the
> ancestors of the provided `--negotiation-tip=` arguments,
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a1960e3e0c..ef50e2fbe9 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -99,6 +99,7 @@ static struct transport *gsecondary;
> static struct refspec refmap = REFSPEC_INIT_FETCH;
> static struct string_list server_options = STRING_LIST_INIT_DUP;
> static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP;
> +static struct string_list negotiation_include = STRING_LIST_INIT_NODUP;
> > struct fetch_config {
> enum display_format display_format;
> @@ -1547,10 +1548,14 @@ static void add_negotiation_restrict_tips(struct git_transport_options *smart_op
> int old_nr;
> if (!has_glob_specials(s)) {
> struct object_id oid;
> +
> + /* Ignore missing reference. */
> if (repo_get_oid(the_repository, s, &oid))
> - die(_("%s is not a valid object"), s);
> + continue;
> + /* Fail on missing object pointed by ref. */
> if (!odb_has_object(the_repository->objects, &oid, 0))
> die(_("the object %s does not exist"), s);
> +
> oid_array_append(oids, &oid);
> continue;
> }
This is the change in behaviour - unresolvable revs were a fatal error
and are now silently ignored.
Note that t5510 '--negotiation-tip rejects missing OIDs' still passes
because it uses an all-zero OID, which parses as a valid hex string,
and dies on the second check "object does not exist". Using something
like `--negotiation-tip=notreal` that previously would error will now
silently be ignored.
Is it worth another test? (invalid object vs not exists)?
> @@ -1615,6 +1620,13 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
> strbuf_release(&config_name);
> }
> }
> + if (negotiation_include.nr) {
> + if (transport->smart_options)
> + transport->smart_options->negotiation_include = &negotiation_include;
> + else
> + warning(_("ignoring %s because the protocol does not support it"),
> + "--negotiation-include");
> + }
> return transport;
> }
There is a difference between the existing `--negotiation-restrict`
option and the new `--negotiation-include` option. Patch 3's commit
message says:
"The 'tips' part is kept because this is an oid_array in the transport
layer. This requires the builtin to handle parsing refs into
collections of oids so the transport layer can handle this cleaner
form of the data."
The new option passes the raw `string_list` to the transport layer and
lets it resolve it instead. If the transport layer now learns how to
resolve refs to oids, why not for tips/restrict?
Would it be easier for future readers for these complementary options
to resolve their inputs at the same layer? Or at least call out why:
"would prefer raw tips but for back-compat we resolve in the built-in"
for example.
> @@ -2582,6 +2594,8 @@ int cmd_fetch(int argc,
> OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_restrict, N_("revision"),
> N_("report that we have only objects reachable from this object")),
> OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
> + OPT_STRING_LIST(0, "negotiation-include", &negotiation_include, N_("revision"),
> + N_("ensure this ref is always sent as a negotiation have")),
> 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),
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 821cc6699a..86c85b60ef 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1002,6 +1002,9 @@ int cmd_pull(int argc,
> OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
> N_("report that we have only objects reachable from this object"),
> 0),
> + OPT_PASSTHRU_ARGV(0, "negotiation-include", &opt_fetch, N_("revision"),
> + N_("ensure this ref is always sent as a negotiation have"),
> + 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,
> diff --git a/fetch-pack.c b/fetch-pack.c
> index baf239adf9..8b080b0080 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -25,6 +25,7 @@
> #include "oidset.h"
> #include "packfile.h"
> #include "odb.h"
> +#include "object-name.h"
> #include "path.h"
> #include "connected.h"
> #include "fetch-negotiator.h"
> @@ -332,6 +333,48 @@ static void send_filter(struct fetch_pack_args *args,
> }
> }
> > +static int add_oid_to_oidset(const struct reference *ref, void *cb_data)
> +{
> + struct oidset *set = cb_data;
> + if (!odb_has_object(the_repository->objects, ref->oid, 0))
> + die(_("the object %s does not exist"), oid_to_hex(ref->oid));
> + oidset_insert(set, ref->oid);
> + return 0;
> +}
> +
> +static void resolve_negotiation_include(const struct string_list *negotiation_include,
> + struct oidset *result)
> +{
> + struct string_list_item *item;
> +
> + if (!negotiation_include || !negotiation_include->nr)
> + return;
> +
> + for_each_string_list_item(item, negotiation_include) {
> + if (!has_glob_specials(item->string)) {
> + struct object_id oid;
> +
> + /* Ignore missing reference. */
> + if (repo_get_oid(the_repository, item->string, &oid))
> + continue;
> +
> + /* Fail on missing object pointed by ref. */
> + if (!odb_has_object(the_repository->objects, &oid, 0))
> + die(_("the object %s does not exist"),
> + item->string);
> +
> + oidset_insert(result, &oid);
> + } else {
> + struct refs_for_each_ref_options opts = {
> + .pattern = item->string,
> + };
> + refs_for_each_ref_ext(
> + get_main_ref_store(the_repository),
> + add_oid_to_oidset, result, &opts);
> + }
> + }
> +}
> +
`resolve_negotiation_include()` is basically doing the same as
`add_negotiation_restrict_tips()` except outputting to an `oidset`
vs `oid_array`. This is a result of the difference in ref resolution
layer between `--negotiation-restrict/tip` and `-include`.
> static int find_common(struct fetch_negotiator *negotiator,
> struct fetch_pack_args *args,
> int fd[2], struct object_id *result_oid,
> @@ -347,6 +390,7 @@ static int find_common(struct fetch_negotiator *negotiator,
> struct strbuf req_buf = STRBUF_INIT;
> size_t state_len = 0;
> struct packet_reader reader;
> + struct oidset negotiation_include_oids = OIDSET_INIT;
> > if (args->stateless_rpc && multi_ack == 1)
> die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
> @@ -474,6 +518,33 @@ static int find_common(struct fetch_negotiator *negotiator,
> trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
> flushes = 0;
> retval = -1;
> +
> + /* Send unconditional haves from --negotiation-include */
> + resolve_negotiation_include(args->negotiation_include,
> + &negotiation_include_oids);
> + if (oidset_size(&negotiation_include_oids)) {
> + struct oidset_iter iter;
> + oidset_iter_init(&negotiation_include_oids, &iter);
> +
> + while ((oid = oidset_iter_next(&iter))) {
> + struct commit *commit;
> + packet_buf_write(&req_buf, "have %s\n",
> + oid_to_hex(oid));
> + print_verbose(args, "have %s", oid_to_hex(oid));
> + count++;
> +
> + /*
> + * If this is a commit, then mark as COMMON to
> + * avoid the negotiator also outputting it as
> + * a have.
> + */
> + commit = lookup_commit(the_repository, oid);
> + if (commit &&
> + !repo_parse_commit(the_repository, commit))
> + commit->object.flags |= COMMON;
> + }
> + }
> +
I want to make sure I understand the COMMON pre-marking before
commenting further on this patch. My understanding is there are actually
two different COMMON bits in the tree, one defined in fetch-pack.c
(bit 6) and one in negotiator/default.c (bit 2):
- fetch-pack.c's COMMON (bit 6) is set after a server ACK confirms an
OID is common with us and is read to decide when we've established
enough common ground to terminate negotiation. This is not consulted
in find_common().
- negotiator/default.c's COMMON (bit 2) is a book-keeping flag used by
`get_rev()` to decide if we skip emitting a commit as a 'have'.
Since we're in fetch-pack.c here, the `commit->object.flags |= COMMON`
line is setting bit 6. The `get_rev()` call in negotiator/default.c
never checks bit 6, only bit 2. As far as I can tell, this mark won't
suppress the negotiator from emitting another 'have' line in the
protocol v0/v1 paths in `find_common()`.
The v2 path doesn't touch the flags.. `add_haves` dedups via `oidset_contains()`:
while ((oid = negotiator->next(negotiator))) {
if (negotiation_include_oids &&
oidset_contains(negotiation_include_oids, oid))
continue;
packet_buf_write(req_buf, "have %s\n", ...);
}
This works, and is what the new 'avoids duplicates with negotiator' test
runs against, on protocol v2. If we run on protocol v0/v1, and if my
assessment is correct, then we'd see a duplicate I think?
Sorry if I've not understood correctly or am missing something, which is
entirely possible :-)
Thanks,
MatthewThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/12/26 10:38 AM, Matthew John Cheetham wrote:
> On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> Add a new --negotiation-include option to 'git fetch', which ensures
>> that certain ref tips are always sent as 'have' lines during fetch
>> negotiation, regardless of what the negotiation algorithm selects.
>>
>> This is useful when the repository has a large number of references, so
>> the normal negotiation algorithm truncates the list. This is especially
>> important in repositories with long parallel commit histories. For
>> example, a repo could have a 'dev' branch for development and a
>> 'release' branch for released versions. If the 'dev' branch isn't
>> selected for negotiation, then it's not a big deal because there are
>> many in-progress development branches with a shared history. However, if
>> 'release' is not selected for negotiation, then the server may think
>> that this is the first time the client has asked for that reference,
>> causing a full download of its parallel commit history (and any extra
>> data that may be unique to that branch). This is based on a real example
>> where certain fetches would grow to 60+ GB when a release branch
>> updated.
>>
>> This option is a complement to --negotiation-restrict, which reduces the
>> negotiation ref set to a specific list. In the earlier example, using
>> --negotiation-restrict to focus the negotiation to 'dev' and 'release'
>> would avoid those problematic downloads, but would still not allow
>> advertising potentially-relevant user brances. In this way, the
>> 'include' version solves the problem I mention while allowing
>> negotiation to pick other references opportunistically. The two options
>> can also be combined to allow the best of both worlds.
> > Nice explanation and motivation for the need of such as feature.
> > One small typo: s/brances/branches/
Thanks.
>> --- a/Documentation/fetch-options.adoc
>> +++ b/Documentation/fetch-options.adoc
>> @@ -73,6 +73,25 @@ See also the `fetch.negotiationAlgorithm` and `push.negotiate`
>> configuration variables documented in linkgit:git-config[1], and the
>> `--negotiate-only` option below.
>> +`--negotiation-include=<revision>`::
>> + Ensure that the given ref tip is always sent as a "have" line
>> + during fetch negotiation, regardless of what the negotiation
>> + algorithm selects. This is useful to guarantee that common
>> + history reachable from specific refs is always considered, even
>> + when `--negotiation-restrict` restricts the set of tips or when
>> + the negotiation algorithm would otherwise skip them.
>> ++
>> +This option may be specified more than once; if so, each ref is sent
>> +unconditionally.
>> ++
>> +The argument may be an exact ref name (e.g. `refs/heads/release`) or a
>> +glob pattern (e.g. `refs/heads/release/{asterisk}`). The pattern syntax
>> +is the same as for `--negotiation-restrict`.
>> ++
>> +If `--negotiation-restrict` is used, the have set is first restricted by
>> +that option and then increased to include the tips specified by
>> +`--negotiation-include`.
>> +
> > The placeholder `<revision>` and the description in the body of "ref
> name or glob" slightly disagree with each other. The `--negotiation-restrict` > docs use `(<commit>|<glob>)` in the syntax definition and
> "a glob on ref names, a ref, or .. SHA-1 of a commit".
Good eye.
> `resolve_negotiation_include()` calls `repo_get_oid()` for non-globs
> so bare OIDs and abbreviated SHAs work too. Perhaps consider aligning the > syntaxes, and mention that OIDs work too.
Will do.
>> @@ -1547,10 +1548,14 @@ static void add_negotiation_restrict_tips(struct >> git_transport_options *smart_op
>> int old_nr;
>> if (!has_glob_specials(s)) {
>> struct object_id oid;
>> +
>> + /* Ignore missing reference. */
>> if (repo_get_oid(the_repository, s, &oid))
>> - die(_("%s is not a valid object"), s);
>> + continue;
>> + /* Fail on missing object pointed by ref. */
>> if (!odb_has_object(the_repository->objects, &oid, 0))
>> die(_("the object %s does not exist"), s);
>> +
>> oid_array_append(oids, &oid);
>> continue;
>> }
> > This is the change in behaviour - unresolvable revs were a fatal error
> and are now silently ignored.
> > Note that t5510 '--negotiation-tip rejects missing OIDs' still passes
> because it uses an all-zero OID, which parses as a valid hex string,
> and dies on the second check "object does not exist". Using something
> like `--negotiation-tip=notreal` that previously would error will now
> silently be ignored.
> > Is it worth another test? (invalid object vs not exists)?
Yes, let's add a test to guarantee this behavior works.
>> @@ -1615,6 +1620,13 @@ static struct transport *prepare_transport(struct >> remote *remote, int deepen,
>> strbuf_release(&config_name);
>> }
>> }
>> + if (negotiation_include.nr) {
>> + if (transport->smart_options)
>> + transport->smart_options->negotiation_include = >> &negotiation_include;
>> + else
>> + warning(_("ignoring %s because the protocol does not support it"),
>> + "--negotiation-include");
>> + }
>> return transport;
>> }
> > There is a difference between the existing `--negotiation-restrict`
> option and the new `--negotiation-include` option. Patch 3's commit
> message says:
> > "The 'tips' part is kept because this is an oid_array in the transport
> layer. This requires the builtin to handle parsing refs into
> collections of oids so the transport layer can handle this cleaner
> form of the data."
> > The new option passes the raw `string_list` to the transport layer and
> lets it resolve it instead. If the transport layer now learns how to
> resolve refs to oids, why not for tips/restrict?
> > Would it be easier for future readers for these complementary options
> to resolve their inputs at the same layer? Or at least call out why:
> "would prefer raw tips but for back-compat we resolve in the built-in"
> for example.
This is a really key observation. It's a bit of work to unravel, but I
think it's better for unifying these things. Look forward to a better
organization in the next version.
>> +static void resolve_negotiation_include(const struct string_list >> *negotiation_include,
>> + struct oidset *result)
>> +{
>> + struct string_list_item *item;
>> +
>> + if (!negotiation_include || !negotiation_include->nr)
>> + return;
>> +
>> + for_each_string_list_item(item, negotiation_include) {
>> + if (!has_glob_specials(item->string)) {
>> + struct object_id oid;
>> +
>> + /* Ignore missing reference. */
>> + if (repo_get_oid(the_repository, item->string, &oid))
>> + continue;
>> +
>> + /* Fail on missing object pointed by ref. */
>> + if (!odb_has_object(the_repository->objects, &oid, 0))
>> + die(_("the object %s does not exist"),
>> + item->string);
>> +
>> + oidset_insert(result, &oid);
>> + } else {
>> + struct refs_for_each_ref_options opts = {
>> + .pattern = item->string,
>> + };
>> + refs_for_each_ref_ext(
>> + get_main_ref_store(the_repository),
>> + add_oid_to_oidset, result, &opts);
>> + }
>> + }
>> +}
>> +
> > `resolve_negotiation_include()` is basically doing the same as
> `add_negotiation_restrict_tips()` except outputting to an `oidset`
> vs `oid_array`. This is a result of the difference in ref resolution
> layer between `--negotiation-restrict/tip` and `-include`.
Yes, this code will be replaced with a unified approach in the
next version.
>> static int find_common(struct fetch_negotiator *negotiator,
>> struct fetch_pack_args *args,
>> int fd[2], struct object_id *result_oid,
>> @@ -347,6 +390,7 @@ static int find_common(struct fetch_negotiator *negotiator,
>> struct strbuf req_buf = STRBUF_INIT;
>> size_t state_len = 0;
>> struct packet_reader reader;
>> + struct oidset negotiation_include_oids = OIDSET_INIT;
>> if (args->stateless_rpc && multi_ack == 1)
>> die(_("the option '%s' requires '%s'"), "--stateless-rpc", >> "multi_ack_detailed");
>> @@ -474,6 +518,33 @@ static int find_common(struct fetch_negotiator *negotiator,
>> trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
>> flushes = 0;
>> retval = -1;
>> +
>> + /* Send unconditional haves from --negotiation-include */
>> + resolve_negotiation_include(args->negotiation_include,
>> + &negotiation_include_oids);
>> + if (oidset_size(&negotiation_include_oids)) {
>> + struct oidset_iter iter;
>> + oidset_iter_init(&negotiation_include_oids, &iter);
>> +
>> + while ((oid = oidset_iter_next(&iter))) {
>> + struct commit *commit;
>> + packet_buf_write(&req_buf, "have %s\n",
>> + oid_to_hex(oid));
>> + print_verbose(args, "have %s", oid_to_hex(oid));
>> + count++;
>> +
>> + /*
>> + * If this is a commit, then mark as COMMON to
>> + * avoid the negotiator also outputting it as
>> + * a have.
>> + */
>> + commit = lookup_commit(the_repository, oid);
>> + if (commit &&
>> + !repo_parse_commit(the_repository, commit))
>> + commit->object.flags |= COMMON;
>> + }
>> + }
>> +
> > I want to make sure I understand the COMMON pre-marking before
> commenting further on this patch. My understanding is there are actually
> two different COMMON bits in the tree, one defined in fetch-pack.c
> (bit 6) and one in negotiator/default.c (bit 2):
> > - fetch-pack.c's COMMON (bit 6) is set after a server ACK confirms an
> OID is common with us and is read to decide when we've established
> enough common ground to terminate negotiation. This is not consulted
> in find_common().
> > - negotiator/default.c's COMMON (bit 2) is a book-keeping flag used by
> `get_rev()` to decide if we skip emitting a commit as a 'have'.
> > Since we're in fetch-pack.c here, the `commit->object.flags |= COMMON`
> line is setting bit 6. The `get_rev()` call in negotiator/default.c
> never checks bit 6, only bit 2. As far as I can tell, this mark won't
> suppress the negotiator from emitting another 'have' line in the
> protocol v0/v1 paths in `find_common()`.
> > The v2 path doesn't touch the flags.. `add_haves` dedups via `oidset_contains()`:
> > while ((oid = negotiator->next(negotiator))) {
> if (negotiation_include_oids &&
> oidset_contains(negotiation_include_oids, oid))
> continue;
> packet_buf_write(req_buf, "have %s\n", ...);
> }
> > This works, and is what the new 'avoids duplicates with negotiator' test
> runs against, on protocol v2. If we run on protocol v0/v1, and if my
> assessment is correct, then we'd see a duplicate I think?
> > Sorry if I've not understood correctly or am missing something, which is
> entirely possible :-)
This is a great catch! It shows that I'm breaking some abstractions here,
and thus it's easy to make such a mistake. It's worse that I don't catch
this problem in the tests that I am adding. I'll add a test that
demonstrates the difference.
But beyond that, I think the biggest issue is that the consumer of an
abstract 'negotiator' is assuming something about its implementation. This
means that I should update the negotiator struct to have a function
pointer dedicated to "I chose to send this 'have'" and then the negotiator
can control how to prevent sending more 'have's reachable from those tips.
Thanks,
-Stolee |
||
| `--negotiate-only` option below. | ||
|
|
||
| `--negotiation-include=(<commit>|<glob>)`:: | ||
| Ensure that the commits at the given tips are always sent as "have" | ||
| lines during fetch negotiation, regardless of what the negotiation | ||
| algorithm selects. This is useful to guarantee that common | ||
| history reachable from specific refs is always considered, even | ||
| when `--negotiation-restrict` restricts the set of tips or when | ||
| the negotiation algorithm would otherwise skip them. | ||
| + | ||
| This option may be specified more than once; if so, each commit is sent | ||
| unconditionally. | ||
| + | ||
| The argument may be an exact ref name (e.g. `refs/heads/release`), an | ||
| object hash, or a glob pattern (e.g. `refs/heads/release/{asterisk}`). | ||
| The pattern syntax is the same as for `--negotiation-restrict`. | ||
| + | ||
| If `--negotiation-restrict` is used, the have set is first restricted by | ||
| that option and then increased to include the tips specified by | ||
| `--negotiation-include`. | ||
| + | ||
| If this option is not specified on the command line, then any | ||
| `remote.<name>.negotiationInclude` config values for the current remote | ||
| are used instead. | ||
|
|
||
| `--negotiate-only`:: | ||
| Do not fetch anything from the server, and instead print the | ||
| ancestors of the provided `--negotiation-restrict=` arguments, | ||
| which we have in common with the server. | ||
| + | ||
| This is incompatible with `--recurse-submodules=(yes|on-demand)`. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,7 +98,8 @@ static struct transport *gtransport; | |
| static struct transport *gsecondary; | ||
|
There was a problem hiding this comment. Matthew John Cheetham wrote on the Git mailing list (how to reply to this email): On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> > The previous change added the --negotiation-restrict synonym for the
> --negotiation-tips option for 'git fetch'. In anticipation of adding a
> new option that behaves similarly but with distinct changes to its
> behavior, rename the internal representation of this data from
> 'negotiation_tips' to 'negotiation_restrict_tips'.
Nitpick: s/tips/tip/ .. no trailing s for either the option name, nor
the (old) variable name. The function names do use the plural however.
> The 'tips' part is kept because this is an oid_array in the transport
> layer. This requires the builtin to handle parsing refs into collections
> of oids so the transport layer can handle this cleaner form of the data.
> > Also update the string_list used to store the inputs from command-line
> options.
> > Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> builtin/fetch.c | 18 +++++++++---------
> fetch-pack.c | 18 +++++++++---------
> fetch-pack.h | 4 ++--
> transport-helper.c | 2 +-
> transport.c | 10 +++++-----
> transport.h | 4 ++--
> 6 files changed, 28 insertions(+), 28 deletions(-)
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> index fc950fe35b..2ba0051d52 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -98,7 +98,7 @@ static struct transport *gtransport;
> static struct transport *gsecondary;
> static struct refspec refmap = REFSPEC_INIT_FETCH;
> static struct string_list server_options = STRING_LIST_INIT_DUP;
> -static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
> +static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP;
Good - now mirrors the new, preferred, option name.
> struct fetch_config {
> enum display_format display_format;
> @@ -1534,13 +1534,13 @@ static int add_oid(const struct reference *ref, void *cb_data)
> return 0;
> }
> > -static void add_negotiation_tips(struct git_transport_options *smart_options)
> +static void add_negotiation_restrict_tips(struct git_transport_options *smart_options)
> {
> struct oid_array *oids = xcalloc(1, sizeof(*oids));
> int i;
> > - for (i = 0; i < negotiation_tip.nr; i++) {
> - const char *s = negotiation_tip.items[i].string;
> + for (i = 0; i < negotiation_restrict.nr; i++) {
> + const char *s = negotiation_restrict.items[i].string;
> struct refs_for_each_ref_options opts = {
> .pattern = s,
> };
All callers and references are renamed to match consistency. Good.
> @@ -1561,7 +1561,7 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
> warning(_("ignoring %s=%s because it does not match any refs"),
> "--negotiation-restrict", s);
> }
> - smart_options->negotiation_tips = oids;
> + smart_options->negotiation_restrict_tips = oids;
> }
> > static struct transport *prepare_transport(struct remote *remote, int deepen,
> @@ -1595,9 +1595,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
> set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, spec);
> set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
> }
> - if (negotiation_tip.nr) {
> + if (negotiation_restrict.nr) {
> if (transport->smart_options)
> - add_negotiation_tips(transport->smart_options);
> + add_negotiation_restrict_tips(transport->smart_options);
> else
> warning(_("ignoring %s because the protocol does not support it"),
> "--negotiation-restrict");
> @@ -2566,7 +2566,7 @@ 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-restrict", &negotiation_tip, N_("revision"),
> + OPT_STRING_LIST(0, "negotiation-restrict", &negotiation_restrict, 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,
> @@ -2658,7 +2658,7 @@ int cmd_fetch(int argc,
> config.display_format = DISPLAY_FORMAT_PORCELAIN;
> }
> > - if (negotiate_only && !negotiation_tip.nr)
> + if (negotiate_only && !negotiation_restrict.nr)
> die(_("%s needs one or more %s"), "--negotiate-only",
> "--negotiation-restrict=*");
> > diff --git a/fetch-pack.c b/fetch-pack.c
> index 6ecd468ef7..baf239adf9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -291,21 +291,21 @@ static int next_flush(int stateless_rpc, int count)
> }
> > static void mark_tips(struct fetch_negotiator *negotiator,
> - const struct oid_array *negotiation_tips)
> + const struct oid_array *negotiation_restrict_tips)
> {
> struct refs_for_each_ref_options opts = {
> .flags = REFS_FOR_EACH_INCLUDE_BROKEN,
> };
> int i;
> > - if (!negotiation_tips) {
> + if (!negotiation_restrict_tips) {
> refs_for_each_ref_ext(get_main_ref_store(the_repository),
> rev_list_insert_ref_oid, negotiator, &opts);
> return;
> }
> > - for (i = 0; i < negotiation_tips->nr; i++)
> - rev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);
> + for (i = 0; i < negotiation_restrict_tips->nr; i++)
> + rev_list_insert_ref(negotiator, &negotiation_restrict_tips->oid[i]);
> return;
> }
> > @@ -355,7 +355,7 @@ static int find_common(struct fetch_negotiator *negotiator,
> PACKET_READ_CHOMP_NEWLINE |
> PACKET_READ_DIE_ON_ERR_PACKET);
> > - mark_tips(negotiator, args->negotiation_tips);
> + mark_tips(negotiator, args->negotiation_restrict_tips);
> for_each_cached_alternate(negotiator, insert_one_alternate_object);
> > fetching = 0;
> @@ -1728,7 +1728,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> else
> state = FETCH_SEND_REQUEST;
> > - mark_tips(negotiator, args->negotiation_tips);
> + mark_tips(negotiator, args->negotiation_restrict_tips);
> for_each_cached_alternate(negotiator,
> insert_one_alternate_object);
> break;
> @@ -2177,7 +2177,7 @@ static void clear_common_flag(struct oidset *s)
> }
> }
> > -void negotiate_using_fetch(const struct oid_array *negotiation_tips,
> +void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
> const struct string_list *server_options,
> int stateless_rpc,
> int fd[],
> @@ -2195,13 +2195,13 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
> timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
> > fetch_negotiator_init(the_repository, &negotiator);
> - mark_tips(&negotiator, negotiation_tips);
> + mark_tips(&negotiator, negotiation_restrict_tips);
> > packet_reader_init(&reader, fd[0], NULL, 0,
> PACKET_READ_CHOMP_NEWLINE |
> PACKET_READ_DIE_ON_ERR_PACKET);
> > - oid_array_for_each((struct oid_array *) negotiation_tips,
> + oid_array_for_each((struct oid_array *) negotiation_restrict_tips,
> add_to_object_array,
> &nt_object_array);
> > diff --git a/fetch-pack.h b/fetch-pack.h
> index 9d3470366f..6c70c942c2 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -21,7 +21,7 @@ struct fetch_pack_args {
> * If not NULL, during packfile negotiation, fetch-pack will send "have"
> * lines only with these tips and their ancestors.
> */
> - const struct oid_array *negotiation_tips;
> + const struct oid_array *negotiation_restrict_tips;
> > unsigned deepen_relative:1;
> unsigned quiet:1;
> @@ -89,7 +89,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
> * In the capability advertisement that has happened prior to invoking this
> * function, the "wait-for-done" capability must be present.
> */
> -void negotiate_using_fetch(const struct oid_array *negotiation_tips,
> +void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
> const struct string_list *server_options,
> int stateless_rpc,
> int fd[],
LGTM up to here.
> diff --git a/transport-helper.c b/transport-helper.c
> index 4d95d84f9e..0e5b3b7202 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -754,7 +754,7 @@ static int fetch_refs(struct transport *transport,
> set_helper_option(transport, "filter", spec);
> }
> > - if (data->transport_options.negotiation_tips)
> + if (data->transport_options.negotiation_restrict_tips)
> warning("Ignoring --negotiation-tip because the protocol does not support it.");
> > if (data->fetch)
Oh! Looks like a place was missed when renaming the preferred option name in strings. It probably makes sense to do this rename in this patch
(rather than in patch 1) since we're already updating the struct field
name here anyway, but up to you.
Also do we also want to make it translatable like the others?
> diff --git a/transport.c b/transport.c
> index 107f4fa5dc..a3051f6733 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -463,7 +463,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> args.refetch = data->options.refetch;
> args.stateless_rpc = transport->stateless_rpc;
> args.server_options = transport->server_options;
> - args.negotiation_tips = data->options.negotiation_tips;
> + args.negotiation_restrict_tips = data->options.negotiation_restrict_tips;
> args.reject_shallow_remote = transport->smart_options->reject_shallow;
> > if (!data->finished_handshake) {
> @@ -491,7 +491,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> warning(_("server does not support wait-for-done"));
> ret = -1;
> } else {
> - negotiate_using_fetch(data->options.negotiation_tips,
> + negotiate_using_fetch(data->options.negotiation_restrict_tips,
> transport->server_options,
> transport->stateless_rpc,
> data->fd,
> @@ -979,9 +979,9 @@ static int disconnect_git(struct transport *transport)
> finish_connect(data->conn);
> }
> > - if (data->options.negotiation_tips) {
> - oid_array_clear(data->options.negotiation_tips);
> - free(data->options.negotiation_tips);
> + if (data->options.negotiation_restrict_tips) {
> + oid_array_clear(data->options.negotiation_restrict_tips);
> + free(data->options.negotiation_restrict_tips);
> }
> list_objects_filter_release(&data->options.filter_options);
> oid_array_clear(&data->extra_have);
> diff --git a/transport.h b/transport.h
> index 892f19454a..cdeb33c16f 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -40,13 +40,13 @@ struct git_transport_options {
> > /*
> * This is only used during fetch. See the documentation of
> - * negotiation_tips in struct fetch_pack_args.
> + * negotiation_restrict_tips in struct fetch_pack_args.
> *
> * This field is only supported by transports that support connect or
> * stateless_connect. Set this field directly instead of using
> * transport_set_option().
> */
> - struct oid_array *negotiation_tips;
> + struct oid_array *negotiation_restrict_tips;
> > /*
> * If allocated, whenever transport_fetch_refs() is called, add known
Just a missing string rename, and a nitpick typo in the commit message, but otherwise this patch looks functionally correct.
Aside: I just noticed another '--negotiation-tip' instance in the
`get_commons_through_negotiation` function in send-pack.c. It still uses
the 'tip' option name when forming the shell cmdline.
Thanks,
Matthew
There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/12/26 7:30 AM, Matthew John Cheetham wrote:
> On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <stolee@gmail.com>
>>
>> The previous change added the --negotiation-restrict synonym for the
>> --negotiation-tips option for 'git fetch'. In anticipation of adding a
>> new option that behaves similarly but with distinct changes to its
>> behavior, rename the internal representation of this data from
>> 'negotiation_tips' to 'negotiation_restrict_tips'.
> > Nitpick: s/tips/tip/ .. no trailing s for either the option name, nor
> the (old) variable name. The function names do use the plural however.
Thanks for the close eye!
>> diff --git a/transport-helper.c b/transport-helper.c
>> index 4d95d84f9e..0e5b3b7202 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -754,7 +754,7 @@ static int fetch_refs(struct transport *transport,
>> set_helper_option(transport, "filter", spec);
>> }
>> - if (data->transport_options.negotiation_tips)
>> + if (data->transport_options.negotiation_restrict_tips)
>> warning("Ignoring --negotiation-tip because the protocol does not >> support it.");
>> if (data->fetch)
> > Oh! Looks like a place was missed when renaming the preferred option name in > strings. It probably makes sense to do this rename in this patch
> (rather than in patch 1) since we're already updating the struct field
> name here anyway, but up to you.
> > Also do we also want to make it translatable like the others?
I will update this as part of the previous patch that handles all the strings,
including making it translatable and dropping the capital "I" at the start.
Good find.
> Aside: I just noticed another '--negotiation-tip' instance in the
> `get_commons_through_negotiation` function in send-pack.c. It still uses
> the 'tip' option name when forming the shell cmdline.
Thanks! I've done a more careful search and confirmed that you found
everything I had missed.
-Stolee |
||
| static struct refspec refmap = REFSPEC_INIT_FETCH; | ||
| static struct string_list server_options = STRING_LIST_INIT_DUP; | ||
| static struct string_list negotiation_restrict = STRING_LIST_INIT_NODUP; | ||
| static struct string_list negotiation_include = STRING_LIST_INIT_NODUP; | ||
|
|
||
| struct fetch_config { | ||
| enum display_format display_format; | ||
|
|
@@ -1534,34 +1535,39 @@ static int add_oid(const struct reference *ref, void *cb_data) | |
| return 0; | ||
|
There was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Apr 15, 2026 at 03:14:22PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 3bcb0c9686..4c3c5f2faa 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
Don't we want to also rename the local `negotiation_tip` variable in
`cmd_fetch()`?
Patrick |
||
| } | ||
|
|
||
| static void add_negotiation_tips(struct string_list *input_list, | ||
| struct oid_array **output_list) | ||
| { | ||
| struct oid_array *oids = xcalloc(1, sizeof(*oids)); | ||
| int i; | ||
|
|
||
| for (i = 0; i < input_list->nr; i++) { | ||
| const char *s = input_list->items[i].string; | ||
| struct refs_for_each_ref_options opts = { | ||
| .pattern = s, | ||
| }; | ||
| int old_nr; | ||
| if (!has_glob_specials(s)) { | ||
| struct object_id oid; | ||
|
|
||
| /* Ignore missing reference. */ | ||
| if (repo_get_oid(the_repository, s, &oid)) | ||
| continue; | ||
| /* Fail on missing object pointed by ref. */ | ||
| if (!odb_has_object(the_repository->objects, &oid, 0)) | ||
| die(_("the object %s does not exist"), s); | ||
|
|
||
| oid_array_append(oids, &oid); | ||
| continue; | ||
| } | ||
| old_nr = oids->nr; | ||
| refs_for_each_ref_ext(get_main_ref_store(the_repository), | ||
| add_oid, oids, &opts); | ||
| if (old_nr == oids->nr) | ||
| warning(_("ignoring %s=%s because it does not match any refs"), | ||
| "--negotiation-restrict", s); | ||
| } | ||
| *output_list = oids; | ||
| } | ||
|
|
||
| static struct transport *prepare_transport(struct remote *remote, int deepen, | ||
|
|
@@ -1595,11 +1601,46 @@ static struct transport *prepare_transport(struct remote *remote, int deepen, | |
| set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, spec); | ||
| set_option(transport, TRANS_OPT_FROM_PROMISOR, "1"); | ||
| } | ||
| if (negotiation_restrict.nr) { | ||
| if (transport->smart_options) | ||
| add_negotiation_tips(&negotiation_restrict, | ||
| &transport->smart_options->negotiation_restrict_tips); | ||
| 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_tips(&negotiation_restrict, | ||
| &transport->smart_options->negotiation_restrict_tips); | ||
| 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); | ||
| } | ||
| } | ||
| if (negotiation_include.nr) { | ||
| if (transport->smart_options) | ||
| add_negotiation_tips(&negotiation_include, | ||
| &transport->smart_options->negotiation_include_tips); | ||
| else | ||
| warning(_("ignoring %s because the protocol does not support it"), | ||
| "--negotiation-include"); | ||
| } else if (remote->negotiation_include.nr) { | ||
| if (transport->smart_options) { | ||
| add_negotiation_tips(&remote->negotiation_include, | ||
| &transport->smart_options->negotiation_include_tips); | ||
| } else { | ||
| struct strbuf config_name = STRBUF_INIT; | ||
| strbuf_addf(&config_name, "remote.%s.negotiationInclude", remote->name); | ||
| warning(_("ignoring %s because the protocol does not support it"), | ||
| config_name.buf); | ||
| strbuf_release(&config_name); | ||
| } | ||
| } | ||
| return transport; | ||
| } | ||
|
|
@@ -2565,8 +2606,11 @@ 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-restrict", &negotiation_restrict, N_("revision"), | ||
| N_("report that we have only objects reachable from this object")), | ||
| OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"), | ||
| OPT_STRING_LIST(0, "negotiation-include", &negotiation_include, N_("revision"), | ||
| N_("ensure this ref is always sent as a negotiation have")), | ||
| 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), | ||
|
|
@@ -2656,9 +2700,6 @@ int cmd_fetch(int argc, | |
| config.display_format = DISPLAY_FORMAT_PORCELAIN; | ||
| } | ||
|
|
||
| if (deepen_relative) { | ||
| if (deepen_relative < 0) | ||
| die(_("negative depth in --deepen is not supported")); | ||
|
|
@@ -2746,14 +2787,19 @@ 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) { | ||
| warning(_("protocol does not support --negotiate-only, exiting")); | ||
| result = 1; | ||
| trace2_region_leave("fetch", "negotiate-only", the_repository); | ||
| goto cleanup; | ||
| } | ||
| if (!gtransport->smart_options->negotiation_restrict_tips) | ||
| die(_("%s needs one or more %s"), "--negotiate-only", | ||
| "--negotiation-restrict=*"); | ||
|
|
||
| gtransport->smart_options->acked_commits = &acked_commits; | ||
|
|
||
| if (server_options.nr) | ||
| gtransport->server_options = &server_options; | ||
| result = transport_fetch_refs(gtransport, NULL); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):