Re: [PATCH v2 5/7] fetch: add --negotiation-require option for negotiation

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2026, 19:50 UTC
Message-ID: <xmqqik9s57gc.fsf@gitster.g>
In-Reply-To: <49c80cef2e25ecadf894cf42661d39dc82493f47.1776266066.git.gitgitgadget@gmail.com>

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

Show 18 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

> +	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).

Show 18 quoted lines

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