Skip to contentSkip to conversation

fetch: add --must-have and remote.*.mustHave

PATCH Superseded Derrick Stolee via GitGitGadget 5 participants 86 messages Apr 8 – May 20 Documentation Page 2 of 2

lore
Share & tools
lore
#72 lore raw

PATCH v5 8/8 send-pack: pass negotiation config in push

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

---
 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(-)
Documentation/config/remote.adoc +6 -0
index 1951df154e..eb9c8a3c48 100644
@@ -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.
 
@@ -147,6 +150,9 @@ negotiation algorithm still runs and advertises its own selected commits,
 but the refs matching `remote.<name>.negotiationInclude` are sent
 unconditionally on top of those heuristically selected commits.
 +
+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.
 
send-pack.c +30 -7
index 3d5d36ba3b..d18e030ce8 100644
@@ -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);
 	}
 
send-pack.h +2 -0
index c5ded2d200..13850c98bb 100644
@@ -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,
t/t5516-fetch-push.sh +30 -0
index ac8447f21e..177cbc6c75 100755
@@ -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 &&
 
transport.c +2 -0
index fa54928966..a2d8958cb8 100644
@@ -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;
Signature · 1 line
gitgitgadget
#73 lore raw

Re: [PATCH v5 0/8] fetch: rework negotiation tip options

In reply to Derrick Stolee via GitGitGadget
Quoted 9 lines · Derrick Stolee via GitGitGadget

Range-diff vs v4:

1: 7409a479d6 ! 1: 538913a327 t5516: fix test order flakiness
@@ Commit message

Use 'sort -k 3' to match the actual number of columns in the output.

+ Reviewed-by: Matthew John Cheetham <[email protected]>

[email protected], not [email protected].

