fetch: rework negotiation tip options#2085
Open
derrickstolee wants to merge 8 commits into
Open
Conversation
derrickstolee
changed the title
Author
|
/submit |
|
Submitted as pull.2085.git.1775658970.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Based on my understanding, the '--negotiation-tip' option is close but not
> quite what I want. I could have the client only advertise 'release' and
> 'main' and never advertise any user branches. But then we'd download all
> content from each user branch every time it updates. Perhaps this would
> happen even with opportunistic inclusion of more haves, but I'd like to
> explore this area more.
>
> There's also an issue that the '--negotiation-tip' feature doesn't seem to
> have a config key that enables it without CLI arguments. This is something
> that we could consider independently.
> ...
> Big picture questions to think about:
>
> * Is this a valuable addition to the fetch negotiation?
> * Is the interaction between --must-have and --negotiation-tip correct?
> * Is the "must have" name sensical to users? I expect that this only
> matters to experts, but I'm open to better names that could be more
> self-documenting.
> * Should we add a similar config key for --negotiation-tip?
Just like you, I hate the name "must have", but stepping back a bit,
would it work if we add a single boolean option that says "use the
negotiation tips as the primary source of 'have's you'd send, but
unlike the way how the original negotiation-tip feature worked
without this bit enabled, which did not send anything other than the
ones reachable by negotiation tips, do advertise opportunistically
other tips", essentially turning the existing negotiation-tips
feature into your must-have feature? You could even call the option
"--negotiate-better(=(yes|no))" or something, perhaps?
|
|
Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 4/8/2026 2:59 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Based on my understanding, the '--negotiation-tip' option is close but not
>> quite what I want. I could have the client only advertise 'release' and
>> 'main' and never advertise any user branches. But then we'd download all
>> content from each user branch every time it updates. Perhaps this would
>> happen even with opportunistic inclusion of more haves, but I'd like to
>> explore this area more.
>>
>> There's also an issue that the '--negotiation-tip' feature doesn't seem to
>> have a config key that enables it without CLI arguments. This is something
>> that we could consider independently.
>> ...
>> Big picture questions to think about:
>>
>> * Is this a valuable addition to the fetch negotiation?
>> * Is the interaction between --must-have and --negotiation-tip correct?
>> * Is the "must have" name sensical to users? I expect that this only
>> matters to experts, but I'm open to better names that could be more
>> self-documenting.
>> * Should we add a similar config key for --negotiation-tip?
>
> Just like you, I hate the name "must have", but stepping back a bit,
> would it work if we add a single boolean option that says "use the
> negotiation tips as the primary source of 'have's you'd send, but
> unlike the way how the original negotiation-tip feature worked
> without this bit enabled, which did not send anything other than the
> ones reachable by negotiation tips, do advertise opportunistically
> other tips", essentially turning the existing negotiation-tips
> feature into your must-have feature? You could even call the option
> "--negotiate-better(=(yes|no))" or something, perhaps?
I like this line of thought. You essentially want to use the existing
scaffolding of the --negotiate-tip option but change it from being a
_maximum set_ to being a _minimum set_.
## Considering --negotiation-tip-mode=<mode>
With that in mind, we could have an option like --negotiation-tip-mode
that takes one of a few options. Here are some word choices that I
immediately thought about:
* maximum|minimum: Are these sets a maximum set to choose from or a
minimum set to include?
* restrict|include: Are we restricting the haves to this set, or are
we including these tips by default?
* v1|v2: Use numerical versions to indicate the mode without commentary
so it could be extended in the future to v3 or more.
None of these jump out as a clear winner in my head. I'm interested in
more exploration of this space before rerolling.
## To mix modes, or not to mix modes?
One downside of this approach is that it disables the ability to use
both modes, at least in its most obvious implementation. What if someone
wants to force a minimum set of wants but also wants to focus the set
of additional wants to a specific ref space?
Theoretically, we could implement the option to toggle with multiple
options, using
--negotiation-tip-mode=minimum --negotiation-tip=refs/remotes/origin/main \
--negotiation-tip-mode=maximum --negotiation-tip=refs/remotes/origin/*
and as we process the --negotiation-tip options we'd put the input data
into different lists. Would this complexity be worth it compared to making
a new set of options?
This also becomes more complicated how to describe the interaction of
these options and any config options that enable them by default. When
exactly does the config get ignored in favor of CLI options?
## Considering --negotiation-(required|restricted)
We could alternatively create two new types of options that are clearly
related:
* --negotiation-restricted works exactly like --negotiation-tips and
would be a synonym (with the old one being "deprecated" in favor of
the newer one).
* --negotiation-required works like the --must-have in this series.
---
Thanks for considering these options with me. There is a lot of room
for creativity here. This series isn't even my first attempt at this
functionality because there are so many possible ways to accomplish
this goal.
Thanks,
-Stolee
|
derrickstolee
force-pushed
the
must-have
branch
from
6c227f1 to
c733b14
Compare
derrickstolee
changed the title
derrickstolee
force-pushed
the
must-have
branch
from
c733b14 to
7cccf59
Compare
Author
|
/submit |
|
Submitted as pull.2085.v2.git.1776266066.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -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. | |||
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:
> 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-<.
| @@ -69,6 +73,25 @@ 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
|
This patch series was integrated into seen via git@9731f94. |
| @@ -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
|
This branch is now known as |
|
This patch series was integrated into seen via git@b3dd369. |
|
This patch series was integrated into seen via git@505d56a. |
|
This patch series was integrated into seen via git@80a8f2a. |
|
This patch series was integrated into seen via git@dacffc3. |
|
This patch series was integrated into seen via git@878809d. |
|
There was a status update in the "New Topics" section about the branch The negotiation tip options in "git fetch" have been reworked to allow requiring certain refs to be sent as "have" lines, and to restrict negotiation to a specific set of refs. Needs review. source: <pull.2085.v2.git.1776266066.gitgitgadget@gmail.com> |
derrickstolee
force-pushed
the
must-have
branch
from
7cccf59 to
49d8d3e
Compare
| @@ -1534,7 +1534,7 @@ 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
| @@ -69,6 +73,25 @@ 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.
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 status update in the "Cooking" section about the branch The negotiation tip options in "git fetch" have been reworked to allow requiring certain refs to be sent as "have" lines, and to restrict negotiation to a specific set of refs. Needs review. source: <pull.2085.v2.git.1776266066.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@860ab15. |
|
This patch series was integrated into seen via git@0ba8794. |
|
There was a status update in the "Cooking" section about the branch The negotiation tip options in "git fetch" have been reworked to allow requiring certain refs to be sent as "have" lines, and to restrict negotiation to a specific set of refs. Needs review. source: <pull.2085.v2.git.1776266066.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@189a9c5. |
|
This patch series was integrated into seen via git@967ccbc. |
|
There was a status update in the "Cooking" section about the branch The negotiation tip options in "git fetch" have been reworked to allow requiring certain refs to be sent as "have" lines, and to restrict negotiation to a specific set of refs. Needs review. source: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@9f6d2c7. |
|
This patch series was integrated into seen via git@3e84e22. |
|
This patch series was integrated into seen via git@63fa4de. |
|
This patch series was integrated into seen via git@afdb5f5. |
|
There was a status update in the "Cooking" section about the branch The negotiation tip options in "git fetch" have been reworked to allow requiring certain refs to be sent as "have" lines, and to restrict negotiation to a specific set of refs. Needs review. source: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@e6fe78f. |
|
This patch series was integrated into seen via git@44246e0. |
|
This patch series was integrated into seen via git@892923a. |
|
There was a status update in the "Cooking" section about the branch The negotiation tip options in "git fetch" have been reworked to allow requiring certain refs to be sent as "have" lines, and to restrict negotiation to a specific set of refs. Needs review. source: <pull.2085.v3.git.1776871546.gitgitgadget@gmail.com> |
| @@ -1349,7 +1349,7 @@ test_expect_success 'fetch follows tags by default' ' | |||
| git for-each-ref >tmp1 && | |||
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 'fetch follows tags by default' test sorts using 'sort -k 4', but
> for-each-ref output only has 3 columns. This relies on sort treating
> records with fewer fields as having an empty fourth field, which may
> produce unstable results depending on locale. Use 'sort -k 3' to match
> the actual number of columns in the output.
> > Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> t/t5516-fetch-push.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 29e2f17608..ac8447f21e 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1349,7 +1349,7 @@ test_expect_success 'fetch follows tags by default' '
> git for-each-ref >tmp1 &&
> sed -n "p; s|refs/heads/main$|refs/remotes/origin/main|p" tmp1 |
> sed -n "p; s|refs/heads/main$|refs/remotes/origin/HEAD|p" |
> - sort -k 4 >../expect
> + sort -k 3 >../expect
> ) &&
> test_when_finished "rm -rf dst" &&
> git init dst &&
Makes sense. Looks like 3f763ddf28 ("fetch: set remote/HEAD if it does
not exist") originally changed it from -k3 to -k4 by mistake.
Thanks,
Matthew
|
User |
| @@ -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.
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
| @@ -98,7 +98,7 @@ 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
The 'fetch follows tags by default' test sorts using 'sort -k 4', but for-each-ref output only has 3 columns. This relies on sort treating records with fewer fields as having an empty fourth field, which may produce unstable results depending on locale. This appears to be an accident added in 3f763dd (fetch: set remote/HEAD if it does not exist, 2024-11-22). Use 'sort -k 3' to match the actual number of columns in the output. Signed-off-by: Derrick Stolee <stolee@gmail.com>
| @@ -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. | |||
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
| @@ -69,6 +73,25 @@ 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.
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
| `--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 |
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
| @@ -433,28 +433,48 @@ static void reject_invalid_nonce(const char *nonce, int len) | |||
|
|
|||
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>
> > When push.negotiate is enabled, 'git push' spawns a child 'git fetch
> --negotiate-only' process to find common commits. Pass
> --negotiation-include and --negotiation-restrict options from the
> 'remote.<name>.negotiationInclude' and
> 'remote.<name>.negotiationRestrict' config keys to this child process.
> > When negotiationRestrict is configured, it replaces the default
> behavior of using all remote refs as negotiation tips. This allows
> the user to control which local refs are used for push negotiation.
> > When negotiationInclude is configured, the specified ref patterns
> are passed as --negotiation-include to ensure their tips are always
> sent as 'have' lines during push negotiation.
> > This change also updates the use of --negotiation-tip into
> --negotiation-restrict now that the new synonym exists.
> > Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> send-pack.c | 39 +++++++++++++++++++++++++++++++--------
> send-pack.h | 2 ++
> t/t5516-fetch-push.sh | 30 ++++++++++++++++++++++++++++++
> transport.c | 2 ++
> 4 files changed, 65 insertions(+), 8 deletions(-)
This patch wires up the negotiation behaviour with push, added in the
previous patches.
> diff --git a/send-pack.c b/send-pack.c
> index 67d6987b1c..d18e030ce8 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -433,28 +433,48 @@ static void reject_invalid_nonce(const char *nonce, int len)
> > static void get_commons_through_negotiation(struct repository *r,
> const char *url,
> + const struct string_list *negotiation_include,
> + const struct string_list *negotiation_restrict,
> const struct ref *remote_refs,
> struct oid_array *commons)
> {
> struct child_process child = CHILD_PROCESS_INIT;
> const struct ref *ref;
> int len = r->hash_algo->hexsz + 1; /* hash + NL */
> - int nr_negotiation_tip = 0;
> + int nr_negotiation = 0;
> > child.git_cmd = 1;
> child.no_stdin = 1;
> child.out = -1;
> strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
> - for (ref = remote_refs; ref; ref = ref->next) {
> - if (!is_null_oid(&ref->new_oid)) {
> - strvec_pushf(&child.args, "--negotiation-tip=%s",
> - oid_to_hex(&ref->new_oid));
> - nr_negotiation_tip++;
> +
> + if (negotiation_restrict && negotiation_restrict->nr) {
> + struct string_list_item *item;
> + for_each_string_list_item(item, negotiation_restrict)
> + strvec_pushf(&child.args, "--negotiation-restrict=%s",
> + item->string);
> + nr_negotiation = negotiation_restrict->nr;
> + } else {
> + for (ref = remote_refs; ref; ref = ref->next) {
> + if (!is_null_oid(&ref->new_oid)) {
> + strvec_pushf(&child.args, "--negotiation-restrict=%s",
> + oid_to_hex(&ref->new_oid));
> + nr_negotiation++;
> + }
> }
> }
> +
> + if (negotiation_include && negotiation_include->nr) {
> + struct string_list_item *item;
> + for_each_string_list_item(item, negotiation_include)
> + strvec_pushf(&child.args, "--negotiation-include=%s",
> + item->string);
> + nr_negotiation += negotiation_include->nr;
> + }
> +
> strvec_push(&child.args, url);
> > - if (!nr_negotiation_tip) {
> + if (!nr_negotiation) {
> child_process_clear(&child);
> return;
> }
Reads cleanly, and also updates the calls to fetch to use the new
preferred option name `restrict` vs the older `tip`. Nice!
> @@ -528,7 +548,10 @@ int send_pack(struct repository *r,
> repo_config_get_bool(r, "push.negotiate", &push_negotiate);
> if (push_negotiate) {
> trace2_region_enter("send_pack", "push_negotiate", r);
> - get_commons_through_negotiation(r, args->url, remote_refs, &commons);
> + get_commons_through_negotiation(r, args->url,
> + args->negotiation_include,
> + args->negotiation_restrict,
> + remote_refs, &commons);
> trace2_region_leave("send_pack", "push_negotiate", r);
> }
> > diff --git a/send-pack.h b/send-pack.h
> index c5ded2d200..13850c98bb 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -18,6 +18,8 @@ struct repository;
> > struct send_pack_args {
> const char *url;
> + const struct string_list *negotiation_include;
> + const struct string_list *negotiation_restrict;
> unsigned verbose:1,
> quiet:1,
> porcelain:1,
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index ac8447f21e..177cbc6c75 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -254,6 +254,36 @@ test_expect_success 'push with negotiation does not attempt to fetch submodules'
> ! grep "Fetching submodule" err
> '
> > +test_expect_success 'push with negotiation and remote.<name>.negotiationInclude' '
> + test_when_finished rm -rf negotiation_include &&
> + mk_empty negotiation_include &&
> + git push negotiation_include $the_first_commit:refs/remotes/origin/first_commit &&
> + test_commit -C negotiation_include unrelated_commit &&
> + git -C negotiation_include config receive.hideRefs refs/remotes/origin/first_commit &&
> + test_when_finished "rm event" &&
> + GIT_TRACE2_EVENT="$(pwd)/event" \
> + git -c protocol.version=2 -c push.negotiate=1 \
> + -c remote.negotiation_include.negotiationInclude=refs/heads/main \
> + push negotiation_include refs/heads/main:refs/remotes/origin/main &&
> + test_grep \"key\":\"total_rounds\" event &&
> + grep_wrote 2 event # 1 commit, 1 tree
> +'
> +
> +test_expect_success 'push with negotiation and remote.<name>.negotiationRestrict' '
> + test_when_finished rm -rf negotiation_restrict &&
> + mk_empty negotiation_restrict &&
> + git push negotiation_restrict $the_first_commit:refs/remotes/origin/first_commit &&
> + test_commit -C negotiation_restrict unrelated_commit &&
> + git -C negotiation_restrict config receive.hideRefs refs/remotes/origin/first_commit &&
> + test_when_finished "rm event" &&
> + GIT_TRACE2_EVENT="$(pwd)/event" \
> + git -c protocol.version=2 -c push.negotiate=1 \
> + -c remote.negotiation_restrict.negotiationRestrict=refs/heads/main \
> + push negotiation_restrict refs/heads/main:refs/remotes/origin/main &&
> + test_grep \"key\":\"total_rounds\" event &&
> + grep_wrote 2 event # 1 commit, 1 tree
> +'
> +
> test_expect_success 'push without wildcard' '
> mk_empty testrepo &&
> > diff --git a/transport.c b/transport.c
> index 8a2d8adffc..60b73feb34 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -921,6 +921,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
> args.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);
> args.push_options = transport->push_options;
> args.url = transport->url;
> + args.negotiation_include = &transport->remote->negotiation_include;
> + args.negotiation_restrict = &transport->remote->negotiation_restrict;
> > if (flags & TRANSPORT_PUSH_CERT_ALWAYS)
> args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
I've got no other comments on this patch :-)
Thanks,
MatthewThe --negotiation-tip option to 'git fetch' and 'git pull' allows users to specify that they want to focus negotiation on a small set of references. This is a _restriction_ on the negotiation set, helping to focus the negotiation when the ref count is high. However, it doesn't allow for the ability to opportunistically select references beyond that list. This subtle detail that this is a 'maximum set' and not a 'minimum set' is not immediately clear from the option name. This makes it more complicated to add a new option that provides the complementary behavior of a minimum set. For now, create a new synonym option, --negotiation-restrict, that behaves identically to --negotiation-tip. Update the documentation to make it clear that this new name is the preferred option, but we keep the old name for compatibility. Mark --negotiation-tip as an alias of the new, preferred option. Update a few warning messages with the new option, but also make them translatable with the option name inserted by formatting. At least one of these messages will be reused later for a new option. Signed-off-by: Derrick Stolee <stolee@gmail.com>
The previous change added the --negotiation-restrict synonym for the --negotiation-tip 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'. 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>
In a previous change, the --negotiation-restrict command-line option of 'git fetch' was added as a synonym of --negotiation-tip. 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. 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>
In a future change, we will introduce a capability to choose specific commit
OIDs as 'have's in fetch negotiation, with the ability to have the
negotiator choose more 'have's to increase coverage beyond that required
core set. The negotiator works to avoid emitting 'have's that can reach each
other, but that logic is hidden beneath the negotiator's iterator function
pointer ('next'). We need a way to communicate to the negotiator that we
have picked a 'have' so it could incorporate that into its logic.
Add a have_sent() method to the fetch_negotiator interface. This is the
signal that allows the negotiator to track the commit as already shown and
can perform the proper bookkeeping to avoid emitting those objects or
anything they can reach.
For our non-trivial negotiators, it is sufficient to mark these commits as
common, so the implementation is quite simple. This logic will be exercised
in the next change.
Signed-off-by: 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 branches. 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. 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. The implementation outputs the requested objects as haves before the negotiator performs its own algorithm to choose the next haves. Use the new have_sent() interface to signal these have commits were sent before engaging with the negotiator's next() iterator. Also add --negotiation-include to 'git pull' passthrough options. Signed-off-by: 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>
When push.negotiate is enabled, 'git push' spawns a child 'git fetch --negotiate-only' process to find common commits. Pass --negotiation-include and --negotiation-restrict options from the 'remote.<name>.negotiationInclude' and 'remote.<name>.negotiationRestrict' config keys to this child process. When negotiationRestrict is configured, it replaces the default behavior of using all remote refs as negotiation tips. This allows the user to control which local refs are used for push negotiation. When negotiationInclude is configured, the specified ref patterns are passed as --negotiation-include to ensure their tips are always sent as 'have' lines during push negotiation. This change also updates the use of --negotiation-tip into --negotiation-restrict now that the new synonym exists. Signed-off-by: Derrick Stolee <stolee@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fetch negotiation aims to find enough information from haves and wants such that the server can be reasonably confident that it will send all necessary objects and not too many "extra" objects that the client already has. However, this can break down if there are too many references, since Git truncates the list of haves based on a few factors (a 256 count limit or the server sending an ACK at the right time).
We already have the
--negotiation-tipfeature to focus the set of references that are used in negotiation, but I feel like this is designed backwards. I'd rather that we have a way to say "this is an important set of refs, but feel free to add more refs if needed" than "only use these refs for negotiation".Here's an example that demonstrates the problem. In an internal monorepo, developers work off of the 'main' branch so there are thousands of user branches that each add a few commits different from the 'main' branch. However, there is also a long-lived 'release' branch. This branch has a first-parent history that is parallel to 'main' and each of those commits is a merge whose second parent is a commit from 'main' that had a successful CI run. There are additional changes in the 'release' branch merge commits that add some changelog data, so there is a nontrivial set of novel blob content in that branch and not just a different set of commits.
The problem we had was that our georeplication system was regularly fetching from the origin and trying to get all data from all reachable branches. When the 'release' branch updated, the client would run out of haves before advertising its copy of the 'release' branch, but it would still list the new 'release' tip as a want. The server would then think that the client had never fetched that branch before and would send all of the changelog data from the whole history of the repo. (This led to a lot of downstream problems; we mitigated by setting a refspec that stopped fetching the 'release' branch, but this is not ideal.)
What I'd like is a mechanism to say "always advertise the client's version of 'main' and 'release' but also opportunistically include some user branches".
Based on my understanding, the '--negotiation-tip' option is close but not quite what I want. I could have the client only advertise 'release' and 'main' and never advertise any user branches. But then we'd download all content from each user branch every time it updates. Perhaps this would happen even with opportunistic inclusion of more haves, but I'd like to explore this area more.
There's also an issue that the '--negotiation-tip' feature doesn't seem to have a config key that enables it without CLI arguments. This is something that we could consider independently.
This patch series adds a new '--negotiation-include' option that does what I want: it makes sure that these references are included as 'have's during negotiation. In order to help clarify the difference between this and '--negotiation-tip', I first create a synonym called '--negotiation-restrict'.
Both of these options get 'remote.*.negotiation(Include|Restrict)' config options that enable their behavior by default.
During development, I had briefly considered only using config values, but that required some strange changes to care about the remote name in the transport layer. This was most different in the 'git push' integration. When I discovered the '--negotiation-tip' feature during the process, that gave me a clear pattern to follow with the addition of a config on top.
Updates in v2
This version is a near-complete rewrite based on feedback around the names of the previous option and config. The --negotiation-restrict option is new and the ability to set it via config is also new.
I did try to be more careful around translatable error messages, too.
Updates in v3
%sto isolate non-translatable options from translatable words.Updates in v4
Thanks, Matthew, for the detailed review! There are some big changes in this version.
Thanks,
-Stolee
cc: gitster@pobox.com
cc: ps@pks.im
cc: Matthew John Cheetham mjcheetham@outlook.com