Skip to content

fetch: rework negotiation tip options #2085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
derrickstolee wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fetch: rework negotiation tip options #2085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
466c56a
t5516: fix test order flakiness
derrickstolee Mar 27, 2026
fe87539
fetch: add --negotiation-restrict option
derrickstolee Apr 14, 2026
4332cbf
transport: rename negotiation_tips
derrickstolee Apr 14, 2026
24f8e9b
remote: add remote.*.negotiationRestrict config
derrickstolee Apr 14, 2026
0e00590
fetch: add --negotiation-require option for negotiation
derrickstolee Apr 14, 2026
ab78cb6
remote: add remote.*.negotiationRequire config
derrickstolee Apr 14, 2026
49d8d3e
send-pack: pass negotiation config in push
derrickstolee Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions Documentation/config/remote.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,46 @@ priority configuration file (e.g. `.git/config` in a repository) to clear
the values inherited from a lower priority configuration files (e.g.
Copy link
Copy Markdown

@gitgitgadget gitgitgadget bot Apr 15, 2026

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):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.
>
> 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.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  Documentation/config/remote.adoc | 16 ++++++++++++++++
>  builtin/fetch.c                  | 24 ++++++++++++++++++++++--
>  remote.c                         |  6 ++++++
>  remote.h                         |  1 +
>  t/t5510-fetch.sh                 | 22 ++++++++++++++++++++++
>  5 files changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
> index 91e46f66f5..5e8ac6cfdd 100644
> --- a/Documentation/config/remote.adoc
> +++ b/Documentation/config/remote.adoc
> @@ -107,6 +107,22 @@ 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.

This is a tangent, but I wonder what happens when this is set in
/etc/gitconfig or ~/.gitconfig by mistake.  I personally do not
think of any good reason to set it in either of these two places,
so it might be fine to declare that we read this only from local
configuration file or "git -c var=val" command line, but alternative
that is easier to implement would be to allow for a variable
definition syntax that allows you to say "forget everything you read
so far, clear this multi-valued variable", e.g.

    == in /etc/gitconfig ==
    [remote "origin"]
	negotiationRestrict = refs/pull/*

    == in .git/config ==
    [remote "origin"]
	# clear them
	negotiationRestrict =
	negotiationRestrict = refs/heads/*
	negotiationRestrict = refs/tags/*

or something like that, perhaps?

It is a shame that our configuration framework do not allow
specifying their meanings and semantics to variables like
parse-options do (where OPT_STRING_LIST naturally allows
--no-negotiation-restrict to act as a way to clear the deck).
Because there is no official way to programatically declare that
remote.<name>.negotiationRestrict is a multi-valued variable whose
values are stored in a string-list, the config callback needs to 
be coded to implement the behaviour for each variable X-<.

`$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.

remote.<name>.negotiationRequire::
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`.
+
These config values are used as defaults for the `--negotiation-require`
command-line option. If `--negotiation-require` 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>.negotiationRequire` are sent
unconditionally on top of those heuristically selected commits. This
option is also used during push negotiation when `push.negotiate` is
enabled.

remote.<name>.followRemoteHEAD::
How linkgit:git-fetch[1] should handle updates to `remotes/<name>/HEAD`
when fetching using the configured refspecs of a remote.
Expand Down
27 changes: 27 additions & 0 deletions Documentation/fetch-options.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown

@gitgitgadget gitgitgadget bot Apr 15, 2026

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):

"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?

Copy link
Copy Markdown

@gitgitgadget gitgitgadget bot Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
-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
Expand All @@ -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.
+
Expand All @@ -69,6 +73,29 @@ See also the `fetch.negotiationAlgorithm` and `push.negotiate`
configuration variables documented in linkgit:git-config[1], and the
Copy link
Copy Markdown

@gitgitgadget gitgitgadget bot Apr 15, 2026

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):

"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?

`--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`.
+
If this option is not specified on the command line, then any
`remote.<name>.negotiationRequire` 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-tip=` arguments,
Expand Down
64 changes: 50 additions & 14 deletions builtin/fetch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ 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;
static struct string_list negotiation_require = STRING_LIST_INIT_NODUP;

struct fetch_config {
enum display_format display_format;
Expand Down Expand Up @@ -1534,13 +1535,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,
};
Expand All @@ -1558,10 +1559,10 @@ 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;
smart_options->negotiation_restrict_tips = oids;
}

static struct transport *prepare_transport(struct remote *remote, int deepen,
Expand Down Expand Up @@ -1595,11 +1596,42 @@ 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 --negotiation-tip because the protocol does not support it");
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);
}
}
if (negotiation_require.nr) {
if (transport->smart_options)
transport->smart_options->negotiation_require = &negotiation_require;
else
warning(_("ignoring %s because the protocol does not support it"),
"--negotiation-require");
} else if (remote->negotiation_require.nr) {
if (transport->smart_options) {
transport->smart_options->negotiation_require = &remote->negotiation_require;
} else {
struct strbuf config_name = STRBUF_INIT;
strbuf_addf(&config_name, "remote.%s.negotiationRequire", remote->name);
warning(_("ignoring %s because the protocol does not support it"),
config_name.buf);
strbuf_release(&config_name);
}
}
return transport;
}
Expand Down Expand Up @@ -2565,8 +2597,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-tip", &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_STRING_LIST(0, "negotiation-require", &negotiation_require, 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),
Expand Down Expand Up @@ -2656,9 +2691,6 @@ int cmd_fetch(int argc,
config.display_format = DISPLAY_FORMAT_PORCELAIN;
}

if (negotiate_only && !negotiation_tip.nr)
die(_("--negotiate-only needs one or more --negotiation-tip=*"));

if (deepen_relative) {
if (deepen_relative < 0)
die(_("negative depth in --deepen is not supported"));
Expand Down Expand Up @@ -2746,6 +2778,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 {
Expand Down
6 changes: 6 additions & 0 deletions builtin/pull.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,12 @@ 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_PASSTHRU_ARGV(0, "negotiation-require", &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,
Expand Down
Loading
Loading