Quoted 54 lines
  6:  b4cd458fe0 ! 6:  e86c9791e2 fetch: add --negotiation-include option for negotiation
      @@ Commit message
           Also add --negotiation-include to 'git pull' passthrough options.
       
      +    Reviewed-by: Matthew John Cheetham <[email protected]>
           Signed-off-by: Derrick Stolee <[email protected]>
       
        ## Documentation/fetch-options.adoc ##
      @@ builtin/fetch.c: static int add_oid(const struct reference *ref, void *cb_data)
        
       -static void add_negotiation_restrict_tips(struct git_transport_options *smart_options)
       +static void add_negotiation_tips(struct string_list *input_list,
      -+				 struct oid_array **output_list)
      ++				 struct oid_array **output_list,
      ++				 const char *argname)
        {
        	struct oid_array *oids = xcalloc(1, sizeof(*oids));
        	int i;
      @@ builtin/fetch.c: static int add_oid(const struct reference *ref, void *cb_data)
        			continue;
        		}
       @@ builtin/fetch.c: static void add_negotiation_restrict_tips(struct git_transport_options *smart_op
      + 				      add_oid, oids, &opts);
      + 		if (old_nr == oids->nr)
        			warning(_("ignoring %s=%s because it does not match any refs"),
      - 				"--negotiation-restrict", s);
      +-				"--negotiation-restrict", s);
      ++				argname, s);
        	}
       -	smart_options->negotiation_restrict_tips = oids;
       +	*output_list = oids;
      @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
        		if (transport->smart_options)
       -			add_negotiation_restrict_tips(transport->smart_options);
       +			add_negotiation_tips(&negotiation_restrict,
      -+					     &transport->smart_options->negotiation_restrict_tips);
      ++					     &transport->smart_options->negotiation_restrict_tips,
      ++					     "--negotiation-restrict");
        		else
        			warning(_("ignoring %s because the protocol does not support it"),
        				"--negotiation-restrict");
      @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
        		if (transport->smart_options)
       -			add_negotiation_restrict_tips(transport->smart_options);
       +			add_negotiation_tips(&negotiation_restrict,
      -+					     &transport->smart_options->negotiation_restrict_tips);
      ++					     &transport->smart_options->negotiation_restrict_tips,
      ++					     "--negotiation-restrict");
        		else {
        			struct strbuf config_name = STRBUF_INIT;
        			strbuf_addf(&config_name, "remote.%s.negotiationRestrict", remote->name);
      @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
       +	if (negotiation_include.nr) {
Quoted 8 lines
       +		if (transport->smart_options)
       +			add_negotiation_tips(&negotiation_include,
      -+					     &transport->smart_options->negotiation_include_tips);
      ++					     &transport->smart_options->negotiation_include_tips,
      ++					     "--negotiation-include");
       +		else
       +			warning(_("ignoring %s because the protocol does not support it"),
       +				"--negotiation-include");

Perfect!

Quoted 13 lines
      @@ Documentation/config/remote.adoc: values are not used.
       +This option is additive with the normal negotiation process: the
       +negotiation algorithm still runs and advertises its own selected commits,
       +but the refs matching `remote.<name>.negotiationInclude` are sent
      -+unconditionally on top of those heuristically selected commits.  This
      -+option is also used during push negotiation when `push.negotiate` is
      -+enabled.
      ++unconditionally on top of those heuristically selected commits.
       ++
       +Blank values signal to ignore all previous values, allowing a reset of
       +the list from broader config scenarios.
      @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
       +	} else if (remote->negotiation_include.nr) {
Quoted 8 lines
       +		if (transport->smart_options) {
       +			add_negotiation_tips(&remote->negotiation_include,
      -+					     &transport->smart_options->negotiation_include_tips);
      ++					     &transport->smart_options->negotiation_include_tips,
      ++					     "--negotiation-include");
       +		} else {
       +			struct strbuf config_name = STRBUF_INIT;
       +			strbuf_addf(&config_name, "remote.%s.negotiationInclude", remote->name);

Great

Quoted 16 lines

8: 5b968245eb ! 8: ed0be32e2c send-pack: pass negotiation config in push
@@ Commit message
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.
-
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## Documentation/config/remote.adoc ##
@@ Documentation/config/remote.adoc: command-line option. If `--negotiation-restri
Blank values signal to ignore all previous values, allowing a reset of the list from broader config scenarios.

Quoted 10 lines

-@@ Documentation/config/remote.adoc: unconditionally on top of those heuristically selected commits. This
- option is also used during push negotiation when push.negotiate is
- enabled.
+@@ Documentation/config/remote.adoc: negotiation algorithm still runs and advertises its own selected commits,
+ but the refs matching remote.<name>.negotiationInclude are sent
+ unconditionally on top of those heuristically selected commits.
+
+These values also influence negotiation during git push if
+push.negotiate is enabled.

LGTM

Thanks,
Matthew

#74 lore raw

Re: [PATCH v5 0/8] fetch: rework negotiation tip options

Quoted 11 lines · Matthew John Cheetham
Quoted 7 lines · Derrick Stolee via GitGitGadget

Range-diff vs v4:

  1:  7409a479d6 ! 1:  538913a327 t5516: fix test order flakiness
      @@ Commit message
                  Use 'sort -k 3' to match the actual number of columns in the output. +    Reviewed-by: Matthew John Cheetham <[email protected]>

[email protected], not [email protected].

OH NO! Sorry for this typo that I copied into every commit.

Quoted 4 lines

LGTM

Thanks,
Matthew

I'll fix this reviewed-by tag and resend. Sorry :(

Thanks for the review!

#75 lore raw

v6 cover fetch: rework negotiation tip options

In reply to Derrick Stolee via GitGitGadget

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-tip feature 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
=============

* --negotiation-tip is now an alias of --negotiation-restrict.
* More translatable strings use %s to isolate non-translatable options from translatable words.
* The string_list named negotiation_tip is now renamed to negotiation_restrict.
* The config options now allow an empty value to reset the list.
* The --negotiation-require option is now called --negotiation-include.
* Similarly, the config option is renamed and all code references.
* The included haves now mark their commits as COMMON so commits that they can reach are not included in the negotiation walk if they are reached from the restricted commits.
* The ref iterators are more careful about failing on bad references (ref exists but object doesn't) and ignoring missing references (perhaps config is erroneous?).
* When sending tips during push negotiation, use the --negotiation-restrict option instead of -tip.

Updates in v4
=============

Thanks, Matthew, for the detailed review! There are some big changes in this version.

* Expanded commit message to cite the commit that introduced the bug (3f763ddf28).
* Renamed --negotiation-tip to --negotiation-restrict throughout docs/code (including send-pack.c, transport-helper.c, builtin/pull.c). Added OPT_ALIAS in git-pull.
* Switched config parsing to use parse_transport_option() helper. Removed git push from docs (not implemented yet). Restructured --negotiate-only validation flow.
* NEW Patch 5: Added have_sent() interface to negotiators, so included haves can be de-duplicated properly by the negotiation algorithm.
* Replaced COMMON flag hack with negotiator->have_sent() calls. Moved ref-pattern resolution into builtin/fetch.c (add_negotiation_tips()) so fetch-pack receives pre-resolved oid_array instead of string_list. Added test for --negotiation-tip ignoring missing refs. Added duplicate-avoidance test for v0. Accepts commit hashes in addition to ref names/globs.
* Use parse_transport_option() for config. Updated docs to mention commit hashes. Removed git push from config docs. Fixed test to use correct restrict/include combinations.
* In the last patch, add doc notes that remote config values also apply during git push with push.negotiate, now that they are integrated by that change.

Updates in v5
=============

Responded to small comments.

Updates in v6
=============

Corrected reviewed-by annotations in commit messages.

Thanks, -Stolee

Derrick Stolee (8):
t5516: fix test order flakiness
fetch: add --negotiation-restrict option
transport: rename negotiation_tips
remote: add remote.*.negotiationRestrict config
negotiator: add have_sent() interface
fetch: add --negotiation-include option for negotiation
remote: add remote.*.negotiationInclude config
send-pack: pass negotiation config in push

 Documentation/config/fetch.adoc  |   2 +-
 Documentation/config/remote.adoc |  49 ++++++++
 Documentation/fetch-options.adoc |  29 ++++-
 builtin/fetch.c                  |  87 +++++++++++---
 builtin/pull.c                   |   6 +-
 fetch-negotiator.h               |   9 ++
 fetch-pack.c                     |  99 +++++++++++++---
 fetch-pack.h                     |  10 +-
 negotiator/default.c             |   8 ++
 negotiator/noop.c                |   7 ++
 negotiator/skipping.c            |   8 ++
 remote.c                         |  10 ++
 remote.h                         |   2 +
 send-pack.c                      |  39 +++++--
 send-pack.h                      |   2 +
 t/t5510-fetch.sh                 | 191 +++++++++++++++++++++++++++++++
 t/t5516-fetch-push.sh            |  32 +++++-
 t/t5702-protocol-v2.sh           |   4 +-
 transport-helper.c               |   5 +-
 transport.c                      |  20 +++-
 transport.h                      |   7 +-
 21 files changed, 564 insertions(+), 62 deletions(-)

base-commit: 6e8d538aab8fe4dd07ba9fb87b5c7edcfa5706ad
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2085%2Fderrickstolee%2Fmust-have-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2085/derrickstolee/must-have-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/2085

Range-diff vs v5:

1: 538913a327 ! 1: c8c422f646 t5516: fix test order flakiness
@@ Commit message

Use 'sort -k 3' to match the actual number of columns in the output.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## t/t5516-fetch-push.sh ##
2: 580aa58943 ! 2: ac3e8f74d9 fetch: add --negotiation-restrict option
@@ Commit message
translatable with the option name inserted by formatting. At least one of these messages will be reused later for a new option.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## Documentation/config/fetch.adoc ##
3: eee0543647 ! 3: 5206640b8b transport: rename negotiation_tips
@@ Commit message
Also update the string_list used to store the inputs from command-line options.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## builtin/fetch.c ##
4: 63c675e93e ! 4: eec0f90e02 remote: add remote.*.negotiationRestrict config
@@ Commit message
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.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## Documentation/config/remote.adoc ##
5: d423c56283 ! 5: 840db1d957 negotiator: add have_sent() interface
@@ Commit message
common, so the implementation is quite simple. This logic will be exercised in the next change.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## fetch-negotiator.h ##
6: e86c9791e2 ! 6: 62e5ef1a4b fetch: add --negotiation-include option for negotiation
@@ Commit message

Also add --negotiation-include to 'git pull' passthrough options.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## Documentation/fetch-options.adoc ##
7: e5714115b5 ! 7: 05a4b69b9b remote: add remote.*.negotiationInclude config
@@ Commit message
list to allow ignoring earlier config values, such as those that might be set in system or global config.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## Documentation/config/remote.adoc ##
8: ed0be32e2c ! 8: c69ca2e919 send-pack: pass negotiation config in push
@@ Commit message
are passed as --negotiation-include to ensure their tips are always sent as 'have' lines during push negotiation.

- Reviewed-by: Matthew John Cheetham <[email protected]>
+ Reviewed-by: Matthew John Cheetham <[email protected]>

## Documentation/config/remote.adoc ##

Signature · 1 line
gitgitgadget
#76 lore raw

PATCH v6 1/8 t5516: fix test order flakiness

From: Derrick Stolee <[email protected]>

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 3f763ddf28 (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.

---
 t/t5516-fetch-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/t5516-fetch-push.sh +1 -1
index 29e2f17608..ac8447f21e 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#77 lore raw

PATCH v6 2/8 fetch: add --negotiation-restrict option

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

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.

---
 Documentation/config/fetch.adoc  |  2 +-
 Documentation/fetch-options.adoc |  6 +++++-
 builtin/fetch.c                  | 13 ++++++++-----
 builtin/pull.c                   |  3 ++-
 send-pack.c                      |  2 +-
 t/t5510-fetch.sh                 | 25 +++++++++++++++++++++++++
 t/t5702-protocol-v2.sh           |  4 ++--
 transport-helper.c               |  3 ++-
 8 files changed, 46 insertions(+), 12 deletions(-)
Documentation/config/fetch.adoc +1 -1
index cd40db0cad..04ac90912d 100644
@@ -76,7 +76,7 @@
 	default is `skipping`.  Unknown values will cause `git fetch` to
 	error out.
 +
-See also the `--negotiate-only` and `--negotiation-tip` options to
+See also the `--negotiate-only` and `--negotiation-restrict` options to
 linkgit:git-fetch[1].
 
 `fetch.showForcedUpdates`::
Documentation/fetch-options.adoc +5 -1
index 81a9d7f9bb..d39cecb446 100644
@@ -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.
 +
@@ -71,7 +75,7 @@ configuration variables documented in linkgit:git-config[1], and the
 
 `--negotiate-only`::
 	Do not fetch anything from the server, and instead print the
-	ancestors of the provided `--negotiation-tip=` arguments,
+	ancestors of the provided `--negotiation-restrict=` arguments,
 	which we have in common with the server.
 +
 This is incompatible with `--recurse-submodules=(yes|on-demand)`.
builtin/fetch.c +8 -5
index 4795b2a13c..fc950fe35b 100644
@@ -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;
 }
@@ -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;
 }
@@ -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),
@@ -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)
builtin/pull.c +2 -1
index 7e67fdce97..cc6ce485fc 100644
@@ -996,9 +996,10 @@ int cmd_pull(int argc,
 		OPT_PASSTHRU('6',  "ipv6", &opt_ipv6, NULL,
 			N_("use IPv6 addresses only"),
 			PARSE_OPT_NOARG),
-		OPT_PASSTHRU_ARGV(0, "negotiation-tip", &opt_fetch, N_("revision"),
+		OPT_PASSTHRU_ARGV(0, "negotiation-restrict", &opt_fetch, N_("revision"),
 			N_("report that we have only objects reachable from this object"),
 			0),
+		OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
 		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,
send-pack.c +1 -1
index 67d6987b1c..3d5d36ba3b 100644
@@ -447,7 +447,7 @@ static void get_commons_through_negotiation(struct repository *r,
 	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",
+			strvec_pushf(&child.args, "--negotiation-restrict=%s",
 				     oid_to_hex(&ref->new_oid));
 			nr_negotiation_tip++;
 		}
t/t5510-fetch.sh +25 -0
index 5dcb4b51a4..dc3ce56d84 100755
@@ -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 &&
 	(
t/t5702-protocol-v2.sh +2 -2
index f826ac46a5..9f6cf4142d 100755
@@ -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 \
transport-helper.c +2 -1
index 4d95d84f9e..dd78d40668 100644
@@ -755,7 +755,8 @@ static int fetch_refs(struct transport *transport,
 	}
 
 	if (data->transport_options.negotiation_tips)
-		warning("Ignoring --negotiation-tip because the protocol does not support it.");
+		warning(_("ignoring %s because the protocol does not support it."),
+			"--negotiation-restrict");
 
 	if (data->fetch)
 		return fetch_with_fetch(transport, nr_heads, to_fetch);
Signature · 1 line
gitgitgadget
#78 lore raw

PATCH v6 3/8 transport: rename negotiation_tips

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

---
 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(-)
builtin/fetch.c +9 -9
index fc950fe35b..2ba0051d52 100644
@@ -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;
 
 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,
 		};
@@ -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=*");
 
fetch-pack.c +9 -9
index 6ecd468ef7..baf239adf9 100644
@@ -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);
 
fetch-pack.h +2 -2
index 9d3470366f..6c70c942c2 100644
@@ -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[],
transport-helper.c +1 -1
index dd78d40668..f4388da766 100644
@@ -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 %s because the protocol does not support it."),
 			"--negotiation-restrict");
 
transport.c +5 -5
index 107f4fa5dc..a3051f6733 100644
@@ -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);
transport.h +2 -2
index 892f19454a..cdeb33c16f 100644
@@ -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
Signature · 1 line
gitgitgadget
#79 lore raw

PATCH v6 4/8 remote: add remote.*.negotiationRestrict config

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

---
 Documentation/config/remote.adoc | 18 ++++++++++++++++++
 builtin/fetch.c                  | 28 +++++++++++++++++++++-------
 remote.c                         |  5 +++++
 remote.h                         |  1 +
 t/t5510-fetch.sh                 | 26 ++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 7 deletions(-)
Documentation/config/remote.adoc +18 -0
index 91e46f66f5..4dcf81fbce 100644
@@ -107,6 +107,24 @@ 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`, 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.
builtin/fetch.c +21 -7
index 2ba0051d52..a957739f37 100644
@@ -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;
 }
