[RFC] fetch-pack: respect --no-update-shallow in v2

Commit Message

Jonathan Tan Dec. 20, 2018, 7:53 p.m. UTC

While investigating t5537's failure revealed by Aevar's
GIT_TEST_PROTOCOL_VERSION patches [1], I found an answer to my confusion
that I described in [2]. Quoting the relevant part:

> I'm also not sure why this issue only occurs with protocol v2. It's true
> that, when using protocol v0, the server can communicate shallow
> information both before and after "want"s are received, and if shallow
> communication is only communicated before, the client will invoke
> setup_temporary_shallow() instead of setup_alternate_shallow(). (In
> protocol v2, shallow information is always communicated after "want"s,
> thus demonstrating the issue.) But even in protocol v0, writes to the
> shallow file go through setup_alternate_shallow() anyway (in
> update_shallow()), so I would think that the issue would occur, but it
> doesn't.

It turns out that update_shallow() doesn't write pre-"want" shallow
information unless --update-shallow is set. But post-"want" shallow
information is always written regardless of --update-shallow. (So maybe
--update-shallow is not the best name for it, but that is another
issue.)

Which raises the question: in protocol v2, all shallow information is
post-"want", so how should we handle this? My current inclination is to
treat them like v0 pre-"want" information if no depth-like argument is
given by the user (that is, args->deepen is 0), and like v0 post-"want"
information otherwise. (Right now, it is treated always like the
latter.) This patch does it by delaying initialization of the struct
shallow_info until we fully know what's going on, and not only fixes the
bug revealed by t5537, but the bug that [2] fixes as well (eliminating
the need for [2]).

This does mean that there will be some difference in functionality
between v0 and v2: if fetching from a shallow repository with our own
depth arguments, v0 would write shallows resulting from our own depth
arguments but not those that are there because the remote is shallow; v2
would write all shallows. But this is the best solution I can think of
so far.

Any thoughts? The code can be written more clearly but I wanted to
discuss the design first.

[1] https://public-inbox.org/git/[e-mail]/
[2] https://public-inbox.org/git/[e-mail]/

Signed-off-by: Jonathan Tan <[e-mail]>
---
 fetch-pack.c           | 34 ++++++++++++++++++++++++++--------
 t/t5702-protocol-v2.sh | 16 ++++++++++++++++
 2 files changed, 42 insertions(+), 8 deletions(-)

Patch

diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e64..0a89a210b0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1265,8 +1265,11 @@  static int process_acks(struct fetch_negotiator *negotiator,
 }
 
 static void receive_shallow_info(struct fetch_pack_args *args,
-				 struct packet_reader *reader)
+				 struct packet_reader *reader,
+				 struct shallow_info *si)
 {
+	struct oid_array shallows = OID_ARRAY_INIT;
+
 	process_section_header(reader, "shallow-info", 0);
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
 		const char *arg;
@@ -1275,7 +1278,9 @@  static void receive_shallow_info(struct fetch_pack_args *args,
 		if (skip_prefix(reader->line, "shallow ", &arg)) {
 			if (get_oid_hex(arg, &oid))
 				die(_("invalid shallow line: %s"), reader->line);
-			register_shallow(the_repository, &oid);
+			if (args->deepen)
+				register_shallow(the_repository, &oid);
+			oid_array_append(&shallows, &oid);
 			continue;
 		}
 		if (skip_prefix(reader->line, "unshallow ", &arg)) {
@@ -1297,8 +1302,17 @@  static void receive_shallow_info(struct fetch_pack_args *args,
 	    reader->status != PACKET_READ_DELIM)
 		die(_("error processing shallow info: %d"), reader->status);
 
-	setup_alternate_shallow(&shallow_lock, &alternate_shallow_file, NULL);
-	args->deepen = 1;
+	if (args->deepen) {
+		prepare_shallow_info(si, NULL);
+		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
+					NULL);
+	} else {
+		prepare_shallow_info(si, &shallows);
+		if (si->nr_ours || si->nr_theirs)
+			alternate_shallow_file = setup_temporary_shallow(si->shallow);
+		else
+			alternate_shallow_file = NULL;
+	}
 }
 
 static void receive_wanted_refs(struct packet_reader *reader,
@@ -1340,6 +1354,7 @@  static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				    int fd[2],
 				    const struct ref *orig_ref,
 				    struct ref **sought, int nr_sought,
+				    struct shallow_info *si,
 				    char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -1407,7 +1422,9 @@  static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_GET_PACK:
 			/* Check for shallow-info section */
 			if (process_section_header(&reader, "shallow-info", 1))
-				receive_shallow_info(args, &reader);
+				receive_shallow_info(args, &reader, si);
+			else
+				prepare_shallow_info(si, NULL);
 
 			if (process_section_header(&reader, "wanted-refs", 1))
 				receive_wanted_refs(&reader, sought, nr_sought);
@@ -1641,13 +1658,14 @@  struct ref *fetch_pack(struct fetch_pack_args *args,
 		packet_flush(fd[1]);
 		die(_("no matching remote head"));
 	}
-	prepare_shallow_info(&si, shallow);
 	if (version == protocol_v2)
 		ref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,
-					   pack_lockfile);
-	else
+					   &si, pack_lockfile);
+	else {
+		prepare_shallow_info(&si, shallow);
 		ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
 					&si, pack_lockfile);
+	}
 	reprepare_packed_git(the_repository);
 
 	if (!args->cloning && args->deepen) {
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..390a71399d 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -471,6 +471,22 @@  test_expect_success 'upload-pack respects client shallows' '
 	grep "fetch< version 2" trace
 '
 
+test_expect_success '2 fetches in one process updating shallow' '
+	rm -rf server client trace &&
+
+	test_create_repo server &&
+	test_commit -C server one &&
+	test_commit -C server two &&
+	test_commit -C server three &&
+	git clone --shallow-exclude two "file://$(pwd)/server" client &&
+
+	# Triggers tag following (thus, 2 fetches in one process)
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+		fetch --shallow-exclude one origin &&
+	# Ensure that protocol v2 is used
+	grep "fetch< version 2" trace
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh