* Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? @ 2020-04-14 20:22 Elijah Newren 2020-04-14 20:31 ` Taylor Blau ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Elijah Newren @ 2020-04-14 20:22 UTC (permalink / raw) To: Git Mailing List; +Cc: Derrick Stolee Hi, I was building a version of git for internal use, and thought I'd try turning on features.experimental to get more testing of it. The following test error in the testsuite scared me, though: t5537.9 (fetch --update-shallow): ... + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* remote: Enumerating objects: 18, done. remote: Counting objects: 100% (18/18), done. remote: Compressing objects: 100% (6/6), done. remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. From ../shallow/ * [new branch] master -> shallow/master * [new tag] heavy-tag -> heavy-tag * [new tag] light-tag -> light-tag error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 Passing -c fetch.writeCommitGraph=false to the fetch command in that test makes it pass. There were also a couple other tests that failed with features.experimental=true (in t5500), but those weren't scary -- they were just checking exact want/have lines and features.experimental is intended to change those. This test from t5537 was the only one that showed some unexpected fatal error. Thanks, Elijah ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-14 20:22 Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? Elijah Newren @ 2020-04-14 20:31 ` Taylor Blau 2020-04-14 20:31 ` Derrick Stolee 2020-04-15 20:54 ` Jonathan Nieder 2 siblings, 0 replies; 12+ messages in thread From: Taylor Blau @ 2020-04-14 20:31 UTC (permalink / raw) To: Elijah Newren; +Cc: Git Mailing List, Derrick Stolee On Tue, Apr 14, 2020 at 01:22:45PM -0700, Elijah Newren wrote: > Hi, > > I was building a version of git for internal use, and thought I'd try > turning on features.experimental to get more testing of it. The > following test error in the testsuite scared me, though: > > t5537.9 (fetch --update-shallow): > > ... > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > remote: Enumerating objects: 18, done. > remote: Counting objects: 100% (18/18), done. > remote: Compressing objects: 100% (6/6), done. > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. > From ../shallow/ > * [new branch] master -> shallow/master > * [new tag] heavy-tag -> heavy-tag > * [new tag] light-tag -> light-tag > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 > > Passing -c fetch.writeCommitGraph=false to the fetch command in that > test makes it pass. This failure makes sense, since taking the reachability closure over the ref tips will fail in shallow clones of repositories that have more than one commit. I wonder if fetch should be taught to avoid generating commit-graphs (and ignore 'fetch.writeCommitGraph') when in a shallow clone. > There were also a couple other tests that failed with > features.experimental=true (in t5500), but those weren't scary -- they > were just checking exact want/have lines and features.experimental is > intended to change those. This test from t5537 was the only one that > showed some unexpected fatal error. > > Thanks, > Elijah Thanks, Taylor ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-14 20:22 Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? Elijah Newren 2020-04-14 20:31 ` Taylor Blau @ 2020-04-14 20:31 ` Derrick Stolee 2020-04-14 23:50 ` Taylor Blau 2020-04-15 20:54 ` Jonathan Nieder 2 siblings, 1 reply; 12+ messages in thread From: Derrick Stolee @ 2020-04-14 20:31 UTC (permalink / raw) To: Elijah Newren, Git Mailing List On 4/14/2020 4:22 PM, Elijah Newren wrote: > Hi, > > I was building a version of git for internal use, and thought I'd try > turning on features.experimental to get more testing of it. The > following test error in the testsuite scared me, though: > > t5537.9 (fetch --update-shallow): > > ... > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > remote: Enumerating objects: 18, done. > remote: Counting objects: 100% (18/18), done. > remote: Compressing objects: 100% (6/6), done. > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. > From ../shallow/ > * [new branch] master -> shallow/master > * [new tag] heavy-tag -> heavy-tag > * [new tag] light-tag -> light-tag > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 > > Passing -c fetch.writeCommitGraph=false to the fetch command in that > test makes it pass. > > There were also a couple other tests that failed with > features.experimental=true (in t5500), but those weren't scary -- they > were just checking exact want/have lines and features.experimental is > intended to change those. This test from t5537 was the only one that > showed some unexpected fatal error. Well, commit-graphs are not supposed to do anything if we have shallow clones. We definitely don't load a commit-graph in that case. Seems like we need an extra check in write_commit_graph() to stop writing in the presence of shallow commits. -Stolee ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-14 20:31 ` Derrick Stolee @ 2020-04-14 23:50 ` Taylor Blau 2020-04-15 0:07 ` Taylor Blau ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Taylor Blau @ 2020-04-14 23:50 UTC (permalink / raw) To: Derrick Stolee; +Cc: Elijah Newren, Jonathan Tan, Git Mailing List On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote: > On 4/14/2020 4:22 PM, Elijah Newren wrote: > > Hi, > > > > I was building a version of git for internal use, and thought I'd try > > turning on features.experimental to get more testing of it. The > > following test error in the testsuite scared me, though: > > > > t5537.9 (fetch --update-shallow): > > > > ... > > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > > remote: Enumerating objects: 18, done. > > remote: Counting objects: 100% (18/18), done. > > remote: Compressing objects: 100% (6/6), done. > > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 > > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. > > From ../shallow/ > > * [new branch] master -> shallow/master > > * [new tag] heavy-tag -> heavy-tag > > * [new tag] light-tag -> light-tag > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 > > > > Passing -c fetch.writeCommitGraph=false to the fetch command in that > > test makes it pass. > > > > There were also a couple other tests that failed with > > features.experimental=true (in t5500), but those weren't scary -- they > > were just checking exact want/have lines and features.experimental is > > intended to change those. This test from t5537 was the only one that > > showed some unexpected fatal error. > > Well, commit-graphs are not supposed to do anything if we have > shallow clones. We definitely don't load a commit-graph in that > case. Seems like we need an extra check in write_commit_graph() > to stop writing in the presence of shallow commits. This rang a bell to me, too. There's a bug, but it's due to the mutative side-effects of 'is_repository_shallow' along with '--update-shallow' (a normal 'git fetch' works fine here, with or without fetch.writeCommitGraph). Here's a patch that I didn't sign-off on that fixes the problem for me. --- >8 --- Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate 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 object(s), and therefore can't take the reachability closure over commits to be written to the commit-graph). Address this by introducing 'reset_repository_shallow()', and calling it 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. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- commit.h | 1 + fetch-pack.c | 1 + shallow.c | 15 ++++++++------- 3 files changed, 10 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..051902ef6d 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args, if (*alternate_shallow_file == '\0') { /* --unshallow */ unlink_or_warn(git_path_shallow(the_repository)); rollback_lock_file(&shallow_lock); + reset_repository_shallow(the_repository); } else commit_lock_file(&shallow_lock); alternate_shallow_file = NULL; diff --git a/shallow.c b/shallow.c index 7fd04afed1..fac383dec9 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); } @@ -411,6 +411,7 @@ void prune_shallow(unsigned options) die_errno("failed to write to %s", get_lock_file_path(&shallow_lock)); commit_lock_file(&shallow_lock); + reset_repository_shallow(the_repository); } else { unlink(git_path_shallow(the_repository)); rollback_lock_file(&shallow_lock); -- 2.26.0.106.g9fadedd637 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-14 23:50 ` Taylor Blau @ 2020-04-15 0:07 ` Taylor Blau 2020-04-15 11:55 ` Derrick Stolee 2020-04-16 2:05 ` Jonathan Tan 2 siblings, 0 replies; 12+ messages in thread From: Taylor Blau @ 2020-04-15 0:07 UTC (permalink / raw) To: Git Mailing List; +Cc: Derrick Stolee, Elijah Newren, Jonathan Tan On Tue, Apr 14, 2020 at 05:50:57PM -0600, Taylor Blau wrote: > On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote: > > On 4/14/2020 4:22 PM, Elijah Newren wrote: > > > Hi, > > > > > > I was building a version of git for internal use, and thought I'd try > > > turning on features.experimental to get more testing of it. The > > > following test error in the testsuite scared me, though: > > > > > > t5537.9 (fetch --update-shallow): > > > > > > ... > > > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > > > remote: Enumerating objects: 18, done. > > > remote: Counting objects: 100% (18/18), done. > > > remote: Compressing objects: 100% (6/6), done. > > > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 > > > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. > > > From ../shallow/ > > > * [new branch] master -> shallow/master > > > * [new tag] heavy-tag -> heavy-tag > > > * [new tag] light-tag -> light-tag > > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 > > > > > > Passing -c fetch.writeCommitGraph=false to the fetch command in that > > > test makes it pass. > > > > > > There were also a couple other tests that failed with > > > features.experimental=true (in t5500), but those weren't scary -- they > > > were just checking exact want/have lines and features.experimental is > > > intended to change those. This test from t5537 was the only one that > > > showed some unexpected fatal error. > > > > Well, commit-graphs are not supposed to do anything if we have > > shallow clones. We definitely don't load a commit-graph in that > > case. Seems like we need an extra check in write_commit_graph() > > to stop writing in the presence of shallow commits. > > This rang a bell to me, too. There's a bug, but it's due to the mutative > side-effects of 'is_repository_shallow' along with '--update-shallow' (a > normal 'git fetch' works fine here, with or without > fetch.writeCommitGraph). > > Here's a patch that I didn't sign-off on that fixes the problem for me. Oh, apparently I did sign this one off ;). I'll blame that 'git commit --amend -vs' is muscle memory for me. In either case, this probably needs some work (if it's even the right approach) before queueing it. > --- >8 --- > > ... Thanks, Taylor ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-14 23:50 ` Taylor Blau 2020-04-15 0:07 ` Taylor Blau @ 2020-04-15 11:55 ` Derrick Stolee 2020-04-15 15:55 ` Taylor Blau 2020-04-15 18:07 ` Elijah Newren 2020-04-16 2:05 ` Jonathan Tan 2 siblings, 2 replies; 12+ messages in thread From: Derrick Stolee @ 2020-04-15 11:55 UTC (permalink / raw) To: Taylor Blau; +Cc: Elijah Newren, Jonathan Tan, Git Mailing List On 4/14/2020 7:50 PM, Taylor Blau wrote: > On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote: >> On 4/14/2020 4:22 PM, Elijah Newren wrote: >>> Hi, >>> >>> I was building a version of git for internal use, and thought I'd try >>> turning on features.experimental to get more testing of it. The >>> following test error in the testsuite scared me, though: >>> >>> t5537.9 (fetch --update-shallow): >>> >>> ... >>> + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* [snip] >> Well, commit-graphs are not supposed to do anything if we have >> shallow clones. We definitely don't load a commit-graph in that >> case. Seems like we need an extra check in write_commit_graph() >> to stop writing in the presence of shallow commits. Here, I assumed that the commit-graph wasn't checking the shallow status appropriately, but... > This rang a bell to me, too. There's a bug, but it's due to the mutative > side-effects of 'is_repository_shallow' along with '--update-shallow' (a > normal 'git fetch' works fine here, with or without > fetch.writeCommitGraph). ...this makes sense as to why this particular case failed. (Please allow me this brief moment to communicate my extreme dislike of shallow clones. There, I'm done.) > Here's a patch that I didn't sign-off on that fixes the problem for me. > > --- >8 --- > > Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate > > 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. compatibility > This causes problems in shallow repositories with at least shallow refs > that have at least one ancestor (since the client won't have those > object(s), and therefore can't take the reachability closure over > commits to be written to the commit-graph). > > Address this by introducing 'reset_repository_shallow()', and calling it > 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. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > commit.h | 1 + > fetch-pack.c | 1 + > shallow.c | 15 ++++++++------- > 3 files changed, 10 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..051902ef6d 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args, > if (*alternate_shallow_file == '\0') { /* --unshallow */ > unlink_or_warn(git_path_shallow(the_repository)); > rollback_lock_file(&shallow_lock); > + reset_repository_shallow(the_repository); > } else > commit_lock_file(&shallow_lock); > alternate_shallow_file = NULL; > diff --git a/shallow.c b/shallow.c > index 7fd04afed1..fac383dec9 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. > - */ > - It's always good to remove these NEEDSWORK comments. I wonder if the problem is more complicated than your patch makes it seem, or else the original author would have done it correctly at first. But you are definitely closing out one dangling side-effect, which is an improvement. > 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); > } > > @@ -411,6 +411,7 @@ void prune_shallow(unsigned options) > die_errno("failed to write to %s", > get_lock_file_path(&shallow_lock)); > commit_lock_file(&shallow_lock); > + reset_repository_shallow(the_repository); > } else { These are likely good places to call reset_repository_shallow(), but perhaps we should also call it here in commit-graph.c? static int commit_graph_compatible(struct repository *r) { if (!r->gitdir) return 0; if (read_replace_refs) { prepare_replace_object(r); if (hashmap_get_size(&r->objects->replace_map->map)) return 0; } prepare_commit_graft(r); if (r->parsed_objects && r->parsed_objects->grafts_nr) return 0; + reset_repository_shallow(r); if (is_repository_shallow(r)) return 0; return 1; } Or am I misunderstanding the state that reset_repository_shallow() puts us in? My expectation is that is_repository_shallow() will act as if the shallow variables had never been set and will look for shallow data from disk. Thanks, -Stolee ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-15 11:55 ` Derrick Stolee @ 2020-04-15 15:55 ` Taylor Blau 2020-04-15 18:07 ` Elijah Newren 1 sibling, 0 replies; 12+ messages in thread From: Taylor Blau @ 2020-04-15 15:55 UTC (permalink / raw) To: Derrick Stolee; +Cc: Taylor Blau, Elijah Newren, Jonathan Tan, Git Mailing List On Wed, Apr 15, 2020 at 07:55:19AM -0400, Derrick Stolee wrote: > On 4/14/2020 7:50 PM, Taylor Blau wrote: > > On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote: > >> On 4/14/2020 4:22 PM, Elijah Newren wrote: > >>> Hi, > >>> > >>> I was building a version of git for internal use, and thought I'd try > >>> turning on features.experimental to get more testing of it. The > >>> following test error in the testsuite scared me, though: > >>> > >>> t5537.9 (fetch --update-shallow): > >>> > >>> ... > >>> + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > [snip] > >> Well, commit-graphs are not supposed to do anything if we have > >> shallow clones. We definitely don't load a commit-graph in that > >> case. Seems like we need an extra check in write_commit_graph() > >> to stop writing in the presence of shallow commits. > > Here, I assumed that the commit-graph wasn't checking the shallow status > appropriately, but... > > > This rang a bell to me, too. There's a bug, but it's due to the mutative > > side-effects of 'is_repository_shallow' along with '--update-shallow' (a > > normal 'git fetch' works fine here, with or without > > fetch.writeCommitGraph). > > ...this makes sense as to why this particular case failed. > > (Please allow me this brief moment to communicate my extreme dislike > of shallow clones. There, I'm done.) Noted ;). > > Here's a patch that I didn't sign-off on that fixes the problem for me. > > > > --- >8 --- > > > > Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate > > > > 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. > > compatibility Whoops, thanks for pointing these out. > > This causes problems in shallow repositories with at least shallow refs > > that have at least one ancestor (since the client won't have those > > object(s), and therefore can't take the reachability closure over > > commits to be written to the commit-graph). > > > > Address this by introducing 'reset_repository_shallow()', and calling it > > 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. > > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > commit.h | 1 + > > fetch-pack.c | 1 + > > shallow.c | 15 ++++++++------- > > 3 files changed, 10 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..051902ef6d 100644 > > --- a/fetch-pack.c > > +++ b/fetch-pack.c > > @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args, > > if (*alternate_shallow_file == '\0') { /* --unshallow */ > > unlink_or_warn(git_path_shallow(the_repository)); > > rollback_lock_file(&shallow_lock); > > + reset_repository_shallow(the_repository); > > } else > > commit_lock_file(&shallow_lock); > > alternate_shallow_file = NULL; > > diff --git a/shallow.c b/shallow.c > > index 7fd04afed1..fac383dec9 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. > > - */ > > - > > It's always good to remove these NEEDSWORK comments. I wonder if the > problem is more complicated than your patch makes it seem, or else > the original author would have done it correctly at first. > > But you are definitely closing out one dangling side-effect, which is > an improvement. Yeah, I have no idea if there are other spots that I'm missing (I only spent a few minutes on this patch before leaving for the day to go eat dinner), hence why I CC'd Jonathan Tan to see if he had anything else in mind when he wrote this 'NEEDSWORK' comment. If he feels that this is a generally good direction, I'm more than happy to look for other spots myself, address them, and then submit this as a real patch. > > 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); > > } > > > > @@ -411,6 +411,7 @@ void prune_shallow(unsigned options) > > die_errno("failed to write to %s", > > get_lock_file_path(&shallow_lock)); > > commit_lock_file(&shallow_lock); > > + reset_repository_shallow(the_repository); > > } else { > > These are likely good places to call reset_repository_shallow(), > but perhaps we should also call it here in commit-graph.c? > > static int commit_graph_compatible(struct repository *r) > { > if (!r->gitdir) > return 0; > > if (read_replace_refs) { > prepare_replace_object(r); > if (hashmap_get_size(&r->objects->replace_map->map)) > return 0; > } > > prepare_commit_graft(r); > if (r->parsed_objects && r->parsed_objects->grafts_nr) > return 0; > + reset_repository_shallow(r); > if (is_repository_shallow(r)) > return 0; > > return 1; > } > > Or am I misunderstanding the state that reset_repository_shallow() > puts us in? My expectation is that is_repository_shallow() will > act as if the shallow variables had never been set and will look > for shallow data from disk. You're not misunderstanding it at all. I don't think that this location is strictly necessary, since all of the other locations that change '.git/shallow' are already invaliding the shallow-ness checks, so by the time we get to this point 'is_repository_shallow' has to take the slow path and redetermine whether or not we are actually shallow. So, I don't think this location is strictly necessary, and it's potentially slowing us down a little bit, but it is hardening us against other parts of the code that may not call 'reset_repository_shallow' when they should have. > Thanks, > -Stolee Thanks for the review. Thanks, Taylor ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-15 11:55 ` Derrick Stolee 2020-04-15 15:55 ` Taylor Blau @ 2020-04-15 18:07 ` Elijah Newren 1 sibling, 0 replies; 12+ messages in thread From: Elijah Newren @ 2020-04-15 18:07 UTC (permalink / raw) To: Derrick Stolee; +Cc: Taylor Blau, Jonathan Tan, Git Mailing List On Wed, Apr 15, 2020 at 4:55 AM Derrick Stolee <stolee@gmail.com> wrote: > (Please allow me this brief moment to communicate my extreme dislike > of shallow clones. There, I'm done.) I share your extreme dislike, but I've merely succeeded in discouraging some people from using them, not in stopping their use altogether. (I don't have anything to add to the review of the patches, other than I'm happy to see folks trying to address the issue.) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-14 23:50 ` Taylor Blau 2020-04-15 0:07 ` Taylor Blau 2020-04-15 11:55 ` Derrick Stolee @ 2020-04-16 2:05 ` Jonathan Tan 2 siblings, 0 replies; 12+ messages in thread From: Jonathan Tan @ 2020-04-16 2:05 UTC (permalink / raw) To: me; +Cc: stolee, newren, jonathantanmy, git > Here's a patch that I didn't sign-off on that fixes the problem for me. > > --- >8 --- > > Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate > > 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'. Thanks for the reference to my commit. When I wrote that, if I remember correctly, I found it difficult to be able to guarantee that the clearing of is_shallow and shallow_stat was performed upon every commit-lock (which creates a new shallow file) and <unlink+rollback-lock> (which removes the shallow file, potentially changing the repo state from shallow to none), so I added the NEEDSWORK instead. I see that this patch makes some progress in solving that. > @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args, > if (*alternate_shallow_file == '\0') { /* --unshallow */ > unlink_or_warn(git_path_shallow(the_repository)); > rollback_lock_file(&shallow_lock); > + reset_repository_shallow(the_repository); > } else > commit_lock_file(&shallow_lock); > alternate_shallow_file = NULL; Here, do you need a reset in the "else" branch as well? (I.e. in both branches, so you can put it outside the "if" block.) I didn't look at it too closely, but I would think that when the shallow_lock file is committed, there might have been no shallow beforehand, changing the state from no-shallow to shallow. > @@ -411,6 +411,7 @@ void prune_shallow(unsigned options) > die_errno("failed to write to %s", > get_lock_file_path(&shallow_lock)); > commit_lock_file(&shallow_lock); > + reset_repository_shallow(the_repository); > } else { > unlink(git_path_shallow(the_repository)); > rollback_lock_file(&shallow_lock); And the opposite case here - reset in the "else" branch because the state could have changed from shallow to no-shallow. 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-14 20:22 Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? Elijah Newren 2020-04-14 20:31 ` Taylor Blau 2020-04-14 20:31 ` Derrick Stolee @ 2020-04-15 20:54 ` Jonathan Nieder 2020-04-15 22:54 ` Elijah Newren 2 siblings, 1 reply; 12+ messages in thread From: Jonathan Nieder @ 2020-04-15 20:54 UTC (permalink / raw) To: Elijah Newren; +Cc: Git Mailing List, Derrick Stolee, Taylor Blau, Jonathan Tan Hi, Elijah Newren wrote: > I was building a version of git for internal use, and thought I'd try > turning on features.experimental to get more testing of it. The > following test error in the testsuite scared me, though: > > t5537.9 (fetch --update-shallow): > > ... > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > remote: Enumerating objects: 18, done. > remote: Counting objects: 100% (18/18), done. > remote: Compressing objects: 100% (6/6), done. > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. > From ../shallow/ > * [new branch] master -> shallow/master > * [new tag] heavy-tag -> heavy-tag > * [new tag] light-tag -> light-tag > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 > > Passing -c fetch.writeCommitGraph=false to the fetch command in that > test makes it pass. Oh! Thanks for checking this. At $DAYJOB this was the week we were going to roll out features.experimental. Time to roll that back... How did you go about the experiment? Does Taylor's patch make it pass? (I'm thinking it would be nice to have a GIT_TEST_EXPERIMENTAL_FEATURES setting that uses test_config_global at test init time.) Thanks, Jonathan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-15 20:54 ` Jonathan Nieder @ 2020-04-15 22:54 ` Elijah Newren 2020-04-16 0:47 ` Taylor Blau 0 siblings, 1 reply; 12+ messages in thread From: Elijah Newren @ 2020-04-15 22:54 UTC (permalink / raw) To: Jonathan Nieder Cc: Git Mailing List, Derrick Stolee, Taylor Blau, Jonathan Tan On Wed, Apr 15, 2020 at 1:54 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > Hi, > > Elijah Newren wrote: > > > I was building a version of git for internal use, and thought I'd try > > turning on features.experimental to get more testing of it. The > > following test error in the testsuite scared me, though: > > > > t5537.9 (fetch --update-shallow): > > > > ... > > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > > remote: Enumerating objects: 18, done. > > remote: Counting objects: 100% (18/18), done. > > remote: Compressing objects: 100% (6/6), done. > > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 > > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. > > From ../shallow/ > > * [new branch] master -> shallow/master > > * [new tag] heavy-tag -> heavy-tag > > * [new tag] light-tag -> light-tag > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 > > > > Passing -c fetch.writeCommitGraph=false to the fetch command in that > > test makes it pass. > > Oh! Thanks for checking this. At $DAYJOB this was the week we were > going to roll out features.experimental. Time to roll that back... > > How did you go about the experiment? Does Taylor's patch make it pass? Yes, Taylor's patch makes the experiment pass. My experiment was pretty simple; modify the code so that features.experimental defaults to true: diff --git a/repo-settings.c b/repo-settings.c index a703e407a3..b39bc21b99 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -8,6 +8,7 @@ void prepare_repo_settings(struct repository *r) { int value; char *strval; + int feature_experimental; if (r->settings.initialized) return; @@ -51,7 +54,10 @@ void prepare_repo_settings(struct repository *r) } if (!repo_config_get_bool(r, "fetch.writecommitgraph", &value)) r->settings.fetch_write_commit_graph = value; - if (!repo_config_get_bool(r, "feature.experimental", &value) && value) { + feature_experimental = 1; + if (!repo_config_get_bool(r, "feature.experimental", &value)) + feature_experimental = value; + if (feature_experimental) { UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1); UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING); UPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1); (based on 2.26.0; Stolee has since made pack_use_sparse default to true anyway) and then run the test suite and see what breaks. For the tests that fail, check if the failure is expected based on different defaults and the code still functions correctly with the new defaults (and update the test manually if so), or whether the test failure represents some potentially scary problem. This was the only bug exposed by this little experiment. > (I'm thinking it would be nice to have a > GIT_TEST_EXPERIMENTAL_FEATURES setting that uses test_config_global at > test init time.) ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? 2020-04-15 22:54 ` Elijah Newren @ 2020-04-16 0:47 ` Taylor Blau 0 siblings, 0 replies; 12+ messages in thread From: Taylor Blau @ 2020-04-16 0:47 UTC (permalink / raw) To: Elijah Newren Cc: Jonathan Nieder, Git Mailing List, Derrick Stolee, Taylor Blau, Jonathan Tan On Wed, Apr 15, 2020 at 03:54:09PM -0700, Elijah Newren wrote: > On Wed, Apr 15, 2020 at 1:54 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > > > Hi, > > > > Elijah Newren wrote: > > > > > I was building a version of git for internal use, and thought I'd try > > > turning on features.experimental to get more testing of it. The > > > following test error in the testsuite scared me, though: > > > > > > t5537.9 (fetch --update-shallow): > > > > > > ... > > > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* > > > remote: Enumerating objects: 18, done. > > > remote: Counting objects: 100% (18/18), done. > > > remote: Compressing objects: 100% (6/6), done. > > > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0 > > > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done. > > > From ../shallow/ > > > * [new branch] master -> shallow/master > > > * [new tag] heavy-tag -> heavy-tag > > > * [new tag] light-tag -> light-tag > > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5 > > > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5 > > > > > > Passing -c fetch.writeCommitGraph=false to the fetch command in that > > > test makes it pass. > > > > Oh! Thanks for checking this. At $DAYJOB this was the week we were > > going to roll out features.experimental. Time to roll that back... > > > > How did you go about the experiment? Does Taylor's patch make it pass? > > Yes, Taylor's patch makes the experiment pass. My experiment was > pretty simple; modify the code so that features.experimental defaults > to true: I'm glad to hear that it is passing now. I would like to have Jonathan Tan chime in on whether or not this patch is making sense, but if so, I'll prepare it for real and send it out. Thanks, Taylor ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-16 2:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-14 20:22 Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? Elijah Newren 2020-04-14 20:31 ` Taylor Blau 2020-04-14 20:31 ` Derrick Stolee 2020-04-14 23:50 ` Taylor Blau 2020-04-15 0:07 ` Taylor Blau 2020-04-15 11:55 ` Derrick Stolee 2020-04-15 15:55 ` Taylor Blau 2020-04-15 18:07 ` Elijah Newren 2020-04-16 2:05 ` Jonathan Tan 2020-04-15 20:54 ` Jonathan Nieder 2020-04-15 22:54 ` Elijah Newren 2020-04-16 0:47 ` Taylor Blau
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).