fetch-pack: respect fetch.haveRefs

The previous change documented and loaded  the fetch.haveRefs
multi-valued config, but did not change any behavior. Now, let's update
the behavior for fetch negotiation.

This is a config-only option right now, unlike the --negotiation-tips
command-line options. For this reason, the values are read on-demand
from the repo_settings struct instead of passed down via options. If we
wish to reflect this option in the command line as well, then that
plumbing would be easy to refactor.

One interesting aspect is how this relates to --negotiation-tips
arguments. The --negotation-tips arguments _restricts_ the set of haves
while the fetch.haveRefs configs _increases_ the set of haves. I chose
to have these options done in that order. This helps create tests by
enforcing that the negotiation strategy restricts the set of haves
before the config adds them back in. This provides some simulation of
the intended use case where the haves were restricted due to an
overwhelming number of refs instead of by --negotiation-tips.

The set of fetch.haveRefs are output entirely before considering a batch
for flushing. This should hopefully not be a problem as users _should_
select a small core set of references. While these are output, we add
them to an oidset to avoid duplicates.

During my development, I found that if I try to increment the counters
while outputting these ref tips, then I could get into an infinite loop
where the client and server go back and forth forever. This happened
when the fetch.haveRefs included the entire set of references that could
be selected, which is covered in the tests.

Signed-off-by: Derrick Stolee <stolee@gmail.com>

Author: derrickstolee

Documentation/config/fetch.adoc

