Re: [PATCH v3 5/7] fetch: add --negotiation-include option for negotiation
- From
Matthew John Cheetham <mjcheetham@outlook.com>
- Date
- May 12, 2026, 14:38 UTC
- Message-ID
- <VI0PR03MB1163403743E62E0FDC4AFE52DC0392@VI0PR03MB11634.eurprd03.prod.outlook.com>
- In-Reply-To
- <ae81ef36a1b3ca04e39e891cce827fa55540b9bb.1776871546.git.gitgitgadget@gmail.com>
On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:
Show 28 quoted lines
> 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.
Show 48 quoted lines
> 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.
Show 31 quoted lines
> `--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)?
Show 13 quoted lines
> @@ -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.
Show 81 quoted lines
> @@ -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`.
Show 42 quoted lines
> 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, Matthew