| Message ID | 8d295389ea43c6b7e008514067b7af6eacba64a5.1587492422.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shallow.c: use 'reset_repository_shallow' when appropriate | expand |
Taylor Blau <me@ttaylorr.com> writes: > In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily, > 2019-01-10), the author noted that 'is_repository_shallow' produces > visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'. > > This is a problem for e.g., fetching with '--update-shallow' in a > shallow repsoitory with 'fetch.writeCommitGraph' enabled, since the repository. > update to '.git/shallow' will cause Git to think that the repository > isn't shallow when it is, thereby circumventing the commit-graph > compatability check. > > This causes problems in shallow repositories with at least shallow refs > that have at least one ancestor (since the client won't have those > objects, and therefore can't take the reachability closure over commits > when writing a commit-graph). OK. > 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. > > As a result of the second change, 'prune_shallow' can now only be called > once (since 'check_shallow_file_for_update' will die after calling > 'reset_repository_shallow'). But, this is OK since we only call > 'prune_shallow' at most once per process. > > Helped-by: Jonathan Tan <jonathantanmy@google.com> > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > > Here's a cleaned up version of the patch that we were originally > discussing in > https://lore.kernel.org/git/20200414235057.GA6863@syl.local/, which > addresses some of Jonathan's feedback and adds a test to make sure that > the new behavior is working correctly. > > commit.h | 1 + > fetch-pack.c | 1 + > shallow.c | 15 ++++++++------- > t/t5537-fetch-shallow.sh | 36 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 46 insertions(+), 7 deletions(-) > > diff --git a/commit.h b/commit.h > index 008a0fa4a0..ee1ba139d4 100644 > --- a/commit.h > +++ b/commit.h > @@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid); > int unregister_shallow(const struct object_id *oid); > int for_each_commit_graft(each_commit_graft_fn, void *); > int is_repository_shallow(struct repository *r); > +void reset_repository_shallow(struct repository *r); > struct commit_list *get_shallow_commits(struct object_array *heads, > int depth, int shallow_flag, int not_shallow_flag); > struct commit_list *get_shallow_commits_by_rev_list( > diff --git a/fetch-pack.c b/fetch-pack.c > index 1734a573b0..684868bc17 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1632,6 +1632,7 @@ static void update_shallow(struct fetch_pack_args *args, > rollback_lock_file(&shallow_lock); > } else > commit_lock_file(&shallow_lock); > + reset_repository_shallow(the_repository); > alternate_shallow_file = NULL; > return; > } So, after updating shallow file with "fetch --update-shallow" (or failing to do so), we reset the in-core data. > +void reset_repository_shallow(struct repository *r) > +{ > + r->parsed_objects->is_shallow = -1; > + stat_validity_clear(r->parsed_objects->shallow_stat); > +} OK. > @@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock, > * shallow file". > */ > *alternate_shallow_file = ""; > + reset_repository_shallow(the_repository); > strbuf_release(&sb); > } And also after writing out the alternate shallow file (whether it is empty or polulated). > @@ -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); > } Here, we reset only after we realize we cannot write the updated shallow file. Intended? > + cat <<EOF >expect.refs && > +refs/remotes/shallow/master > +refs/remotes/shallow/no-shallow > +refs/tags/heavy-tag > +refs/tags/heavy-tag-for-graph > +refs/tags/light-tag > +refs/tags/light-tag-for-graph > +EOF cat <<-EOF >expect.refs && ... body can be indented by any number of TAB ... to make it easier to view EOF > + test_cmp expect.refs actual.refs && > + git log --format=%s shallow/master >actual && > + cat <<EOF >expect && Likewise.
Hi Junio, On Tue, Apr 21, 2020 at 01:41:56PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily, > > 2019-01-10), the author noted that 'is_repository_shallow' produces > > visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'. > > > > This is a problem for e.g., fetching with '--update-shallow' in a > > shallow repsoitory with 'fetch.writeCommitGraph' enabled, since the > > repository. Whoops, sorry about that. > > update to '.git/shallow' will cause Git to think that the repository > > isn't shallow when it is, thereby circumventing the commit-graph > > compatability check. > > > > This causes problems in shallow repositories with at least shallow refs > > that have at least one ancestor (since the client won't have those > > objects, and therefore can't take the reachability closure over commits > > when writing a commit-graph). > > OK. > > > 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. > > > > As a result of the second change, 'prune_shallow' can now only be called > > once (since 'check_shallow_file_for_update' will die after calling > > 'reset_repository_shallow'). But, this is OK since we only call > > 'prune_shallow' at most once per process. > > > > Helped-by: Jonathan Tan <jonathantanmy@google.com> > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > > > Here's a cleaned up version of the patch that we were originally > > discussing in > > https://lore.kernel.org/git/20200414235057.GA6863@syl.local/, which > > addresses some of Jonathan's feedback and adds a test to make sure that > > the new behavior is working correctly. > > > > commit.h | 1 + > > fetch-pack.c | 1 + > > shallow.c | 15 ++++++++------- > > t/t5537-fetch-shallow.sh | 36 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 46 insertions(+), 7 deletions(-) > > > > diff --git a/commit.h b/commit.h > > index 008a0fa4a0..ee1ba139d4 100644 > > --- a/commit.h > > +++ b/commit.h > > @@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid); > > int unregister_shallow(const struct object_id *oid); > > int for_each_commit_graft(each_commit_graft_fn, void *); > > int is_repository_shallow(struct repository *r); > > +void reset_repository_shallow(struct repository *r); > > struct commit_list *get_shallow_commits(struct object_array *heads, > > int depth, int shallow_flag, int not_shallow_flag); > > struct commit_list *get_shallow_commits_by_rev_list( > > diff --git a/fetch-pack.c b/fetch-pack.c > > index 1734a573b0..684868bc17 100644 > > --- a/fetch-pack.c > > +++ b/fetch-pack.c > > @@ -1632,6 +1632,7 @@ static void update_shallow(struct fetch_pack_args *args, > > rollback_lock_file(&shallow_lock); > > } else > > commit_lock_file(&shallow_lock); > > + reset_repository_shallow(the_repository); > > alternate_shallow_file = NULL; > > return; > > } > > So, after updating shallow file with "fetch --update-shallow" (or > failing to do so), we reset the in-core data. > > > +void reset_repository_shallow(struct repository *r) > > +{ > > + r->parsed_objects->is_shallow = -1; > > + stat_validity_clear(r->parsed_objects->shallow_stat); > > +} > > OK. > > > @@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock, > > * shallow file". > > */ > > *alternate_shallow_file = ""; > > + reset_repository_shallow(the_repository); > > strbuf_release(&sb); > > } > > And also after writing out the alternate shallow file (whether it is > empty or polulated). > > > @@ -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); > > } > > Here, we reset only after we realize we cannot write the updated > shallow file. Intended? Yes, see this earlier discussion I had about it with Jonathan: https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/. > > + cat <<EOF >expect.refs && > > +refs/remotes/shallow/master > > +refs/remotes/shallow/no-shallow > > +refs/tags/heavy-tag > > +refs/tags/heavy-tag-for-graph > > +refs/tags/light-tag > > +refs/tags/light-tag-for-graph > > +EOF > > cat <<-EOF >expect.refs && > ... body can be indented by any number of TAB > ... to make it easier to view > EOF > > > + test_cmp expect.refs actual.refs && > > + git log --format=%s shallow/master >actual && > > + cat <<EOF >expect && > > Likewise. I'd be happy to update these, but I am matching the (poor) style of the surrounding tests. Are you OK with the inconsistency? Would you like another preparatory patch to clean up the surrounding? Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: >> > @@ -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); >> > } >> >> Here, we reset only after we realize we cannot write the updated >> shallow file. Intended? > > Yes, see this earlier discussion I had about it with Jonathan: > https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/. I did, and then I asked the question, because I couldn't quite get if JTan was asking a question similar to the one he asked earlier in the message ("do you need a reset in the "else" branch as well?"), or if he was saying what he sees there, "the opposite case", was good. Also, I was sort-of reacting to """In any case, I think the commit message should discuss why reset_repository_shallow() is added only on the unlink+rollback side in one "if" statement, but only on the opposite "commit" side in another "if" statement.""" in that message. Thanks.
On Tue, Apr 21, 2020 at 01:52:46PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > >> > @@ -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); > >> > } > >> > >> Here, we reset only after we realize we cannot write the updated > >> shallow file. Intended? > > > > Yes, see this earlier discussion I had about it with Jonathan: > > https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/. > > I did, and then I asked the question, because I couldn't quite get > if JTan was asking a question similar to the one he asked earlier in > the message ("do you need a reset in the "else" branch as well?"), > or if he was saying what he sees there, "the opposite case", was > good. Sorry, I think that the linked message is confusing (at least, it confused me the second time I read it because I wasn't sure which part of the mail Jonathan was asking about: my patch, or his response to my patch). I think that he was referring to how I had it in the original patch I sent in that thread, which was wrong. Based on my understanding of his message, his recommendations match what I have in _this_ patch. > Also, I was sort-of reacting to """In any case, I think the commit > message should discuss why reset_repository_shallow() is added only > on the unlink+rollback side in one "if" statement, but only on the > opposite "commit" side in another "if" statement.""" in that > message. Is the description that I attached in the earlier patch sufficient? I could certainly add more detail if it's not. In either case, I'm sitting on another patch locally to improve the style of the surrounding tests, which is done as a preparatory step before this patch. I'll re-send after hearing back from you. > Thanks. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > Sorry, I think that the linked message is confusing (at least, it > confused me the second time I read it because I wasn't sure which part > of the mail Jonathan was asking about: my patch, or his response to my > patch). > > I think that he was referring to how I had it in the original patch I > sent in that thread, which was wrong. Based on my understanding of his > message, his recommendations match what I have in _this_ patch. Thanks for a clarification. > In either case, I'm sitting on another patch locally to improve the > style of the surrounding tests, which is done as a preparatory step > before this patch. I'll re-send after hearing back from you. Alright. Thanks.
> 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 &&
> Taylor Blau <me@ttaylorr.com> writes: > > >> > @@ -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); > >> > } > >> > >> Here, we reset only after we realize we cannot write the updated > >> shallow file. Intended? > > > > Yes, see this earlier discussion I had about it with Jonathan: > > https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/. > > I did, and then I asked the question, because I couldn't quite get > if JTan was asking a question similar to the one he asked earlier in > the message ("do you need a reset in the "else" branch as well?"), > or if he was saying what he sees there, "the opposite case", was > good. Sorry for not being clear. My intention was to ask a question similar to the earlier one - in this case, and in the previous case, I think that the reset should happen no matter whether we execute the "if" case or the "else" case, so we should just put it right after the entire "if" statement.
Jonathan Tan <jonathantanmy@google.com> writes: >> @@ -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. Is there any rollback_lock_file() or commit_lock_file() call on the shallow lock file in the files involved in this patch that does not need a call to reset_repository_shallow() left after your work? What I am trying to get at is if it would be safer to have a pair of thin wrapper for rolling back or committing a new version of new shallow file, e.g. rollback_shallow_file() + commit_shallow_file(), and replace calls to {rollback,commit}_lock_file() with calls to them.
On Wed, Apr 22, 2020 at 11:15:33AM -0700, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: > > >> @@ -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. > > Is there any rollback_lock_file() or commit_lock_file() call on the > shallow lock file in the files involved in this patch that does not > need a call to reset_repository_shallow() left after your work? > > What I am trying to get at is if it would be safer to have a pair of > thin wrapper for rolling back or committing a new version of new > shallow file, e.g. rollback_shallow_file() + commit_shallow_file(), > and replace calls to {rollback,commit}_lock_file() with calls to > them. Very elegant. Thanks for an excellent suggestion. v2 incoming just as soon as 'make test' finishes... Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: >> What I am trying to get at is if it would be safer to have a pair of >> thin wrapper for rolling back or committing a new version of new >> shallow file, e.g. rollback_shallow_file() + commit_shallow_file(), >> and replace calls to {rollback,commit}_lock_file() with calls to >> them. > > Very elegant. Thanks for an excellent suggestion. v2 incoming just as > soon as 'make test' finishes... Note that I didn't verify there is a case where we want not to call reset_repository_shallow() after committing or rolling back, either for performance or correctness purposes. As long as the experts on the codepaths involved are happy with the idea, I'd be happy, too. JNieder raised the idea of using a different type to avoid calling the bare rollback/commit functions by mistake. It appears that, in addition to these two functions, setup_alternate_shallow() needs to be updated if we wanted to go that route, and it sounds like a good idea to gain safety with minimal cost. Thanks.
diff --git a/commit.h b/commit.h index 008a0fa4a0..ee1ba139d4 100644 --- a/commit.h +++ b/commit.h @@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid); int unregister_shallow(const struct object_id *oid); int for_each_commit_graft(each_commit_graft_fn, void *); int is_repository_shallow(struct repository *r); +void reset_repository_shallow(struct repository *r); struct commit_list *get_shallow_commits(struct object_array *heads, int depth, int shallow_flag, int not_shallow_flag); struct commit_list *get_shallow_commits_by_rev_list( diff --git a/fetch-pack.c b/fetch-pack.c index 1734a573b0..684868bc17 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1632,6 +1632,7 @@ static void update_shallow(struct fetch_pack_args *args, rollback_lock_file(&shallow_lock); } else commit_lock_file(&shallow_lock); + reset_repository_shallow(the_repository); alternate_shallow_file = NULL; return; } diff --git a/shallow.c b/shallow.c index 7fd04afed1..9d1304e786 100644 --- a/shallow.c +++ b/shallow.c @@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid) int is_repository_shallow(struct repository *r) { - /* - * NEEDSWORK: This function updates - * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but - * there is no corresponding function to clear them when the shallow - * file is updated. - */ - FILE *fp; char buf[1024]; const char *path = r->parsed_objects->alternate_shallow_file; @@ -79,6 +72,12 @@ int is_repository_shallow(struct repository *r) return r->parsed_objects->is_shallow; } +void reset_repository_shallow(struct repository *r) +{ + r->parsed_objects->is_shallow = -1; + stat_validity_clear(r->parsed_objects->shallow_stat); +} + /* * TODO: use "int" elemtype instead of "int *" when/if commit-slab * supports a "valid" flag. @@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock, * shallow file". */ *alternate_shallow_file = ""; + reset_repository_shallow(the_repository); strbuf_release(&sb); } @@ -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); } diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 4f681dbbe1..c9c731c7a9 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -177,6 +177,42 @@ EOF ) ' +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 && + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* && + git fsck && + git for-each-ref --sort=refname --format="%(refname)" >actual.refs && + cat <<EOF >expect.refs && +refs/remotes/shallow/master +refs/remotes/shallow/no-shallow +refs/tags/heavy-tag +refs/tags/heavy-tag-for-graph +refs/tags/light-tag +refs/tags/light-tag-for-graph +EOF + test_cmp expect.refs actual.refs && + git log --format=%s shallow/master >actual && + cat <<EOF >expect && +8 +7 +6 +5 +4 +3 +EOF + test_cmp expect actual + ) +' + test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' cp -R .git read-only.git && test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily, 2019-01-10), the author noted that 'is_repository_shallow' produces visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'. This is a problem for e.g., fetching with '--update-shallow' in a shallow repsoitory with 'fetch.writeCommitGraph' enabled, since the update to '.git/shallow' will cause Git to think that the repository isn't shallow when it is, thereby circumventing the commit-graph compatability check. This causes problems in shallow repositories with at least shallow refs that have at least one ancestor (since the client won't have those objects, and therefore can't take the reachability closure over commits when writing a commit-graph). 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. As a result of the second change, 'prune_shallow' can now only be called once (since 'check_shallow_file_for_update' will die after calling 'reset_repository_shallow'). But, this is OK since we only call 'prune_shallow' at most once per process. Helped-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Here's a cleaned up version of the patch that we were originally discussing in https://lore.kernel.org/git/20200414235057.GA6863@syl.local/, which addresses some of Jonathan's feedback and adds a test to make sure that the new behavior is working correctly. commit.h | 1 + fetch-pack.c | 1 + shallow.c | 15 ++++++++------- t/t5537-fetch-shallow.sh | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 7 deletions(-) -- 2.26.2.108.g048abe1751