@@ -92,6 +92,10 @@ 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-tip`.
 +
+If `--negotiation-tip` is used, then the have set is first restricted by
+that option and then increased to include the tips specified by the
+`fetch.haveRefs` config values.
++
 This option is additive with the normal negotiation process: the
 negotiation algorithm still runs and advertises its own selected commits,
 but the refs matching `fetch.haveRefs` are sent unconditionally on top of

Documentation/fetch-options.adoc

@@ -67,7 +67,8 @@ this option multiple times, one for each matching ref name.
 +
 See also the `fetch.negotiationAlgorithm` and `push.negotiate`
 configuration variables documented in linkgit:git-config[1], and the
-`--negotiate-only` option below.
+`--negotiate-only` option below. Also, these restrictions do not apply
+to any `fetch.haveRefs` config values.
 
 `--negotiate-only`::
 	Do not fetch anything from the server, and instead print the

builtin/fetch.c

@@ -38,6 +38,7 @@
 #include "promisor-remote.h"
 #include "commit-graph.h"
 #include "shallow.h"
+#include "repo-settings.h"
 #include "trace.h"
 #include "trace2.h"
 #include "bundle-uri.h"

fetch-pack.c

@@ -25,9 +25,11 @@
 #include "oidset.h"
 #include "packfile.h"
 #include "odb.h"
+#include "object-name.h"
 #include "path.h"
 #include "connected.h"
 #include "fetch-negotiator.h"
+#include "repo-settings.h"
 #include "fsck.h"
 #include "shallow.h"
 #include "commit-reach.h"
@@ -332,6 +334,40 @@ static void send_filter(struct fetch_pack_args *args,
 	}
 }
 
+static int add_oid_to_oidset(const struct reference *ref, void *cb_data)
+{
+	struct oidset *set = cb_data;
+	oidset_insert(set, ref->oid);
+	return 0;
+}
+
+static void resolve_have_refs(struct string_list *have_refs,
+			      struct oidset *result)
+{
+	struct string_list_item *item;
+
+	if (!have_refs)
+		return;
+
+	for_each_string_list_item(item, have_refs) {
+		if (!has_glob_specials(item->string)) {
+			struct object_id oid;
+			if (repo_get_oid(the_repository, item->string, &oid))
+				continue;
+			if (!odb_has_object(the_repository->objects, &oid, 0))
+				continue;
+			oidset_insert(result, &oid);
+		} else {
+			struct refs_for_each_ref_options opts = {
+				.pattern = item->string,
+			};
+			refs_for_each_ref_ext(
+				get_main_ref_store(the_repository),
+				add_oid_to_oidset, result, &opts);
+		}
+	}
+}
+
 static int find_common(struct fetch_negotiator *negotiator,
 		       struct fetch_pack_args *args,
 		       int fd[2], struct object_id *result_oid,
@@ -347,6 +383,7 @@ static int find_common(struct fetch_negotiator *negotiator,
 	struct strbuf req_buf = STRBUF_INIT;
 	size_t state_len = 0;
 	struct packet_reader reader;
+	struct oidset have_refs_oids = OIDSET_INIT;
 
 	if (args->stateless_rpc && multi_ack == 1)
 		die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
@@ -474,7 +511,26 @@ 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 fetch.haveRefs */
+	prepare_repo_settings(the_repository);
+	resolve_have_refs(the_repository->settings.fetch_have_refs,
+			  &have_refs_oids);
+	if (oidset_size(&have_refs_oids)) {
+		struct oidset_iter iter;
+		oidset_iter_init(&have_refs_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));
+		}
+	}
+
 	while ((oid = negotiator->next(negotiator))) {
+		/* avoid duplicate oids as fetch.haveRefs. */
+		if (oidset_contains(&have_refs_oids, oid))
+			continue;
 		packet_buf_write(&req_buf, "have %s\n", oid_to_hex(oid));
 		print_verbose(args, "have %s", oid_to_hex(oid));
 		in_vain++;
@@ -584,6 +640,7 @@ static int find_common(struct fetch_negotiator *negotiator,
 		flushes++;
 	}
 	strbuf_release(&req_buf);
+	oidset_clear(&have_refs_oids);
 
 	if (!got_ready || !no_done)
 		consume_shallow_list(args, &reader);
@@ -1305,12 +1362,25 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
 
 static int add_haves(struct fetch_negotiator *negotiator,
 		     struct strbuf *req_buf,
-		     int *haves_to_send)
+		     int *haves_to_send,
+		     struct oidset *have_refs_oids)
 {
 	int haves_added = 0;
 	const struct object_id *oid;
 
+	/* Send unconditional haves from fetch.haveRefs */
+	if (have_refs_oids) {
+		struct oidset_iter iter;
+		oidset_iter_init(have_refs_oids, &iter);
+
+		while ((oid = oidset_iter_next(&iter)))
+			packet_buf_write(req_buf, "have %s\n",
+					 oid_to_hex(oid));
+	}
+
 	while ((oid = negotiator->next(negotiator))) {
+		if (have_refs_oids && oidset_contains(have_refs_oids, oid))
+			continue;
 		packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
 		if (++haves_added >= *haves_to_send)
 			break;
@@ -1358,7 +1428,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 			      struct fetch_pack_args *args,
 			      const struct ref *wants, struct oidset *common,
 			      int *haves_to_send, int *in_vain,
-			      int sideband_all, int seen_ack)
+			      int sideband_all, int seen_ack,
+			      struct oidset *have_refs_oids)
 {
 	int haves_added;
 	int done_sent = 0;
@@ -1413,7 +1484,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 	/* Add all of the common commits we've found in previous rounds */
 	add_common(&req_buf, common);
 
-	haves_added = add_haves(negotiator, &req_buf, haves_to_send);
+	haves_added = add_haves(negotiator, &req_buf, haves_to_send,
+			       have_refs_oids);
 	*in_vain += haves_added;
 	trace2_data_intmax("negotiation_v2", the_repository, "haves_added", haves_added);
 	trace2_data_intmax("negotiation_v2", the_repository, "in_vain", *in_vain);
@@ -1657,6 +1729,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	struct ref *ref = copy_ref_list(orig_ref);
 	enum fetch_state state = FETCH_CHECK_LOCAL;
 	struct oidset common = OIDSET_INIT;
+	struct oidset have_refs_oids = OIDSET_INIT;
 	struct packet_reader reader;
 	int in_vain = 0, negotiation_started = 0;
 	int negotiation_round = 0;
@@ -1708,6 +1781,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		reader.me = "fetch-pack";
 	}
 
+	prepare_repo_settings(the_repository);
+	resolve_have_refs(the_repository->settings.fetch_have_refs,
+			  &have_refs_oids);
+
 	while (state != FETCH_DONE) {
 		switch (state) {
 		case FETCH_CHECK_LOCAL:
@@ -1747,7 +1824,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 					       &common,
 					       &haves_to_send, &in_vain,
 					       reader.use_sideband,
-					       seen_ack)) {
+					       seen_ack,
+					       &have_refs_oids)) {
 				trace2_region_leave_printf("negotiation_v2", "round",
 							   the_repository, "%d",
 							   negotiation_round);
@@ -1883,6 +1961,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		negotiator->release(negotiator);
 
 	oidset_clear(&common);
+	oidset_clear(&have_refs_oids);
 	return ref;
 }
 
@@ -2187,6 +2266,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
 	struct packet_reader reader;
 	struct object_array nt_object_array = OBJECT_ARRAY_INIT;
 	struct strbuf req_buf = STRBUF_INIT;
+	struct oidset have_refs_oids = OIDSET_INIT;
 	int haves_to_send = INITIAL_FLUSH;
 	int in_vain = 0;
 	int seen_ack = 0;
@@ -2205,6 +2285,10 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
 			   add_to_object_array,
 			   &nt_object_array);
 
+	prepare_repo_settings(the_repository);
+	resolve_have_refs(the_repository->settings.fetch_have_refs,
+			  &have_refs_oids);
+
 	trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
 	while (!last_iteration) {
 		int haves_added;
@@ -2221,7 +2305,8 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
 
 		packet_buf_write(&req_buf, "wait-for-done");
 
-		haves_added = add_haves(&negotiator, &req_buf, &haves_to_send);
+		haves_added = add_haves(&negotiator, &req_buf, &haves_to_send,
+				       &have_refs_oids);
 		in_vain += haves_added;
 		if (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))
 			last_iteration = 1;
@@ -2273,6 +2358,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
 
 	clear_common_flag(acked_commits);
 	object_array_clear(&nt_object_array);
+	oidset_clear(&have_refs_oids);
 	negotiator.release(&negotiator);
 	strbuf_release(&req_buf);
 }

t/t5510-fetch.sh

@@ -1728,6 +1728,84 @@ test_expect_success REFFILES "HEAD is updated even with conflicts" '
 	)
 '
 
+test_expect_success 'fetch.haveRefs includes configured refs as haves' '
+	test_when_finished git -C client config --unset-all fetch.haveRefs &&
+	test_when_finished rm -f trace &&
+	setup_negotiation_tip server server 0 &&
+
+	# With --negotiation-tip restricting tips, only alpha_1 is
+	# normally sent. Configure fetch.haveRefs to also include beta_1.
+	git -C client config --add fetch.haveRefs refs/tags/beta_1 &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+		--negotiation-tip=alpha_1 \
+		origin alpha_s beta_s &&
+
+	ALPHA_1=$(git -C client rev-parse alpha_1) &&
+	test_grep "fetch> have $ALPHA_1" trace &&
+	BETA_1=$(git -C client rev-parse beta_1) &&
+	test_grep "fetch> have $BETA_1" trace
+'
+
+test_expect_success 'fetch.haveRefs works with glob patterns' '
+	test_when_finished git -C client config --unset-all fetch.haveRefs &&
+	test_when_finished rm -f trace &&
+	setup_negotiation_tip server server 0 &&
+
+	git -C client config --add fetch.haveRefs "refs/tags/beta_*" &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+		--negotiation-tip=alpha_1 \
+		origin alpha_s beta_s &&
+
+	BETA_1=$(git -C client rev-parse beta_1) &&
+	test_grep "fetch> have $BETA_1" trace &&
+	BETA_2=$(git -C client rev-parse beta_2) &&
+	test_grep "fetch> have $BETA_2" trace
+'
+
+test_expect_success 'fetch.haveRefs is additive with negotiation' '
+	test_when_finished git -C client config --unset-all fetch.haveRefs &&
+	test_when_finished rm -f trace &&
+	setup_negotiation_tip server server 0 &&
+
+	# Without --negotiation-tip, all local refs are used as tips.
+	# fetch.haveRefs should add its refs unconditionally on top.
+	git -C client config --add fetch.haveRefs refs/tags/beta_1 &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+		origin alpha_s beta_s &&
+
+	BETA_1=$(git -C client rev-parse beta_1) &&
+	test_grep "fetch> have $BETA_1" trace
+'
+
+test_expect_success 'fetch.haveRefs ignores non-existent refs silently' '
+	test_when_finished git -C client config --unset-all fetch.haveRefs &&
+	setup_negotiation_tip server server 0 &&
+
+	git -C client config --add fetch.haveRefs refs/tags/nonexistent &&
+	git -C client fetch --quiet \
+		--negotiation-tip=alpha_1 \
+		origin alpha_s beta_s 2>err &&
+	test_must_be_empty err
+'
+
+test_expect_success 'fetch.haveRefs avoids duplicates with negotiator' '
+	test_when_finished git -C client config --unset-all fetch.haveRefs &&
+	test_when_finished rm -f trace &&
+	setup_negotiation_tip server server 0 &&
+
+	# Configure a ref that will also be a negotiation tip.
+	# fetch should still complete successfully.
+	ALPHA_1=$(git -C client rev-parse alpha_1) &&
+	git -C client config --add fetch.haveRefs refs/tags/alpha_1 &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+		--negotiation-tip=alpha_1 \
+		origin alpha_s beta_s &&
+
+	# alpha_1 should appear as a have
+	test_grep "fetch> have $ALPHA_1" trace >matches &&
+	test_line_count = 1 matches
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

t/t5516-fetch-push.sh

@@ -254,6 +254,21 @@ test_expect_success 'push with negotiation does not attempt to fetch submodules'
 	! grep "Fetching submodule" err
 '
 
+test_expect_success 'push with negotiation and fetch.haveRefs' '
+	test_when_finished rm -rf haverefs &&
+	mk_empty haverefs &&
+	git push haverefs $the_first_commit:refs/remotes/origin/first_commit &&
+	test_commit -C haverefs unrelated_commit &&
+	git -C haverefs 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 fetch.haveRefs=refs/heads/main \
+		push haverefs 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 &&