From: Jonathan Tan <jonathantanmy@google.com>
To: me@ttaylorr.com
Cc: git@vger.kernel.org, newren@gmail.com, jrnieder@gmail.com,
dstolee@microsoft.com, Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
Date: Wed, 22 Apr 2020 11:02:45 -0700 [thread overview]
Message-ID: <20200422180245.196132-1-jonathantanmy@google.com> (raw)
In-Reply-To: <8d295389ea43c6b7e008514067b7af6eacba64a5.1587492422.git.me@ttaylorr.com>
> 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.
> @@ -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 <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
next prev parent reply other threads:[~2020-04-22 18:02 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-21 18:09 [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Taylor Blau
2020-04-21 20:41 ` Junio C Hamano
2020-04-21 20:45 ` Taylor Blau
2020-04-21 20:52 ` Junio C Hamano
2020-04-21 22:21 ` Taylor Blau
2020-04-21 23:06 ` Junio C Hamano
2020-04-22 18:05 ` Jonathan Tan
2020-04-22 18:02 ` Jonathan Tan [this message]
2020-04-22 18:15 ` Junio C Hamano
2020-04-23 0:14 ` Taylor Blau
2020-04-23 0:25 ` [PATCH v2 0/2] shallow.c: reset shallow-ness after updating Taylor Blau
2020-04-23 0:25 ` [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability Taylor Blau
2020-04-23 1:14 ` Jonathan Nieder
2020-04-24 17:11 ` Taylor Blau
2020-04-24 17:17 ` Jonathan Nieder
2020-04-24 20:45 ` Junio C Hamano
2020-04-23 0:25 ` [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file' Taylor Blau
2020-04-23 1:23 ` Jonathan Nieder
2020-04-23 18:09 ` Jonathan Tan
2020-04-23 20:40 ` Junio C Hamano
2020-04-24 17:13 ` Taylor Blau
2020-06-03 3:42 ` Jonathan Nieder
2020-06-03 4:52 ` Taylor Blau
2020-06-03 5:16 ` Taylor Blau
2020-06-03 13:08 ` Derrick Stolee
2020-06-03 19:26 ` Taylor Blau
2020-06-03 21:23 ` Jonathan Nieder
2020-06-03 20:51 ` Jonathan Nieder
2020-06-03 22:14 ` Taylor Blau
2020-06-03 23:06 ` Jonathan Nieder
2020-06-04 17:45 ` Taylor Blau
2020-04-23 19:05 ` [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200422180245.196132-1-jonathantanmy@google.com \
--to=jonathantanmy@google.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
--cc=me@ttaylorr.com \
--cc=newren@gmail.com \
/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.
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 NNTP newsgroup(s).