| Message ID | 296e70790d7a391d471554b0bc5a58e2a091ce88.1587601501.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shallow.c: reset shallow-ness after updating | expand |
Taylor Blau wrote: > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -872,12 +872,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_file(the_repository, &shallow_lock); I like it. I wonder, is there a way we can make it more difficult to accidentally use rollback_lock_file where rollback_shallow_file is needed? For example, what if shallow_lock has a different type "struct shallow_lock" so one would have to reach in to its lock_file member to bypass the shallow_file interface? [...] > oid_array_clear(&extra); > return -1; > } > > - commit_lock_file(&shallow_lock); > + commit_shallow_file(the_repository, &shallow_lock); > > /* > * Make sure setup_alternate_shallow() for the next ref does > diff --git a/commit.h b/commit.h > index 008a0fa4a0..ab91d21131 100644 > --- a/commit.h > +++ b/commit.h > @@ -249,6 +249,8 @@ struct oid_array; > struct ref; > int register_shallow(struct repository *r, const struct object_id *oid); > int unregister_shallow(const struct object_id *oid); > +int commit_shallow_file(struct repository *r, struct lock_file *lk); > +void rollback_shallow_file(struct repository *r, struct lock_file *lk); optional: might make sense to put this near setup_alternate_shallow for discoverability Could this have an API doc comment? [...] > --- 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) Not about this patch: it might make sense to split out a shallow.h header / API. Thanks and hope that helps, Jonathan
> Replace each instance of 'commit_lock_file' and 'rollback_lock_file' > with 'commit_shallow_file' and 'rollback_shallow_file' when the lock > being held is over the '.git/shallow' file. I think Jonathan Nieder already covered 1/2 so I'll just close the loop on this patch. There was one potential issue in that a previous version of this patch also called reset_repository_shallow() in setup_alternate_shallow(), but this version does not. But after looking into it, this looks fine - setup_alternate_shallow() deals with a passed-in alternate_shallow_file variable, which is different from the r->parsed_objects->alternate_shallow_file that is_repository_shallow() uses to set the global variables. (I might have confused the two during earlier reviews.) Also, setup_alternate_shallow() is called either before any shallow processing (empirically demonstrating that no resetting is needed in this case, because it has been working), or right before a commit or rollback of the lock file (so the global variables are being reset anyway, so we do not need to call reset_repository_shallow()). So, Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes: >> Replace each instance of 'commit_lock_file' and 'rollback_lock_file' >> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock >> being held is over the '.git/shallow' file. > > I think Jonathan Nieder already covered 1/2 so I'll just close the loop > on this patch. There was one potential issue in that a previous version > of this patch also called reset_repository_shallow() in > setup_alternate_shallow(), but this version does not. But after looking > into it, this looks fine - setup_alternate_shallow() deals with a > passed-in alternate_shallow_file variable, which is different from the > r->parsed_objects->alternate_shallow_file that is_repository_shallow() > uses to set the global variables. (I might have confused the two during > earlier reviews.) Also, setup_alternate_shallow() is called either > before any shallow processing (empirically demonstrating that no > resetting is needed in this case, because it has been working), or right > before a commit or rollback of the lock file (so the global variables > are being reset anyway, so we do not need to call > reset_repository_shallow()). > > So, > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Thanks for a review. And of course, thanks Taylor. Will queue.
On Thu, Apr 23, 2020 at 01:40:47PM -0700, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: > > >> Replace each instance of 'commit_lock_file' and 'rollback_lock_file' > >> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock > >> being held is over the '.git/shallow' file. > > > > I think Jonathan Nieder already covered 1/2 so I'll just close the loop > > on this patch. There was one potential issue in that a previous version > > of this patch also called reset_repository_shallow() in > > setup_alternate_shallow(), but this version does not. But after looking > > into it, this looks fine - setup_alternate_shallow() deals with a > > passed-in alternate_shallow_file variable, which is different from the > > r->parsed_objects->alternate_shallow_file that is_repository_shallow() > > uses to set the global variables. (I might have confused the two during > > earlier reviews.) Also, setup_alternate_shallow() is called either > > before any shallow processing (empirically demonstrating that no > > resetting is needed in this case, because it has been working), or right > > before a commit or rollback of the lock file (so the global variables > > are being reset anyway, so we do not need to call > > reset_repository_shallow()). > > > > So, > > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > > Thanks for a review. > > And of course, thanks Taylor. Will queue. Thanks, both. I'll send some more patches on top to introduce a 'shallow_lock' type. Thanks, Taylor
Hi, Taylor Blau wrote: > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > builtin/receive-pack.c | 4 ++-- > commit.h | 2 ++ > fetch-pack.c | 10 +++++----- > shallow.c | 30 +++++++++++++++++++++--------- > t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++ > 5 files changed, 59 insertions(+), 16 deletions(-) I haven't investigated the cause yet, but I've run into an interesting bug that bisects to this commit. Jay Conrod (cc-ed) reports: | I believe this is also the cause of Go toolchain test failures we've | been seeing. Go uses git to fetch dependencies. | | The problem we're seeing can be reproduced with the script below. It | should print "success". Instead, the git merge-base command fails | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c | (origin/master) has no history. -- 8< -- #!/bin/bash set -euxo pipefail if [ -d legacytest ]; then echo "legacytest directory already exists" >&2 exit 1 fi mkdir legacytest cd legacytest git init --bare git config protocol.version 2 git config fetch.writeCommitGraph true git remote add origin -- https://github.com/rsc/legacytest git fetch -f --depth=1 origin refs/heads/master:refs/heads/master git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*' git fetch --unshallow -f origin git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c echo success -- >8 -- The fetch.writeCommitGraph part is interesting. When does a commit graph file get written in this sequence of operations? In an unshallow operation, does the usual guard against writing a commit graph in a shallow repo get missed? "rm -fr objects/info/commit-graphs" recovers the full history in the repo, so this is not a case of writing the wrong shallows --- it's only a commit graph issue. I'll take a closer look, but thought I'd give others a chance to look to in case there's something obvious. :) Thanks, Jonathan
Hi Jonathan, On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote: > Hi, > > Taylor Blau wrote: > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > builtin/receive-pack.c | 4 ++-- > > commit.h | 2 ++ > > fetch-pack.c | 10 +++++----- > > shallow.c | 30 +++++++++++++++++++++--------- > > t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++ > > 5 files changed, 59 insertions(+), 16 deletions(-) > > I haven't investigated the cause yet, but I've run into an interesting > bug that bisects to this commit. Jay Conrod (cc-ed) reports: > > | I believe this is also the cause of Go toolchain test failures we've > | been seeing. Go uses git to fetch dependencies. > | > | The problem we're seeing can be reproduced with the script below. It > | should print "success". Instead, the git merge-base command fails > | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c > | (origin/master) has no history. > > -- 8< -- > #!/bin/bash > > set -euxo pipefail > if [ -d legacytest ]; then > echo "legacytest directory already exists" >&2 > exit 1 > fi > mkdir legacytest > cd legacytest > git init --bare > git config protocol.version 2 > git config fetch.writeCommitGraph true > git remote add origin -- https://github.com/rsc/legacytest > git fetch -f --depth=1 origin refs/heads/master:refs/heads/master > git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*' > git fetch --unshallow -f origin > git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c > echo success > -- >8 -- Thanks to you and Jay for the report and reproduction script. Indeed, I can reproduce this on the tip of master (which is equivalent to v2.27.0 at the time of writing). > The fetch.writeCommitGraph part is interesting. When does a commit > graph file get written in this sequence of operations? In an > unshallow operation, does the usual guard against writing a commit > graph in a shallow repo get missed? The last 'git fetch' is the one that writes the commit-graph. You can verify this by sticking a 'ls objects/info' after each 'git' invocation in your script. Here's where things get weird, though. Prior to this patch, Git would pick up that the repository is shallow before unshallowing, but never invalidate this fact after unshallowing. That means that once we got to 'write_commit_graph', we'd exit immediately since it appears as if the repository is shallow. In this patch, we don't do that anymore, since we rightly unset the fact that we are (were) shallow. In a debugger, I ran your script and a 'git commit-graph write --split --reachable' side-by-side, and found an interesting discrepancy: some commits (loaded from 'copy_oids_to_commits') *don't* have their parents set when invoked from 'git fetch', but *do* when invoked as 'git commit-graph write ...'. I'm not an expert in the object cache, but my hunch is that when we fetch these objects they're marked as parsed without having loaded their parents. When we load them again via 'lookup_object', we get objects that look parsed, but don't have parents where they otherwise should. I'm going to CC Stolee to see if he has any thoughts on how to handle this and/or if my idea is on the right track. > "rm -fr objects/info/commit-graphs" recovers the full history in the > repo, so this is not a case of writing the wrong shallows --- it's > only a commit graph issue. > > I'll take a closer look, but thought I'd give others a chance to look > to in case there's something obvious. :) > > Thanks, > Jonathan Thanks, Taylor
On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote: > Hi Jonathan, > > On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote: > > Hi, > > > > Taylor Blau wrote: > > > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > > --- > > > builtin/receive-pack.c | 4 ++-- > > > commit.h | 2 ++ > > > fetch-pack.c | 10 +++++----- > > > shallow.c | 30 +++++++++++++++++++++--------- > > > t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++ > > > 5 files changed, 59 insertions(+), 16 deletions(-) > > > > I haven't investigated the cause yet, but I've run into an interesting > > bug that bisects to this commit. Jay Conrod (cc-ed) reports: > > > > | I believe this is also the cause of Go toolchain test failures we've > > | been seeing. Go uses git to fetch dependencies. > > | > > | The problem we're seeing can be reproduced with the script below. It > > | should print "success". Instead, the git merge-base command fails > > | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c > > | (origin/master) has no history. > > > > -- 8< -- > > #!/bin/bash > > > > set -euxo pipefail > > if [ -d legacytest ]; then > > echo "legacytest directory already exists" >&2 > > exit 1 > > fi > > mkdir legacytest > > cd legacytest > > git init --bare > > git config protocol.version 2 > > git config fetch.writeCommitGraph true > > git remote add origin -- https://github.com/rsc/legacytest > > git fetch -f --depth=1 origin refs/heads/master:refs/heads/master > > git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*' > > git fetch --unshallow -f origin > > git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c > > echo success > > -- >8 -- > > Thanks to you and Jay for the report and reproduction script. Indeed, I > can reproduce this on the tip of master (which is equivalent to v2.27.0 > at the time of writing). > > > The fetch.writeCommitGraph part is interesting. When does a commit > > graph file get written in this sequence of operations? In an > > unshallow operation, does the usual guard against writing a commit > > graph in a shallow repo get missed? > > The last 'git fetch' is the one that writes the commit-graph. You can > verify this by sticking a 'ls objects/info' after each 'git' invocation > in your script. > > Here's where things get weird, though. Prior to this patch, Git would > pick up that the repository is shallow before unshallowing, but never > invalidate this fact after unshallowing. That means that once we got to > 'write_commit_graph', we'd exit immediately since it appears as if the > repository is shallow. > > In this patch, we don't do that anymore, since we rightly unset the fact > that we are (were) shallow. > > In a debugger, I ran your script and a 'git commit-graph write --split > --reachable' side-by-side, and found an interesting discrepancy: some > commits (loaded from 'copy_oids_to_commits') *don't* have their parents > set when invoked from 'git fetch', but *do* when invoked as 'git > commit-graph write ...'. > > I'm not an expert in the object cache, but my hunch is that when we > fetch these objects they're marked as parsed without having loaded their > parents. When we load them again via 'lookup_object', we get objects > that look parsed, but don't have parents where they otherwise should. Ah, this only sort of has to do with the object cache. In 'parse_commit_buffer()' we stop parsing parents in the case that the repository is shallow (this goes back to 7f3140cd23 (git repack: keep commits hidden by a graft, 2009-07-23)). That makes me somewhat nervous. We're going to keep any objects opened prior to unshallowing in the cache, along with their hidden parents. I suspect that this is why Git has kept the shallow bit as sticky for so long. I'm not quite sure what to do here. I think that any of the following would work: * Keep the shallow bit sticky, at least for fetch.writeCommitGraph (i.e., pretend as if fetch.writecommitgraph=0 in the case of '--unshallow'). * Dump the object cache upon un-shallowing, forcing us to re-discover the parents when they are no longer hidden behind a graft. The latter seems like the most complete feasible fix. The former should work fine to address this case, but I wonder if there are other call-sites that are affected by this behavior. My hunch is that this is a unique case, since it requires going from shallow to unshallow in the same process. I have yet to create a smaller test case, but the following should be sufficient to dump the cache of parsed objects upon shallowing or un-shallowing: diff --git a/shallow.c b/shallow.c index b826de9b67..06db857f53 100644 --- a/shallow.c +++ b/shallow.c @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r) { r->parsed_objects->is_shallow = -1; stat_validity_clear(r->parsed_objects->shallow_stat); + + parsed_object_pool_clear(r->parsed_objects); + r->parsed_objects = parsed_object_pool_new(); } int commit_shallow_file(struct repository *r, struct shallow_lock *lk) Is this something we want to go forward with? Are there some far-reaching implications that I'm missing? > I'm going to CC Stolee to see if he has any thoughts on how to handle > this and/or if my idea is on the right track. > > > "rm -fr objects/info/commit-graphs" recovers the full history in the > > repo, so this is not a case of writing the wrong shallows --- it's > > only a commit graph issue. > > > > I'll take a closer look, but thought I'd give others a chance to look > > to in case there's something obvious. :) > > > > Thanks, > > Jonathan > > Thanks, > Taylor Thanks, Taylor
On 6/3/2020 1:16 AM, Taylor Blau wrote: > On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote: >> Hi Jonathan, >> >> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote: >>> Hi, >>> >>> Taylor Blau wrote: >>> >>>> Signed-off-by: Taylor Blau <me@ttaylorr.com> >>>> --- >>>> builtin/receive-pack.c | 4 ++-- >>>> commit.h | 2 ++ >>>> fetch-pack.c | 10 +++++----- >>>> shallow.c | 30 +++++++++++++++++++++--------- >>>> t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++ >>>> 5 files changed, 59 insertions(+), 16 deletions(-) >>> >>> I haven't investigated the cause yet, but I've run into an interesting >>> bug that bisects to this commit. Jay Conrod (cc-ed) reports: >>> >>> | I believe this is also the cause of Go toolchain test failures we've >>> | been seeing. Go uses git to fetch dependencies. >>> | >>> | The problem we're seeing can be reproduced with the script below. It >>> | should print "success". Instead, the git merge-base command fails >>> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c >>> | (origin/master) has no history. >>> >>> -- 8< -- >>> #!/bin/bash >>> >>> set -euxo pipefail >>> if [ -d legacytest ]; then >>> echo "legacytest directory already exists" >&2 >>> exit 1 >>> fi >>> mkdir legacytest >>> cd legacytest >>> git init --bare >>> git config protocol.version 2 >>> git config fetch.writeCommitGraph true >>> git remote add origin -- https://github.com/rsc/legacytest >>> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master >>> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*' >>> git fetch --unshallow -f origin >>> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c >>> echo success >>> -- >8 -- >> >> Thanks to you and Jay for the report and reproduction script. Indeed, I >> can reproduce this on the tip of master (which is equivalent to v2.27.0 >> at the time of writing). >> >>> The fetch.writeCommitGraph part is interesting. When does a commit >>> graph file get written in this sequence of operations? In an >>> unshallow operation, does the usual guard against writing a commit >>> graph in a shallow repo get missed? >> >> The last 'git fetch' is the one that writes the commit-graph. You can >> verify this by sticking a 'ls objects/info' after each 'git' invocation >> in your script. >> >> Here's where things get weird, though. Prior to this patch, Git would >> pick up that the repository is shallow before unshallowing, but never >> invalidate this fact after unshallowing. That means that once we got to >> 'write_commit_graph', we'd exit immediately since it appears as if the >> repository is shallow. >> >> In this patch, we don't do that anymore, since we rightly unset the fact >> that we are (were) shallow. >> >> In a debugger, I ran your script and a 'git commit-graph write --split >> --reachable' side-by-side, and found an interesting discrepancy: some >> commits (loaded from 'copy_oids_to_commits') *don't* have their parents >> set when invoked from 'git fetch', but *do* when invoked as 'git >> commit-graph write ...'. >> >> I'm not an expert in the object cache, but my hunch is that when we >> fetch these objects they're marked as parsed without having loaded their >> parents. When we load them again via 'lookup_object', we get objects >> that look parsed, but don't have parents where they otherwise should. > > Ah, this only sort of has to do with the object cache. In > 'parse_commit_buffer()' we stop parsing parents in the case that the > repository is shallow (this goes back to 7f3140cd23 (git repack: keep > commits hidden by a graft, 2009-07-23)). > > That makes me somewhat nervous. We're going to keep any objects opened > prior to unshallowing in the cache, along with their hidden parents. I > suspect that this is why Git has kept the shallow bit as sticky for so > long. > > I'm not quite sure what to do here. I think that any of the following > would work: > > * Keep the shallow bit sticky, at least for fetch.writeCommitGraph > (i.e., pretend as if fetch.writecommitgraph=0 in the case of > '--unshallow'). I'm in favor of this option, if possible. Anything that alters the commit history in-memory at any point in the Git process is unsafe to combine with a commit-graph read _or_ write. I'm sorry that the guards in commit_graph_compatible() are not enough here. > * Dump the object cache upon un-shallowing, forcing us to re-discover > the parents when they are no longer hidden behind a graft. > > The latter seems like the most complete feasible fix. The former should > work fine to address this case, but I wonder if there are other > call-sites that are affected by this behavior. My hunch is that this is > a unique case, since it requires going from shallow to unshallow in the > same process. The latter would solve issues that could arise outside of the commit-graph space. But it also presents an opportunity for another gap if someone edits the shallow logic without putting in the proper guards. To be extra safe, I'd be in favor of adding an "if (grafts_ever_existed)" condition in commit_graph_compatible() based on a global that is assigned a non-zero value whenever grafts are loaded at any point in the process, mostly because it would be easy to guarantee that it is safe. It could even be localized to the repository struct. > I have yet to create a smaller test case, but the following should be > sufficient to dump the cache of parsed objects upon shallowing or > un-shallowing: > > diff --git a/shallow.c b/shallow.c > index b826de9b67..06db857f53 100644 > --- a/shallow.c > +++ b/shallow.c > @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r) > { > r->parsed_objects->is_shallow = -1; > stat_validity_clear(r->parsed_objects->shallow_stat); > + > + parsed_object_pool_clear(r->parsed_objects); > + r->parsed_objects = parsed_object_pool_new(); > } > > int commit_shallow_file(struct repository *r, struct shallow_lock *lk) > > Is this something we want to go forward with? Are there some > far-reaching implications that I'm missing? I'd like to see the extra-careful check, in addition to this one. This is such a rarely-used and narrowly-tested case that we need to be really really careful to avoid regressions. Thanks, -Stolee
Hi Stolee, On Wed, Jun 03, 2020 at 09:08:26AM -0400, Derrick Stolee wrote: > On 6/3/2020 1:16 AM, Taylor Blau wrote: > > On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote: > >> Hi Jonathan, > >> > >> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote: > >>> Hi, > >>> > >>> Taylor Blau wrote: > >>> > >>>> Signed-off-by: Taylor Blau <me@ttaylorr.com> > >>>> --- > >>>> builtin/receive-pack.c | 4 ++-- > >>>> commit.h | 2 ++ > >>>> fetch-pack.c | 10 +++++----- > >>>> shallow.c | 30 +++++++++++++++++++++--------- > >>>> t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++ > >>>> 5 files changed, 59 insertions(+), 16 deletions(-) > >>> > >>> I haven't investigated the cause yet, but I've run into an interesting > >>> bug that bisects to this commit. Jay Conrod (cc-ed) reports: > >>> > >>> | I believe this is also the cause of Go toolchain test failures we've > >>> | been seeing. Go uses git to fetch dependencies. > >>> | > >>> | The problem we're seeing can be reproduced with the script below. It > >>> | should print "success". Instead, the git merge-base command fails > >>> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c > >>> | (origin/master) has no history. > >>> > >>> -- 8< -- > >>> #!/bin/bash > >>> > >>> set -euxo pipefail > >>> if [ -d legacytest ]; then > >>> echo "legacytest directory already exists" >&2 > >>> exit 1 > >>> fi > >>> mkdir legacytest > >>> cd legacytest > >>> git init --bare > >>> git config protocol.version 2 > >>> git config fetch.writeCommitGraph true > >>> git remote add origin -- https://github.com/rsc/legacytest > >>> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master > >>> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*' > >>> git fetch --unshallow -f origin > >>> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c > >>> echo success > >>> -- >8 -- > >> > >> Thanks to you and Jay for the report and reproduction script. Indeed, I > >> can reproduce this on the tip of master (which is equivalent to v2.27.0 > >> at the time of writing). > >> > >>> The fetch.writeCommitGraph part is interesting. When does a commit > >>> graph file get written in this sequence of operations? In an > >>> unshallow operation, does the usual guard against writing a commit > >>> graph in a shallow repo get missed? > >> > >> The last 'git fetch' is the one that writes the commit-graph. You can > >> verify this by sticking a 'ls objects/info' after each 'git' invocation > >> in your script. > >> > >> Here's where things get weird, though. Prior to this patch, Git would > >> pick up that the repository is shallow before unshallowing, but never > >> invalidate this fact after unshallowing. That means that once we got to > >> 'write_commit_graph', we'd exit immediately since it appears as if the > >> repository is shallow. > >> > >> In this patch, we don't do that anymore, since we rightly unset the fact > >> that we are (were) shallow. > >> > >> In a debugger, I ran your script and a 'git commit-graph write --split > >> --reachable' side-by-side, and found an interesting discrepancy: some > >> commits (loaded from 'copy_oids_to_commits') *don't* have their parents > >> set when invoked from 'git fetch', but *do* when invoked as 'git > >> commit-graph write ...'. > >> > >> I'm not an expert in the object cache, but my hunch is that when we > >> fetch these objects they're marked as parsed without having loaded their > >> parents. When we load them again via 'lookup_object', we get objects > >> that look parsed, but don't have parents where they otherwise should. > > > > Ah, this only sort of has to do with the object cache. In > > 'parse_commit_buffer()' we stop parsing parents in the case that the > > repository is shallow (this goes back to 7f3140cd23 (git repack: keep > > commits hidden by a graft, 2009-07-23)). > > > > That makes me somewhat nervous. We're going to keep any objects opened > > prior to unshallowing in the cache, along with their hidden parents. I > > suspect that this is why Git has kept the shallow bit as sticky for so > > long. > > > > I'm not quite sure what to do here. I think that any of the following > > would work: > > > > * Keep the shallow bit sticky, at least for fetch.writeCommitGraph > > (i.e., pretend as if fetch.writecommitgraph=0 in the case of > > '--unshallow'). > > I'm in favor of this option, if possible. Anything that alters the > commit history in-memory at any point in the Git process is unsafe to > combine with a commit-graph read _or_ write. I'm sorry that the guards > in commit_graph_compatible() are not enough here. > > > * Dump the object cache upon un-shallowing, forcing us to re-discover > > the parents when they are no longer hidden behind a graft. > > > > The latter seems like the most complete feasible fix. The former should > > work fine to address this case, but I wonder if there are other > > call-sites that are affected by this behavior. My hunch is that this is > > a unique case, since it requires going from shallow to unshallow in the > > same process. > > The latter would solve issues that could arise outside of the commit-graph > space. But it also presents an opportunity for another gap if someone edits > the shallow logic without putting in the proper guards. > > To be extra safe, I'd be in favor of adding an "if (grafts_ever_existed)" > condition in commit_graph_compatible() based on a global that is assigned > a non-zero value whenever grafts are loaded at any point in the process, > mostly because it would be easy to guarantee that it is safe. It could > even be localized to the repository struct. > > > I have yet to create a smaller test case, but the following should be > > sufficient to dump the cache of parsed objects upon shallowing or > > un-shallowing: > > > > diff --git a/shallow.c b/shallow.c > > index b826de9b67..06db857f53 100644 > > --- a/shallow.c > > +++ b/shallow.c > > @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r) > > { > > r->parsed_objects->is_shallow = -1; > > stat_validity_clear(r->parsed_objects->shallow_stat); > > + > > + parsed_object_pool_clear(r->parsed_objects); > > + r->parsed_objects = parsed_object_pool_new(); > > } > > > > int commit_shallow_file(struct repository *r, struct shallow_lock *lk) > > > > Is this something we want to go forward with? Are there some > > far-reaching implications that I'm missing? > > I'd like to see the extra-careful check, in addition to this one. This > is such a rarely-used and narrowly-tested case that we need to be really > really careful to avoid regressions. I'm a little confused at which suggestion you're in favor of ;-). For clarity, are you suggesting that we add a new 'r->grafts_ever_existed' bit in addition to doing a hard reset of the object pool? > Thanks, > -Stolee Thanks, Taylor
Taylor Blau wrote: > Ah, this only sort of has to do with the object cache. In > 'parse_commit_buffer()' we stop parsing parents in the case that the > repository is shallow (this goes back to 7f3140cd23 (git repack: keep > commits hidden by a graft, 2009-07-23)). Ah, good analysis. (In fact, the behavior is older: it's from 5da5c8f4cf4 (Teach parse_commit_buffer about grafting., 2005-07-30).) So this is additional "cached" data that needs to be invalidated by reset_repository_shallow. So the question is, what other information falls into that category? [...] > --- a/shallow.c > +++ b/shallow.c > @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r) > { > r->parsed_objects->is_shallow = -1; > stat_validity_clear(r->parsed_objects->shallow_stat); > + (nit: the above two lines wouldn't be needed if r->parsed_objects is being thrown away.) > + parsed_object_pool_clear(r->parsed_objects); > + r->parsed_objects = parsed_object_pool_new(); > } Shallows don't affect the ref store. They only affect object walks. So r->parsed_objects does seem like the only place that could be affected. That said, with this change I'd worry about use-after-free from any existing references to objects in the pool. Stepping back, what I think I would like to see is to *not* have grafts and shallow state affect the in-memory persisted parsed objects. Instead, act as an overlay in accessors that traverse over them. Lacking that, I like the idea of a "dirty bit" that gets written as soon as we have started lying in the parsed object pool. Something like this. What do you think? diff --git i/commit-graph.c w/commit-graph.c index 2ff042fbf4f..84b49ce903b 100644 --- i/commit-graph.c +++ w/commit-graph.c @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r) } prepare_commit_graft(r); - if (r->parsed_objects && r->parsed_objects->grafts_nr) + if (r->parsed_objects && + (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) return 0; if (is_repository_shallow(r)) return 0; diff --git i/commit.c w/commit.c index 87686a7055b..762f09e53ae 100644 --- i/commit.c +++ w/commit.c @@ -423,6 +423,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b pptr = &item->parents; graft = lookup_commit_graft(r, &item->object.oid); + if (graft) + r->parsed_objects->substituted_parent = 1; while (bufptr + parent_entry_len < tail && !memcmp(bufptr, "parent ", 7)) { struct commit *new_parent; @@ -447,6 +449,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b if (graft) { int i; struct commit *new_parent; + for (i = 0; i < graft->nr_parent; i++) { new_parent = lookup_commit(r, &graft->parent[i]); diff --git i/object.h w/object.h index b22328b8383..db02fdcd6b2 100644 --- i/object.h +++ w/object.h @@ -26,6 +26,7 @@ struct parsed_object_pool { char *alternate_shallow_file; int commit_graft_prepared; + int substituted_parent; struct buffer_slab *buffer_slab; }; Thanks, Jonathan
Hi, Derrick Stolee wrote: > On 6/3/2020 1:16 AM, Taylor Blau wrote: >> * Keep the shallow bit sticky, at least for fetch.writeCommitGraph >> (i.e., pretend as if fetch.writecommitgraph=0 in the case of >> '--unshallow'). > > I'm in favor of this option, if possible. Anything that alters the > commit history in-memory at any point in the Git process is unsafe to > combine with a commit-graph read _or_ write. I'm sorry that the guards > in commit_graph_compatible() are not enough here. As described in [1], I agree --- this kind of "dirty bit" is a good option and seems like the right thing to do here. And I'm glad you said read _or_ write here. I hadn't realized that this check already applies for reads, and I'm very happy it does. [...] >> * Dump the object cache upon un-shallowing, forcing us to re-discover >> the parents when they are no longer hidden behind a graft. >> >> The latter seems like the most complete feasible fix. The former should >> work fine to address this case, but I wonder if there are other >> call-sites that are affected by this behavior. My hunch is that this is >> a unique case, since it requires going from shallow to unshallow in the >> same process. > > The latter would solve issues that could arise outside of the commit-graph > space. But it also presents an opportunity for another gap if someone edits > the shallow logic without putting in the proper guards. This, however, I don't agree with. I'm a strong believer in having clear invariants --- without them, code can only be understood empirically, and https://ieeexplore.ieee.org/document/9068304 describes how painful of a world that can be. So because shallow is specifically about changing the set of parents in objects used for traversal, I want to make sure that we as reviewers will push back on any potential other new meaning of shallow. *If* we had a safe way to invalidate the object cache, it would be sufficient and would be the right thing to do. As described in [1], we unfortunately don't have such a thing. Okay, that's all an aside. Now for the actual reason I'm replying: I had been wondering why this wasn't caught at read time, but of course --unshallow clears away the shallows so there was no way for that to happen. So what I wonder is, why isn't this caught by fsck? Can "git fsck" run "git commit-graph verify" automatically and include its result as part of what it reports? Thanks, Jonathan [1] https://lore.kernel.org/git/20200603205151.GC253041@google.com/
On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote: > Taylor Blau wrote: > > > Ah, this only sort of has to do with the object cache. In > > 'parse_commit_buffer()' we stop parsing parents in the case that the > > repository is shallow (this goes back to 7f3140cd23 (git repack: keep > > commits hidden by a graft, 2009-07-23)). > > Ah, good analysis. (In fact, the behavior is older: it's from > 5da5c8f4cf4 (Teach parse_commit_buffer about grafting., 2005-07-30).) > So this is additional "cached" data that needs to be invalidated by > reset_repository_shallow. > > So the question is, what other information falls into that category? > > [...] > > --- a/shallow.c > > +++ b/shallow.c > > @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r) > > { > > r->parsed_objects->is_shallow = -1; > > stat_validity_clear(r->parsed_objects->shallow_stat); > > + > > (nit: the above two lines wouldn't be needed if r->parsed_objects is > being thrown away.) Right, thanks. I don't think that it matters since you point out a legitimate issue with dangling references, but serves me right for working on this so late at night ;-). > > + parsed_object_pool_clear(r->parsed_objects); > > + r->parsed_objects = parsed_object_pool_new(); > > } > > Shallows don't affect the ref store. They only affect object walks. > So r->parsed_objects does seem like the only place that could be > affected. > > That said, with this change I'd worry about use-after-free from any > existing references to objects in the pool. > > Stepping back, what I think I would like to see is to *not* have > grafts and shallow state affect the in-memory persisted parsed > objects. Instead, act as an overlay in accessors that traverse over > them. > > Lacking that, I like the idea of a "dirty bit" that gets written as > soon as we have started lying in the parsed object pool. Something > like this. What do you think? > > diff --git i/commit-graph.c w/commit-graph.c > index 2ff042fbf4f..84b49ce903b 100644 > --- i/commit-graph.c > +++ w/commit-graph.c > @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r) > } > > prepare_commit_graft(r); > - if (r->parsed_objects && r->parsed_objects->grafts_nr) > + if (r->parsed_objects && > + (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) This is a little tricky. Why would we set substituted_parent without also incrementing grafts_nr? That seems like the real bug here: if we incremented grafts_nr, then we would return a non-zero value from 'commit_graph_compatible' and rightly stop even without this sticky-bit. I don't quite understand this myself. If it's an oversight, it's a remarkably long-lived one. Do you have a better sense of this? > return 0; > if (is_repository_shallow(r)) > return 0; > diff --git i/commit.c w/commit.c > index 87686a7055b..762f09e53ae 100644 > --- i/commit.c > +++ w/commit.c > @@ -423,6 +423,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b > pptr = &item->parents; > > graft = lookup_commit_graft(r, &item->object.oid); > + if (graft) > + r->parsed_objects->substituted_parent = 1; > while (bufptr + parent_entry_len < tail && !memcmp(bufptr, "parent ", 7)) { > struct commit *new_parent; > > @@ -447,6 +449,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b > if (graft) { > int i; > struct commit *new_parent; > + Nit: unnecessary whitespace change, but I doubt it really matters much. > for (i = 0; i < graft->nr_parent; i++) { > new_parent = lookup_commit(r, > &graft->parent[i]); > diff --git i/object.h w/object.h > index b22328b8383..db02fdcd6b2 100644 > --- i/object.h > +++ w/object.h > @@ -26,6 +26,7 @@ struct parsed_object_pool { > char *alternate_shallow_file; > > int commit_graft_prepared; > + int substituted_parent; > > struct buffer_slab *buffer_slab; > }; > > Thanks, > Jonathan Thanks, Taylor
Taylor Blau wrote: > On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote: >> --- i/commit-graph.c >> +++ w/commit-graph.c >> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r) >> } >> >> prepare_commit_graft(r); >> - if (r->parsed_objects && r->parsed_objects->grafts_nr) >> + if (r->parsed_objects && >> + (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) > > This is a little tricky. Why would we set substituted_parent without > also incrementing grafts_nr? That seems like the real bug here: if we > incremented grafts_nr, then we would return a non-zero value from > 'commit_graph_compatible' and rightly stop even without this sticky-bit. > > I don't quite understand this myself. If it's an oversight, it's a > remarkably long-lived one. Do you have a better sense of this? The idea here is to check for two different things: 1. Do we have grafts (either from a grafts file or from a shallow file)? If so, this repository is not commit graph compatible because we could encounter one of them. 2. Have cached parsed objects taken any history modifications into account? If so, this in-memory state is not commit graph compatible because we could encounter one of them. The check (1) might seem sufficient if the set of grafts is constant for the lifetime of a git process. But since 37b9dcabfc (shallow.c: use '{commit,rollback}_shallow_file', 2020-04-22), it is not constant for the process lifetime, so we need the check (2) as well. We might want a similar check for replace refs as well some day, but not today (there is not a way to remove entries from replace_map during the life of a process). I can try sending a proper patch with commit message and tests tomorrow morning (or if someone else takes care of it, that's fine, too). Thanks, both, for your help --- it was nice seeing a clear explanation of the cause already figured out and explained when I woke up. Regards, Jonathan
Hi Jonathan, On Wed, Jun 03, 2020 at 04:06:11PM -0700, Jonathan Nieder wrote: > Taylor Blau wrote: > > On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote: > > >> --- i/commit-graph.c > >> +++ w/commit-graph.c > >> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r) > >> } > >> > >> prepare_commit_graft(r); > >> - if (r->parsed_objects && r->parsed_objects->grafts_nr) > >> + if (r->parsed_objects && > >> + (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) > > > > This is a little tricky. Why would we set substituted_parent without > > also incrementing grafts_nr? That seems like the real bug here: if we > > incremented grafts_nr, then we would return a non-zero value from > > 'commit_graph_compatible' and rightly stop even without this sticky-bit. > > > > I don't quite understand this myself. If it's an oversight, it's a > > remarkably long-lived one. Do you have a better sense of this? > > The idea here is to check for two different things: > > 1. Do we have grafts (either from a grafts file or from a shallow > file)? If so, this repository is not commit graph compatible > because we could encounter one of them. > > 2. Have cached parsed objects taken any history modifications into > account? If so, this in-memory state is not commit graph > compatible because we could encounter one of them. Ah, breaking it up like this makes it clearer. The gist is that it is possible to generate a situation which has (1) no grafts explicitly, but (2) *does* have history modifications. > The check (1) might seem sufficient if the set of grafts is constant > for the lifetime of a git process. But since 37b9dcabfc (shallow.c: > use '{commit,rollback}_shallow_file', 2020-04-22), it is not constant > for the process lifetime, so we need the check (2) as well. > > We might want a similar check for replace refs as well some day, but > not today (there is not a way to remove entries from replace_map > during the life of a process). Right. > I can try sending a proper patch with commit message and tests > tomorrow morning (or if someone else takes care of it, that's fine, > too). Thanks, both, for your help --- it was nice seeing a clear > explanation of the cause already figured out and explained when I woke > up. If you are interested, by all means -- I'd certainly be appreciative. I'm on vacation next week, and so it may be better if you are able to shepherd the patches through. Otherwise, I'd be happy to do it the week after when I get back. > Regards, > Jonathan Thanks, Taylor
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 2cc18bbffd..652661fa99 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -872,12 +872,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_file(the_repository, &shallow_lock); oid_array_clear(&extra); return -1; } - commit_lock_file(&shallow_lock); + commit_shallow_file(the_repository, &shallow_lock); /* * Make sure setup_alternate_shallow() for the next ref does diff --git a/commit.h b/commit.h index 008a0fa4a0..ab91d21131 100644 --- a/commit.h +++ b/commit.h @@ -249,6 +249,8 @@ struct oid_array; struct ref; int register_shallow(struct repository *r, const struct object_id *oid); int unregister_shallow(const struct object_id *oid); +int commit_shallow_file(struct repository *r, struct lock_file *lk); +void rollback_shallow_file(struct repository *r, struct lock_file *lk); int for_each_commit_graft(each_commit_graft_fn, void *); int is_repository_shallow(struct repository *r); struct commit_list *get_shallow_commits(struct object_array *heads, diff --git a/fetch-pack.c b/fetch-pack.c index 1734a573b0..a618f3b029 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1629,9 +1629,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_file(the_repository, &shallow_lock); } else - commit_lock_file(&shallow_lock); + commit_shallow_file(the_repository, &shallow_lock); alternate_shallow_file = NULL; return; } @@ -1655,7 +1655,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_file(the_repository, &shallow_lock); alternate_shallow_file = NULL; } oid_array_clear(&extra); @@ -1693,7 +1693,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_file(the_repository, &shallow_lock); oid_array_clear(&extra); oid_array_clear(&ref); alternate_shallow_file = NULL; @@ -1785,7 +1785,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_file(the_repository, &shallow_lock); goto cleanup; } args->connectivity_checked = 1; diff --git a/shallow.c b/shallow.c index 7fd04afed1..5010a6c732 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,25 @@ int is_repository_shallow(struct repository *r) return r->parsed_objects->is_shallow; } +static void reset_repository_shallow(struct repository *r) +{ + r->parsed_objects->is_shallow = -1; + stat_validity_clear(r->parsed_objects->shallow_stat); +} + +int commit_shallow_file(struct repository *r, struct lock_file *lk) +{ + int res = commit_lock_file(lk); + reset_repository_shallow(r); + return res; +} + +void rollback_shallow_file(struct repository *r, struct lock_file *lk) +{ + rollback_lock_file(lk); + reset_repository_shallow(r); +} + /* * TODO: use "int" elemtype instead of "int *" when/if commit-slab * supports a "valid" flag. @@ -410,10 +422,10 @@ void prune_shallow(unsigned options) if (write_in_full(fd, sb.buf, sb.len) < 0) die_errno("failed to write to %s", get_lock_file_path(&shallow_lock)); - commit_lock_file(&shallow_lock); + commit_shallow_file(the_repository, &shallow_lock); } else { unlink(git_path_shallow(the_repository)); - rollback_lock_file(&shallow_lock); + rollback_shallow_file(the_repository, &shallow_lock); } strbuf_release(&sb); } diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index d66b3656c0..a51c4b39d9 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -146,6 +146,35 @@ test_expect_success 'fetch --update-shallow' ' ) ' +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 + ) && + test_config -C notshallow fetch.writeCommitGraph true && + ( + cd notshallow && + 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 && + test_write_lines 8 7 6 5 4 3 >expect && + 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 repository 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 compatibility 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 thin wrappers over 'commit_lock_file' and 'rollback_lock_file' for use specifically when the lock is held over '.git/shallow'. These wrappers (appropriately called 'commit_shallow_file' and 'rollback_shallow_file') call into their respective functions in 'lockfile.h', but additionally reset validity checks used by the shallow machinery. Replace each instance of 'commit_lock_file' and 'rollback_lock_file' with 'commit_shallow_file' and 'rollback_shallow_file' when the lock being held is over the '.git/shallow' file. As a result, '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> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/receive-pack.c | 4 ++-- commit.h | 2 ++ fetch-pack.c | 10 +++++----- shallow.c | 30 +++++++++++++++++++++--------- t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++ 5 files changed, 59 insertions(+), 16 deletions(-)