@@ -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,14 +2758,19 @@ 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->acked_commits = &acked_commits;
-		} else {
+
+		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;
+
 		if (server_options.nr)
 			gtransport->server_options = &server_options;
 		result = transport_fetch_refs(gtransport, NULL);
remote.c +5 -0
index 7ca2a6501b..620086e16e 100644
@@ -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,9 @@ 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")) {
+		return parse_transport_option(key, value,
+					      &remote->negotiation_restrict);
 	} else if (!strcmp(subkey, "followremotehead")) {
 		const char *no_warn_branch;
 		if (!strcmp(value, "never"))
remote.h +1 -0
index fc052945ee..e6ec37c393 100644
@@ -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;
t/t5510-fetch.sh +26 -0
index dc3ce56d84..eff3ce8e2d 100755
@@ -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 &&
 	(
Signature · 1 line
gitgitgadget
#80 lore raw

PATCH v6 5/8 negotiator: add have_sent() interface

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

---
 fetch-negotiator.h    | 9 +++++++++
 negotiator/default.c  | 8 ++++++++
 negotiator/noop.c     | 7 +++++++
 negotiator/skipping.c | 8 ++++++++
 4 files changed, 32 insertions(+)
fetch-negotiator.h +9 -0
index e348905a1f..6ca422a064 100644
@@ -47,6 +47,15 @@ struct fetch_negotiator {
 	 */
 	int (*ack)(struct fetch_negotiator *, struct commit *);
 
+	/*
+	 * Inform the negotiator that this commit has already been sent as
+	 * a "have" line outside of the negotiator's control. The negotiator
+	 * should avoid outputting it from next() and may use it to optimize
+	 * further negotiation (e.g., by treating it and its ancestors as
+	 * common).
+	 */
+	void (*have_sent)(struct fetch_negotiator *, struct commit *);
+
 	void (*release)(struct fetch_negotiator *);
 
 	/* internal use */
negotiator/default.c +8 -0
index 116dedcf83..05ab616f39 100644
@@ -175,6 +175,13 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
 	return known_to_be_common;
 }
 
+static void have_sent(struct fetch_negotiator *n, struct commit *c)
+{
+	if (repo_parse_commit(the_repository, c))
+		return;
+	mark_common(n->data, c, 0, 0);
+}
+
 static void release(struct fetch_negotiator *n)
 {
 	clear_prio_queue(&((struct negotiation_state *)n->data)->rev_list);
@@ -188,6 +195,7 @@ void default_negotiator_init(struct fetch_negotiator *negotiator)
 	negotiator->add_tip = add_tip;
 	negotiator->next = next;
 	negotiator->ack = ack;
+	negotiator->have_sent = have_sent;
 	negotiator->release = release;
 	negotiator->data = CALLOC_ARRAY(ns, 1);
 	ns->rev_list.compare = compare_commits_by_commit_date;
negotiator/noop.c +7 -0
index 65e3c20008..edf1b456f3 100644
@@ -29,6 +29,12 @@ static int ack(struct fetch_negotiator *n UNUSED, struct commit *c UNUSED)
 	return 0;
 }
 
+static void have_sent(struct fetch_negotiator *n UNUSED,
+		      struct commit *c UNUSED)
+{
+	/* nothing to do */
+}
+
 static void release(struct fetch_negotiator *n UNUSED)
 {
 	/* nothing to release */
@@ -40,6 +46,7 @@ void noop_negotiator_init(struct fetch_negotiator *negotiator)
 	negotiator->add_tip = add_tip;
 	negotiator->next = next;
 	negotiator->ack = ack;
+	negotiator->have_sent = have_sent;
 	negotiator->release = release;
 	negotiator->data = NULL;
 }
negotiator/skipping.c +8 -0
index 0a272130fb..69472c58e1 100644
@@ -243,6 +243,13 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
 	return known_to_be_common;
 }
 
+static void have_sent(struct fetch_negotiator *n, struct commit *c)
+{
+	if (repo_parse_commit(the_repository, c))
+		return;
+	mark_common(n->data, c);
+}
+
 static void release(struct fetch_negotiator *n)
 {
 	struct data *data = n->data;
@@ -259,6 +266,7 @@ void skipping_negotiator_init(struct fetch_negotiator *negotiator)
 	negotiator->add_tip = add_tip;
 	negotiator->next = next;
 	negotiator->ack = ack;
+	negotiator->have_sent = have_sent;
 	negotiator->release = release;
 	negotiator->data = CALLOC_ARRAY(data, 1);
 	data->rev_list.compare = compare;
Signature · 1 line
gitgitgadget
#81 lore raw

PATCH v6 6/8 fetch: add --negotiation-include option for negotiation

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

---
 Documentation/fetch-options.adoc | 19 +++++++
 builtin/fetch.c                  | 38 ++++++++++---
 builtin/pull.c                   |  3 ++
 fetch-pack.c                     | 81 +++++++++++++++++++++++++---
 fetch-pack.h                     |  6 ++-
 t/t5510-fetch.sh                 | 91 ++++++++++++++++++++++++++++++++
 transport.c                      |  8 ++-
 transport.h                      |  5 +-
 8 files changed, 232 insertions(+), 19 deletions(-)
Documentation/fetch-options.adoc +19 -0
index d39cecb446..7b897a7202 100644
@@ -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=(<commit>|<glob>)`::
+	Ensure that the commits at the given tips are always sent as "have"
+	lines 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 commit is sent
+unconditionally.
++
+The argument may be an exact ref name (e.g. `refs/heads/release`), an
+object hash, 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`.
+
 `--negotiate-only`::
 	Do not fetch anything from the server, and instead print the
 	ancestors of the provided `--negotiation-restrict=` arguments,
builtin/fetch.c +30 -8
index a957739f37..ba56e9022b 100644
@@ -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;
@@ -1534,23 +1535,29 @@ static int add_oid(const struct reference *ref, void *cb_data)
 	return 0;
 }
 
-static void add_negotiation_restrict_tips(struct git_transport_options *smart_options)
+static void add_negotiation_tips(struct string_list *input_list,
+				 struct oid_array **output_list,
+				 const char *argname)
 {
 	struct oid_array *oids = xcalloc(1, sizeof(*oids));
 	int i;
 
-	for (i = 0; i < negotiation_restrict.nr; i++) {
-		const char *s = negotiation_restrict.items[i].string;
+	for (i = 0; i < input_list->nr; i++) {
+		const char *s = input_list->items[i].string;
 		struct refs_for_each_ref_options opts = {
 			.pattern = s,
 		};
 		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;
 		}
@@ -1559,9 +1566,9 @@ static void add_negotiation_restrict_tips(struct git_transport_options *smart_op
 				      add_oid, oids, &opts);
 		if (old_nr == oids->nr)
 			warning(_("ignoring %s=%s because it does not match any refs"),
-				"--negotiation-restrict", s);
+				argname, s);
 	}
-	smart_options->negotiation_restrict_tips = oids;
+	*output_list = oids;
 }
 
 static struct transport *prepare_transport(struct remote *remote, int deepen,
@@ -1597,7 +1604,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
 	}
 	if (negotiation_restrict.nr) {
 		if (transport->smart_options)
-			add_negotiation_restrict_tips(transport->smart_options);
+			add_negotiation_tips(&negotiation_restrict,
+					     &transport->smart_options->negotiation_restrict_tips,
+					     "--negotiation-restrict");
 		else
 			warning(_("ignoring %s because the protocol does not support it"),
 				"--negotiation-restrict");
@@ -1606,7 +1615,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
 		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);
