[PATCH] shallow: clear shallow cache when committing lock

[PATCH] shallow: clear shallow cache when committing lock

[Jonathan Tan]

When setup_alternate_shallow() is invoked a second time in the same
process, it fails with the error "shallow file has changed since we read
it". One way of reproducing this is to fetch using protocol v2 in such a
way that backfill_tags() is required, and that the responses to both
requests contain a "shallow-info" section.

This is because when the shallow lock is committed, the stat information
of the shallow file is not cleared. Ensure that this information is
always cleared whenever the shallow lock is committed by introducing a
new API that hides the shallow lock behind a custom struct.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This was discovered with the help of Aevar's GIT_TEST_PROTOCOL_VERSION
patches.

If rebased onto Aevar's GIT_TEST_PROTOCOL_VERSION patches [1], all tests
still pass with GIT_TEST_PROTOCOL_VERSION=2 and without. Also, this
patch allows the following diff to be applied:

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 8b1217ea26..8baa57cf84 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -791,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '
>  '
>
>  test_expect_success 'fetch exclude tag one' '
> -       GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&
> +       git -C shallow12 fetch --shallow-exclude one origin &&
>         git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
>         test_write_lines three two >expected &&
>         test_cmp expected actual

(There is still one more instance of GIT_TEST_PROTOCOL_VERSION in that
file that is still not fixed.)

I couldn't figure out if the test case included in this patch can be
reduced - if any one has any idea, help is appreciated.

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.

[1] https://public-inbox.org/git/20181213155817.27666-1-avarab@gmail.com/
---
 builtin/receive-pack.c |  7 +++----
 commit.h               |  8 +++++++-
 fetch-pack.c           | 13 ++++++-------
 shallow.c              | 25 +++++++++++++++++++++----
 t/t5702-protocol-v2.sh | 16 ++++++++++++++++
 5 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 33187bd8e9..ab7bc5ceb1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1,7 +1,6 @@
 #include "builtin.h"
 #include "repository.h"
 #include "config.h"
-#include "lockfile.h"
 #include "pack.h"
 #include "refs.h"
 #include "pkt-line.h"
@@ -864,7 +863,7 @@ static void refuse_unconfigured_deny_delete_current(void)
 static int command_singleton_iterator(void *cb_data, struct object_id *oid);
 static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 {
-	struct lock_file shallow_lock = LOCK_INIT;
+	struct shallow_lock shallow_lock;
 	struct oid_array extra = OID_ARRAY_INIT;
 	struct check_connected_options opt = CHECK_CONNECTED_INIT;
 	uint32_t mask = 1 << (cmd->index % 32);
@@ -881,12 +880,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	opt.env = tmp_objdir_env(tmp_objdir);
 	setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
 	if (check_connected(command_singleton_iterator, cmd, &opt)) {
-		rollback_lock_file(&shallow_lock);
+		rollback_shallow_lock(&shallow_lock);
 		oid_array_clear(&extra);
 		return -1;
 	}
 
-	commit_lock_file(&shallow_lock);
+	commit_shallow_lock(&shallow_lock);
 
 	/*
 	 * Make sure setup_alternate_shallow() for the next ref does
diff --git a/commit.h b/commit.h
index 98664536cb..233a30ceef 100644
--- a/commit.h
+++ b/commit.h
@@ -9,6 +9,7 @@
 #include "string-list.h"
 #include "pretty.h"
 #include "commit-slab.h"
+#include "lockfile.h"
 
 #define COMMIT_NOT_FROM_GRAPH 0xFFFFFFFF
 #define GENERATION_NUMBER_INFINITY 0xFFFFFFFF
@@ -224,9 +225,14 @@ extern struct commit_list *get_shallow_commits_by_rev_list(
 extern void set_alternate_shallow_file(struct repository *r, const char *path, int override);
 extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 				 const struct oid_array *extra);
-extern void setup_alternate_shallow(struct lock_file *shallow_lock,
+struct shallow_lock {
+	struct lock_file lock;
+};
+extern void setup_alternate_shallow(struct shallow_lock *shallow_lock,
 				    const char **alternate_shallow_file,
 				    const struct oid_array *extra);
+extern int commit_shallow_lock(struct shallow_lock *shallow_lock);
+extern void rollback_shallow_lock(struct shallow_lock *shallow_lock);
 extern const char *setup_temporary_shallow(const struct oid_array *extra);
 extern void advertise_shallow_grafts(int);
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e64..1ae1cf225a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1,7 +1,6 @@
 #include "cache.h"
 #include "repository.h"
 #include "config.h"
-#include "lockfile.h"
 #include "refs.h"
 #include "pkt-line.h"
 #include "commit.h"
@@ -34,7 +33,7 @@ static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static int agent_supported;
 static int server_supports_filtering;
-static struct lock_file shallow_lock;
+static struct shallow_lock shallow_lock;
 static const char *alternate_shallow_file;
 static char *negotiation_algorithm;
 static struct strbuf fsck_msg_types = STRBUF_INIT;
@@ -1512,9 +1511,9 @@ static void update_shallow(struct fetch_pack_args *args,
 	if (args->deepen && alternate_shallow_file) {
 		if (*alternate_shallow_file == '\0') { /* --unshallow */
 			unlink_or_warn(git_path_shallow(the_repository));
-			rollback_lock_file(&shallow_lock);
+			rollback_shallow_lock(&shallow_lock);
 		} else
