Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Apr 22, 2020, 18:02 UTC
- Message-ID
- <20200422180245.196132-1-jonathantanmy@google.com>
- In-Reply-To
- <8d295389ea43c6b7e008514067b7af6eacba64a5.1587492422.git.me@ttaylorr.com>
Show 11 quoted lines
> 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.
Show 8 quoted lines
> @@ -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.
Show 11 quoted lines
> +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 <jonathantanmy@google.com> Date: Wed, 22 Apr 2020 10:53:30 -0700 Subject: [PATCH] fixup
Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- 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