+			add_negotiation_tips(&negotiation_restrict,
+					     &transport->smart_options->negotiation_restrict_tips,
+					     "--negotiation-restrict");
 		else {
 			struct strbuf config_name = STRBUF_INIT;
 			strbuf_addf(&config_name, "remote.%s.negotiationRestrict", remote->name);
@@ -1615,6 +1626,15 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
 			strbuf_release(&config_name);
 		}
 	}
+	if (negotiation_include.nr) {
+		if (transport->smart_options)
+			add_negotiation_tips(&negotiation_include,
+					     &transport->smart_options->negotiation_include_tips,
+					     "--negotiation-include");
+		else
+			warning(_("ignoring %s because the protocol does not support it"),
+				"--negotiation-include");
+	}
 	return transport;
 }
 
@@ -2582,6 +2602,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),
builtin/pull.c +3 -0
index cc6ce485fc..d49b09114a 100644
@@ -1000,6 +1000,9 @@ int cmd_pull(int argc,
 			N_("report that we have only objects reachable from this object"),
 			0),
 		OPT_ALIAS(0, "negotiation-tip", "negotiation-restrict"),
+		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,
fetch-pack.c +75 -6

186 lines not on this page · raw message

fetch-pack.h +4 -2
index 6c70c942c2..6d0dec7f41 100644
@@ -19,9 +19,10 @@ struct fetch_pack_args {
 
 	/*
 	 * If not NULL, during packfile negotiation, fetch-pack will send "have"
-	 * lines only with these tips and their ancestors.
+	 * lines for all _include_ tips and then a subset of the _restrict_ tips.
 	 */
 	const struct oid_array *negotiation_restrict_tips;
+	const struct oid_array *negotiation_include_tips;
 
 	unsigned deepen_relative:1;
 	unsigned quiet:1;
@@ -93,7 +94,8 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
 			   const struct string_list *server_options,
 			   int stateless_rpc,
 			   int fd[],
-			   struct oidset *acked_commits);
+			   struct oidset *acked_commits,
+			   const struct oid_array *negotiation_include_tips);
 
 /*
  * Print an appropriate error message for each sought ref that wasn't
t/t5510-fetch.sh +91 -0

103 lines not on this page · raw message

transport.c +7 -1
index a3051f6733..fa54928966 100644
@@ -464,6 +464,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.stateless_rpc = transport->stateless_rpc;
 	args.server_options = transport->server_options;
 	args.negotiation_restrict_tips = data->options.negotiation_restrict_tips;
+	args.negotiation_include_tips = data->options.negotiation_include_tips;
 	args.reject_shallow_remote = transport->smart_options->reject_shallow;
 
 	if (!data->finished_handshake) {
@@ -495,7 +496,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 					      transport->server_options,
 					      transport->stateless_rpc,
 					      data->fd,
-					      data->options.acked_commits);
+					      data->options.acked_commits,
+					      data->options.negotiation_include_tips);
 			ret = 0;
 		}
 		goto cleanup;
@@ -983,6 +985,10 @@ static int disconnect_git(struct transport *transport)
 		oid_array_clear(data->options.negotiation_restrict_tips);
 		free(data->options.negotiation_restrict_tips);
 	}
+	if (data->options.negotiation_include_tips) {
+		oid_array_clear(data->options.negotiation_include_tips);
+		free(data->options.negotiation_include_tips);
+	}
 	list_objects_filter_release(&data->options.filter_options);
 	oid_array_clear(&data->extra_have);
 	oid_array_clear(&data->shallow);
transport.h +3 -2
index cdeb33c16f..97d905ecc0 100644
@@ -40,13 +40,14 @@ struct git_transport_options {
 
 	/*
 	 * This is only used during fetch. See the documentation of
-	 * negotiation_restrict_tips in struct fetch_pack_args.
+	 * these member names in struct fetch_pack_args.
 	 *
-	 * This field is only supported by transports that support connect or
+	 * These fields are only supported by transports that support connect or
 	 * stateless_connect. Set this field directly instead of using
 	 * transport_set_option().
 	 */
 	struct oid_array *negotiation_restrict_tips;