-			commit_lock_file(&shallow_lock);
+			commit_shallow_lock(&shallow_lock);
 		return;
 	}
 
@@ -1537,7 +1536,7 @@ static void update_shallow(struct fetch_pack_args *args,
 			setup_alternate_shallow(&shallow_lock,
 						&alternate_shallow_file,
 						&extra);
-			commit_lock_file(&shallow_lock);
+			commit_shallow_lock(&shallow_lock);
 		}
 		oid_array_clear(&extra);
 		return;
@@ -1574,7 +1573,7 @@ static void update_shallow(struct fetch_pack_args *args,
 		setup_alternate_shallow(&shallow_lock,
 					&alternate_shallow_file,
 					&extra);
-		commit_lock_file(&shallow_lock);
+		commit_shallow_lock(&shallow_lock);
 		oid_array_clear(&extra);
 		oid_array_clear(&ref);
 		return;
@@ -1660,7 +1659,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 			error(_("remote did not send all necessary objects"));
 			free_refs(ref_cpy);
 			ref_cpy = NULL;
-			rollback_lock_file(&shallow_lock);
+			rollback_shallow_lock(&shallow_lock);
 			goto cleanup;
 		}
 		args->connectivity_checked = 1;
diff --git a/shallow.c b/shallow.c
index 02fdbfc554..0d39906419 100644
--- a/shallow.c
+++ b/shallow.c
@@ -333,22 +333,23 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
 	return "";
 }
 
