Thread (2 messages) 2 messages, 2 authors, 2020-04-22

Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate

From: Jonathan Tan <hidden>
Date: 2020-04-22 18:02:52
Subsystem: the rest · Maintainer: Linus Torvalds

Address this by introducing 'reset_repository_shallow()', and calling
it whenever the shallow files is updated. This happens in two cases:

  * during 'update_shallow', when either the repository is
    un-shallowing, or after commit_lock_file, when the contents of
    .git/shallow is changing, and

  * in 'prune_shallow', when the repository can go from shallow to
    un-shallow when the shallow file is updated, forcing
    'is_repository_shallow' to re-evaluate whether the repository is
    still shallow after fetching in the above scenario.
From a cursory reading of the code, it seems that this happens in
fetch-pack and receive-pack. Looking at those files, I found some more
occasions when this happens. I have outlined them in the patch after the
scissors (I hope I used the scissors correctly).

Maybe instead of enumerating the cases (of which there are quite a few),
say that we do this when we unlink the shallow file, we modify or create
the shallow file, or when we change the value of alternate_shallow_file.
quoted hunk ↗ jump to hunk
@@ -414,6 +414,7 @@ void prune_shallow(unsigned options)
 	} else {
 		unlink(git_path_shallow(the_repository));
 		rollback_lock_file(&shallow_lock);
+		reset_repository_shallow(the_repository);
 	}
 	strbuf_release(&sb);
 }
The "if" part (not quoted here) commits the shallow lock file, and thus
possibly modifies (or creates) the shallow file, so I think we need to
put reset_repository_shallow() outside the whole "if" block. I have done
that in the patch after the scissors.
+test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
+	(
+	cd shallow &&
+	git checkout master &&
+	commit 8 &&
+	git tag -m foo heavy-tag-for-graph HEAD^ &&
+	git tag light-tag-for-graph HEAD^:tracked
+	) &&
+	(
+	cd notshallow &&
+	test_config fetch.writeCommitGraph true &&
When I patched onto master, this line causes the test to fail with a
warning that test_when_finished doesn't work in a subshell. I've
replaced it with a regular "git config" and it works.

Here is the patch containing what I tried. I think that most of the new
reset_repository_shallow() invocations don't change any functionality
(we don't usually read shallow files so many times in a process), so
they can't be tested, but I think that it's better to include them for
completeness, and to close the open question mentioned in bd0b42aed3
("fetch-pack: do not take shallow lock unnecessarily", 2019-01-10)
(about the full solution involving clearing shallow information whenever
we commit the shallow lock - we find here that the full solution
involves this and also clearing shallow information in other cases too).

-- 8< --
From 46a69a133db2e8e948d2bf296294656c9902e5ae Mon Sep 17 00:00:00 2001
From: Jonathan Tan <redacted>
Date: Wed, 22 Apr 2020 10:53:30 -0700
Subject: [PATCH] fixup

Signed-off-by: Jonathan Tan <redacted>
---
 builtin/receive-pack.c   | 1 +
 fetch-pack.c             | 2 ++
 shallow.c                | 2 +-
 t/t5537-fetch-shallow.sh | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2cc18bbffd..d61cbf60e2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -878,6 +878,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	}
 
 	commit_lock_file(&shallow_lock);
+	reset_repository_shallow(the_repository);
 
 	/*
 	 * Make sure setup_alternate_shallow() for the next ref does
diff --git a/fetch-pack.c b/fetch-pack.c
index 684868bc17..9a1cec470c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1657,6 +1657,7 @@ static void update_shallow(struct fetch_pack_args *args,
 						&alternate_shallow_file,
 						&extra);
 			commit_lock_file(&shallow_lock);
+			reset_repository_shallow(the_repository);
 			alternate_shallow_file = NULL;
 		}
 		oid_array_clear(&extra);
@@ -1695,6 +1696,7 @@ static void update_shallow(struct fetch_pack_args *args,
 					&alternate_shallow_file,
 					&extra);
 		commit_lock_file(&shallow_lock);
+		reset_repository_shallow(the_repository);
 		oid_array_clear(&extra);
 		oid_array_clear(&ref);
 		alternate_shallow_file = NULL;
diff --git a/shallow.c b/shallow.c
index 9d1304e786..1a1ca71ffe 100644
--- a/shallow.c
+++ b/shallow.c
@@ -414,8 +414,8 @@ void prune_shallow(unsigned options)
 	} else {
 		unlink(git_path_shallow(the_repository));
 		rollback_lock_file(&shallow_lock);
-		reset_repository_shallow(the_repository);
 	}
+	reset_repository_shallow(the_repository);
 	strbuf_release(&sb);
 }
 
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index c9c731c7a9..c5c40fb8e7 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -187,7 +187,7 @@ test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
 	) &&
 	(
 	cd notshallow &&
-	test_config fetch.writeCommitGraph true &&
+	git config fetch.writeCommitGraph true &&
 	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
 	git fsck &&
 	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
-- 
2.26.1.301.g55bc3eb7cb9-goog
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help