+	struct oid_array *negotiation_include_tips;
 
 	/*
 	 * If allocated, whenever transport_fetch_refs() is called, add known
Signature · 1 line
gitgitgadget
#82 lore raw

PATCH v6 7/8 remote: add remote.*.negotiationInclude config

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

---
 Documentation/config/remote.adoc | 25 ++++++++++++++++
 Documentation/fetch-options.adoc |  4 +++
 builtin/fetch.c                  | 12 ++++++++
 remote.c                         |  5 ++++
 remote.h                         |  1 +
 t/t5510-fetch.sh                 | 49 ++++++++++++++++++++++++++++++++
 6 files changed, 96 insertions(+)
Documentation/config/remote.adoc +25 -0
index 4dcf81fbce..1951df154e 100644
@@ -125,6 +125,31 @@ 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`, 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 references, commit hashes,
+	or ref pattern globs 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`), a
+commit hash, or a glob pattern (e.g. `refs/heads/release/*`).  The
+pattern syntax is the same as for `--negotiation-include`.
++
+These config values are used as defaults for the `--negotiation-include`
+command-line option.  If `--negotiation-include` is specified on the
+command line, then the config values are not used.
++
+This option is additive with the normal negotiation process: the
+negotiation algorithm still runs and advertises its own selected commits,
+but the refs matching `remote.<name>.negotiationInclude` are sent
+unconditionally on top of those heuristically selected commits.
++
+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.
Documentation/fetch-options.adoc +4 -0
index 7b897a7202..8074004377 100644
@@ -91,6 +91,10 @@ 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`.
++
+If this option is not specified on the command line, then any
+`remote.<name>.negotiationInclude` config values for the current remote
+are used instead.
 
 `--negotiate-only`::
 	Do not fetch anything from the server, and instead print the
builtin/fetch.c +12 -0
index ba56e9022b..1af6500c1d 100644
@@ -1634,6 +1634,18 @@ static struct transport *prepare_transport(struct remote *remote, int deepen,
 		else
 			warning(_("ignoring %s because the protocol does not support it"),
 				"--negotiation-include");
+	} else if (remote->negotiation_include.nr) {
+		if (transport->smart_options) {
+			add_negotiation_tips(&remote->negotiation_include,
+					     &transport->smart_options->negotiation_include_tips,
+					     "--negotiation-include");
+		} else {
+			struct strbuf config_name = STRBUF_INIT;
+			strbuf_addf(&config_name, "remote.%s.negotiationInclude", remote->name);
+			warning(_("ignoring %s because the protocol does not support it"),
+				config_name.buf);
+			strbuf_release(&config_name);
+		}
 	}
 	return transport;
 }
remote.c +5 -0
index 620086e16e..6fb5758820 100644
@@ -153,6 +153,7 @@ static struct remote *make_remote(struct remote_state *remote_state,
 	refspec_init_fetch(&ret->fetch);
 	string_list_init_dup(&ret->server_options);
 	string_list_init_dup(&ret->negotiation_restrict);
+	string_list_init_dup(&ret->negotiation_include);
 
 	ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
 		   remote_state->remotes_alloc);
@@ -181,6 +182,7 @@ static void remote_clear(struct remote *remote)
 	FREE_AND_NULL(remote->http_proxy_authmethod);
 	string_list_clear(&remote->server_options, 0);
 	string_list_clear(&remote->negotiation_restrict, 0);
+	string_list_clear(&remote->negotiation_include, 0);
 }
 
 static void add_merge(struct branch *branch, const char *name)
@@ -567,6 +569,9 @@ static int handle_config(const char *key, const char *value,
 	} else if (!strcmp(subkey, "negotiationrestrict")) {
 		return parse_transport_option(key, value,
 					      &remote->negotiation_restrict);
+	} else if (!strcmp(subkey, "negotiationinclude")) {
+		return parse_transport_option(key, value,
+					      &remote->negotiation_include);
 	} else if (!strcmp(subkey, "followremotehead")) {
 		const char *no_warn_branch;
 		if (!strcmp(value, "never"))
remote.h +1 -0
index e6ec37c393..d8809b6991 100644
@@ -118,6 +118,7 @@ struct remote {
 
 	struct string_list server_options;
 	struct string_list negotiation_restrict;
+	struct string_list negotiation_include;
 
 	enum follow_remote_head_settings follow_remote_head;
 	const char *no_warn_branch;
t/t5510-fetch.sh +49 -0

55 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#83 lore raw

PATCH v6 8/8 send-pack: pass negotiation config in push

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

---
 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(-)
Documentation/config/remote.adoc +6 -0
index 1951df154e..eb9c8a3c48 100644
@@ -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.
 
@@ -147,6 +150,9 @@ negotiation algorithm still runs and advertises its own selected commits,
 but the refs matching `remote.<name>.negotiationInclude` are sent
 unconditionally on top of those heuristically selected commits.
 +
+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.
 
send-pack.c +30 -7
index 3d5d36ba3b..d18e030ce8 100644
@@ -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);
 	}
 
send-pack.h +2 -0
index c5ded2d200..13850c98bb 100644
@@ -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,
t/t5516-fetch-push.sh +30 -0
index ac8447f21e..177cbc6c75 100755
@@ -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 &&
 
transport.c +2 -0
index fa54928966..a2d8958cb8 100644
@@ -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;
Signature · 1 line
gitgitgadget
#85 lore raw

Re: [PATCH v6 0/8] fetch: rework negotiation tip options

Quoted 7 lines · Matthew John Cheetham
Quoted 5 lines · Derrick Stolee via GitGitGadget

Updates in v6
=============

Corrected reviewed-by annotations in commit messages.

Quoted 7 lines
Quoted 2 lines

Thanks, -Stolee

v6 look good to me!

Thanks,
Matthew

Thanks.