Re: [PATCH v4 8/8] send-pack: pass negotiation config in push

> 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>
> ---
>   Documentation/config/remote.adoc |  6 ++++++
>   send-pack.c                      | 37 ++++++++++++++++++++++++++------
>   send-pack.h                      |  2 ++
>   t/t5516-fetch-push.sh            | 30 ++++++++++++++++++++++++++
>   transport.c                      |  2 ++
>   5 files changed, 70 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
> index 9ae20e4379..460b4e7952 100644
> --- a/Documentation/config/remote.adoc
> +++ b/Documentation/config/remote.adoc
> @@ -122,6 +122,9 @@ command-line option.  If `--negotiation-restrict` (or its synonym
>   `--negotiation-tip`) is specified on the command line, then the config
>   values are not used.
>   +
> +These values also influence negotiation during `git push` if
> +`push.negotiate` is enabled.
> ++
>   Blank values signal to ignore all previous values, allowing a reset of
>   the list from broader config scenarios.
>   

> @@ -149,6 +152,9 @@ unconditionally on top of those heuristically selected commits.  This
>   option is also used during push negotiation when `push.negotiate` is
>   enabled.
>   +
> +These values also influence negotiation during `git push` if
> +`push.negotiate` is enabled.
> ++
>   Blank values signal to ignore all previous values, allowing a reset of
>   the list from broader config scenarios.
>   
> diff --git a/send-pack.c b/send-pack.c
> index 3d5d36ba3b..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)) {
> +
> +	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",
> -				     oid_to_hex(&ref->new_oid));
> -			nr_negotiation_tip++;
> +				     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;
>   	}
> @@ -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 fa54928966..a2d8958cb8 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;