-void setup_alternate_shallow(struct lock_file *shallow_lock,
+void setup_alternate_shallow(struct shallow_lock *shallow_lock,
 			     const char **alternate_shallow_file,
 			     const struct oid_array *extra)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(shallow_lock,
+	fd = hold_lock_file_for_update(&shallow_lock->lock,
 				       git_path_shallow(the_repository),
 				       LOCK_DIE_ON_ERROR);
 	check_shallow_file_for_update(the_repository);
 	if (write_shallow_commits(&sb, 0, extra)) {
 		if (write_in_full(fd, sb.buf, sb.len) < 0)
 			die_errno("failed to write to %s",
-				  get_lock_file_path(shallow_lock));
-		*alternate_shallow_file = get_lock_file_path(shallow_lock);
+				  get_lock_file_path(&shallow_lock->lock));
+		*alternate_shallow_file =
+			get_lock_file_path(&shallow_lock->lock);
 	} else
 		/*
 		 * is_repository_shallow() sees empty string as "no
@@ -358,6 +359,22 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 	strbuf_release(&sb);
 }
 
+int commit_shallow_lock(struct shallow_lock *shallow_lock)
+{
+	int ret;
+
+	if ((ret = commit_lock_file(&shallow_lock->lock)))
+		return ret;
+	the_repository->parsed_objects->is_shallow = -1;
+	stat_validity_clear(the_repository->parsed_objects->shallow_stat);
+	return 0;
+}
+
+void rollback_shallow_lock(struct shallow_lock *shallow_lock)
+{
+	rollback_lock_file(&shallow_lock->lock);
+}
+
 static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *cb)
 {
 	int fd = *(int *)cb;
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
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH] shallow: clear shallow cache when committing lock

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> When setup_alternate_shallow() is invoked a second time in the same
> process, it fails with the error "shallow file has changed since we read
> it". One way of reproducing this is to fetch using protocol v2 in such a
> way that backfill_tags() is required, and that the responses to both
> requests contain a "shallow-info" section.
>
> This is because when the shallow lock is committed, the stat information
> of the shallow file is not cleared. Ensure that this information is
> always cleared whenever the shallow lock is committed by introducing a
> new API that hides the shallow lock behind a custom struct.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This was discovered with the help of Aevar's GIT_TEST_PROTOCOL_VERSION
> patches.

Good catch.  I think the above note should go in the commit message,
since it would be very useful to anyone looking back at this commit
message and trying to find a quick reproduction recipe.

[...]
> I couldn't figure out if the test case included in this patch can be
> reduced - if any one has any idea, help is appreciated.

It's short enough to be comprehensible, so I wouldn't worry too
much. :)

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

Good question.  I'll try to poke more tomorrow morning, too.

[...]
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1,7 +1,6 @@
>  #include "builtin.h"
>  #include "repository.h"
>  #include "config.h"
> -#include "lockfile.h"
>  #include "pack.h"
>  #include "refs.h"
>  #include "pkt-line.h"
> @@ -864,7 +863,7 @@ static void refuse_unconfigured_deny_delete_current(void)
>  static int command_singleton_iterator(void *cb_data, struct object_id *oid);
>  static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  {
> -	struct lock_file shallow_lock = LOCK_INIT;
> +	struct shallow_lock shallow_lock;

Interesting.  Is there another name that can convey what this object
does more clearly?  At first glance I would expect this to be a less
deep kind of lock.

I'm awful at naming, but a couple of ideas to get things started:

- 'struct shallow_update', since this represents a pending update to
  the .git/shallow file?

- struct lock_file_for_shallow?

- struct locked_shallow_file?

- "struct shallow_file" or "struct shallow_commits_file"?  This is a
  handle representing the state of what will become .git/shallow file
  once the caller commits the update.

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
[...]
> @@ -1660,7 +1659,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  			error(_("remote did not send all necessary objects"));
>  			free_refs(ref_cpy);
>  			ref_cpy = NULL;
> -			rollback_lock_file(&shallow_lock);
> +			rollback_shallow_lock(&shallow_lock);

For a moment, I was worried that this line could be reached without
setup_alternate_shallow having been called first.  Fortunately,
shallow_lock is static so it is zero-initialized automatically, making
that harmless.

[...]
> --- a/shallow.c
> +++ b/shallow.c
[...]
> @@ -358,6 +359,22 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
>  	strbuf_release(&sb);
>  }
>  
> +int commit_shallow_lock(struct shallow_lock *shallow_lock)
> +{
> +	int ret;
> +
> +	if ((ret = commit_lock_file(&shallow_lock->lock)))
> +		return ret;
> +	the_repository->parsed_objects->is_shallow = -1;
> +	stat_validity_clear(the_repository->parsed_objects->shallow_stat);

In 'next', some other functions in this file handle an arbitrary
repository argument 'r'.  Should this one as well?  (I.e. can this
patch come with a conflict-resolution patch to handle that?)

[...]
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -471,6 +471,22 @@ test_expect_success 'upload-pack respects client shallows' '

Yay, thanks for the test.  I'll study it more closely tomorrow.

Thanks and hope that helps,
Jonathan

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

[Jonathan Tan]

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/20181213155817.27666-1-avarab@gmail.com/
[2] https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@google.com/

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

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
-- 
2.19.0.271.gfe8321ec05.dirty