git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: git@vger.kernel.org
Cc: Jonathan Tan <jonathantanmy@google.com>, avarab@gmail.com
Subject: [PATCH] shallow: clear shallow cache when committing lock
Date: Mon, 17 Dec 2018 17:08:11 -0800	[thread overview]
Message-ID: <20181218010811.143608-1-jonathantanmy@google.com> (raw)

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


             reply	other threads:[~2018-12-18  1:08 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-18  1:08 Jonathan Tan [this message]
2018-12-18  3:46 ` [PATCH] shallow: clear shallow cache when committing lock Jonathan Nieder
2018-12-20 19:53 ` [RFC PATCH] fetch-pack: respect --no-update-shallow in v2 Jonathan Tan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181218010811.143608-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).