{"thread":{"id":"53231","subject":"Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","startedAt":"2020-04-14T20:23:09Z","lastAt":"2020-04-16T02:05:33Z","messageCount":12,"participants":["Elijah Newren","Taylor Blau","Derrick Stolee","Jonathan Nieder","Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"395439","messageId":"CABPp-BHGubUX5o9KsQaoh_UFjFh2PaMkkJhCao+5LGnFc0dQNg@mail.gmail.com","threadId":"53231","inReplyTo":null,"subject":"Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-14T20:22:45Z","receivedAt":"2020-04-14T20:23:09Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nI was building a version of git for internal use, and thought I'd try\nturning on features.experimental to get more testing of it.  The\nfollowing test error in the testsuite scared me, though:\n\nt5537.9 (fetch --update-shallow):\n\n...\n+ git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\nremote: Enumerating objects: 18, done.\nremote: Counting objects: 100% (18/18), done.\nremote: Compressing objects: 100% (6/6), done.\nremote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\nUnpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\nFrom ../shallow/\n * [new branch]      master     -> shallow/master\n * [new tag]         heavy-tag  -> heavy-tag\n * [new tag]         light-tag  -> light-tag\nerror: Could not read ac67d3021b4319951fb176469d7732e6914530c5\nerror: Could not read ac67d3021b4319951fb176469d7732e6914530c5\nerror: Could not read ac67d3021b4319951fb176469d7732e6914530c5\nfatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n\nPassing -c fetch.writeCommitGraph=false to the fetch command in that\ntest makes it pass.\n\nThere were also a couple other tests that failed with\nfeatures.experimental=true (in t5500), but those weren't scary -- they\nwere just checking exact want/have lines and features.experimental is\nintended to change those.  This test from t5537 was the only one that\nshowed some unexpected fatal error.\n\nThanks,\nElijah\n"},{"id":"395443","messageId":"20200414203101.GE93424@syl.local","threadId":"53231","inReplyTo":"CABPp-BHGubUX5o9KsQaoh_UFjFh2PaMkkJhCao+5LGnFc0dQNg@mail.gmail.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T20:31:01Z","receivedAt":"2020-04-14T20:31:15Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 14, 2020 at 01:22:45PM -0700, Elijah Newren wrote:\n> Hi,\n>\n> I was building a version of git for internal use, and thought I'd try\n> turning on features.experimental to get more testing of it.  The\n> following test error in the testsuite scared me, though:\n>\n> t5537.9 (fetch --update-shallow):\n>\n> ...\n> + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> remote: Enumerating objects: 18, done.\n> remote: Counting objects: 100% (18/18), done.\n> remote: Compressing objects: 100% (6/6), done.\n> remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\n> Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\n> From ../shallow/\n>  * [new branch]      master     -> shallow/master\n>  * [new tag]         heavy-tag  -> heavy-tag\n>  * [new tag]         light-tag  -> light-tag\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n>\n> Passing -c fetch.writeCommitGraph=false to the fetch command in that\n> test makes it pass.\n\nThis failure makes sense, since taking the reachability closure over\nthe ref tips will fail in shallow clones of repositories that have more\nthan one commit.\n\nI wonder if fetch should be taught to avoid generating commit-graphs\n(and ignore 'fetch.writeCommitGraph') when in a shallow clone.\n\n> There were also a couple other tests that failed with\n> features.experimental=true (in t5500), but those weren't scary -- they\n> were just checking exact want/have lines and features.experimental is\n> intended to change those.  This test from t5537 was the only one that\n> showed some unexpected fatal error.\n>\n> Thanks,\n> Elijah\n\nThanks,\nTaylor\n"},{"id":"395444","messageId":"aca12331-f8e3-28a2-acb6-df9d7e2e70e1@gmail.com","threadId":"53231","inReplyTo":"CABPp-BHGubUX5o9KsQaoh_UFjFh2PaMkkJhCao+5LGnFc0dQNg@mail.gmail.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-14T20:31:19Z","receivedAt":"2020-04-14T20:31:31Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/14/2020 4:22 PM, Elijah Newren wrote:\n> Hi,\n> \n> I was building a version of git for internal use, and thought I'd try\n> turning on features.experimental to get more testing of it.  The\n> following test error in the testsuite scared me, though:\n> \n> t5537.9 (fetch --update-shallow):\n> \n> ...\n> + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> remote: Enumerating objects: 18, done.\n> remote: Counting objects: 100% (18/18), done.\n> remote: Compressing objects: 100% (6/6), done.\n> remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\n> Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\n> From ../shallow/\n>  * [new branch]      master     -> shallow/master\n>  * [new tag]         heavy-tag  -> heavy-tag\n>  * [new tag]         light-tag  -> light-tag\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n> \n> Passing -c fetch.writeCommitGraph=false to the fetch command in that\n> test makes it pass.\n> \n> There were also a couple other tests that failed with\n> features.experimental=true (in t5500), but those weren't scary -- they\n> were just checking exact want/have lines and features.experimental is\n> intended to change those.  This test from t5537 was the only one that\n> showed some unexpected fatal error.\n\nWell, commit-graphs are not supposed to do anything if we have\nshallow clones. We definitely don't load a commit-graph in that\ncase. Seems like we need an extra check in write_commit_graph()\nto stop writing in the presence of shallow commits.\n\n-Stolee\n"},{"id":"395456","messageId":"20200414235057.GA6863@syl.local","threadId":"53231","inReplyTo":"aca12331-f8e3-28a2-acb6-df9d7e2e70e1@gmail.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-14T23:50:57Z","receivedAt":"2020-04-14T23:51:13Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote:\n> On 4/14/2020 4:22 PM, Elijah Newren wrote:\n> > Hi,\n> >\n> > I was building a version of git for internal use, and thought I'd try\n> > turning on features.experimental to get more testing of it.  The\n> > following test error in the testsuite scared me, though:\n> >\n> > t5537.9 (fetch --update-shallow):\n> >\n> > ...\n> > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> > remote: Enumerating objects: 18, done.\n> > remote: Counting objects: 100% (18/18), done.\n> > remote: Compressing objects: 100% (6/6), done.\n> > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\n> > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\n> > From ../shallow/\n> >  * [new branch]      master     -> shallow/master\n> >  * [new tag]         heavy-tag  -> heavy-tag\n> >  * [new tag]         light-tag  -> light-tag\n> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n> >\n> > Passing -c fetch.writeCommitGraph=false to the fetch command in that\n> > test makes it pass.\n> >\n> > There were also a couple other tests that failed with\n> > features.experimental=true (in t5500), but those weren't scary -- they\n> > were just checking exact want/have lines and features.experimental is\n> > intended to change those.  This test from t5537 was the only one that\n> > showed some unexpected fatal error.\n>\n> Well, commit-graphs are not supposed to do anything if we have\n> shallow clones. We definitely don't load a commit-graph in that\n> case. Seems like we need an extra check in write_commit_graph()\n> to stop writing in the presence of shallow commits.\n\nThis rang a bell to me, too. There's a bug, but it's due to the mutative\nside-effects of 'is_repository_shallow' along with '--update-shallow' (a\nnormal 'git fetch' works fine here, with or without\nfetch.writeCommitGraph).\n\nHere's a patch that I didn't sign-off on that fixes the problem for me.\n\n--- >8 ---\n\nSubject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate\n\nIn bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,\n2019-01-10), the author noted that 'is_repository_shallow' produces\nvisible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.\n\nThis is a problem for e.g., fetching with '--update-shallow' in a\nshallow repsoitory with 'fetch.writeCommitGraph' enabled, since the\nupdate to '.git/shallow' will cause Git to think that the repository\n*isn't* shallow when it is, thereby circumventing the commit-graph\ncompatability check.\n\nThis causes problems in shallow repositories with at least shallow refs\nthat have at least one ancestor (since the client won't have those\nobject(s), and therefore can't take the reachability closure over\ncommits to be written to the commit-graph).\n\nAddress this by introducing 'reset_repository_shallow()', and calling it\nwhen the shallow file is updated, forcing 'is_repository_shallow' to\nre-evaluate whether the repository is still shallow after fetching in\nthe above scenario.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit.h     |  1 +\n fetch-pack.c |  1 +\n shallow.c    | 15 ++++++++-------\n 3 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 008a0fa4a0..ee1ba139d4 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid);\n int unregister_shallow(const struct object_id *oid);\n int for_each_commit_graft(each_commit_graft_fn, void *);\n int is_repository_shallow(struct repository *r);\n+void reset_repository_shallow(struct repository *r);\n struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n struct commit_list *get_shallow_commits_by_rev_list(\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1734a573b0..051902ef6d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n \t\t\tunlink_or_warn(git_path_shallow(the_repository));\n \t\t\trollback_lock_file(&shallow_lock);\n+\t\t\treset_repository_shallow(the_repository);\n \t\t} else\n \t\t\tcommit_lock_file(&shallow_lock);\n \t\talternate_shallow_file = NULL;\ndiff --git a/shallow.c b/shallow.c\nindex 7fd04afed1..fac383dec9 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n\n int is_repository_shallow(struct repository *r)\n {\n-\t/*\n-\t * NEEDSWORK: This function updates\n-\t * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but\n-\t * there is no corresponding function to clear them when the shallow\n-\t * file is updated.\n-\t */\n-\n \tFILE *fp;\n \tchar buf[1024];\n \tconst char *path = r->parsed_objects->alternate_shallow_file;\n@@ -79,6 +72,12 @@ int is_repository_shallow(struct repository *r)\n \treturn r->parsed_objects->is_shallow;\n }\n\n+void reset_repository_shallow(struct repository *r)\n+{\n+\tr->parsed_objects->is_shallow = -1;\n+\tstat_validity_clear(r->parsed_objects->shallow_stat);\n+}\n+\n /*\n  * TODO: use \"int\" elemtype instead of \"int *\" when/if commit-slab\n  * supports a \"valid\" flag.\n@@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t * shallow file\".\n \t\t */\n \t\t*alternate_shallow_file = \"\";\n+\treset_repository_shallow(the_repository);\n \tstrbuf_release(&sb);\n }\n\n@@ -411,6 +411,7 @@ void prune_shallow(unsigned options)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t  get_lock_file_path(&shallow_lock));\n \t\tcommit_lock_file(&shallow_lock);\n+\t\treset_repository_shallow(the_repository);\n \t} else {\n \t\tunlink(git_path_shallow(the_repository));\n \t\trollback_lock_file(&shallow_lock);\n--\n2.26.0.106.g9fadedd637\n"},{"id":"395457","messageId":"20200415000728.GA7382@syl.local","threadId":"53231","inReplyTo":"20200414235057.GA6863@syl.local","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T00:07:28Z","receivedAt":"2020-04-15T00:07:44Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 14, 2020 at 05:50:57PM -0600, Taylor Blau wrote:\n> On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote:\n> > On 4/14/2020 4:22 PM, Elijah Newren wrote:\n> > > Hi,\n> > >\n> > > I was building a version of git for internal use, and thought I'd try\n> > > turning on features.experimental to get more testing of it.  The\n> > > following test error in the testsuite scared me, though:\n> > >\n> > > t5537.9 (fetch --update-shallow):\n> > >\n> > > ...\n> > > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> > > remote: Enumerating objects: 18, done.\n> > > remote: Counting objects: 100% (18/18), done.\n> > > remote: Compressing objects: 100% (6/6), done.\n> > > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\n> > > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\n> > > From ../shallow/\n> > >  * [new branch]      master     -> shallow/master\n> > >  * [new tag]         heavy-tag  -> heavy-tag\n> > >  * [new tag]         light-tag  -> light-tag\n> > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n> > >\n> > > Passing -c fetch.writeCommitGraph=false to the fetch command in that\n> > > test makes it pass.\n> > >\n> > > There were also a couple other tests that failed with\n> > > features.experimental=true (in t5500), but those weren't scary -- they\n> > > were just checking exact want/have lines and features.experimental is\n> > > intended to change those.  This test from t5537 was the only one that\n> > > showed some unexpected fatal error.\n> >\n> > Well, commit-graphs are not supposed to do anything if we have\n> > shallow clones. We definitely don't load a commit-graph in that\n> > case. Seems like we need an extra check in write_commit_graph()\n> > to stop writing in the presence of shallow commits.\n>\n> This rang a bell to me, too. There's a bug, but it's due to the mutative\n> side-effects of 'is_repository_shallow' along with '--update-shallow' (a\n> normal 'git fetch' works fine here, with or without\n> fetch.writeCommitGraph).\n>\n> Here's a patch that I didn't sign-off on that fixes the problem for me.\n\nOh, apparently I did sign this one off ;). I'll blame that 'git commit\n--amend -vs' is muscle memory for me. In either case, this probably\nneeds some work (if it's even the right approach) before queueing it.\n\n> --- >8 ---\n>\n> ...\n\nThanks,\nTaylor\n"},{"id":"395489","messageId":"cdc80c0b-696f-19b8-d132-592782537b03@gmail.com","threadId":"53231","inReplyTo":"20200414235057.GA6863@syl.local","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-15T11:55:19Z","receivedAt":"2020-04-15T11:55:47Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/14/2020 7:50 PM, Taylor Blau wrote:\n> On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote:\n>> On 4/14/2020 4:22 PM, Elijah Newren wrote:\n>>> Hi,\n>>>\n>>> I was building a version of git for internal use, and thought I'd try\n>>> turning on features.experimental to get more testing of it.  The\n>>> following test error in the testsuite scared me, though:\n>>>\n>>> t5537.9 (fetch --update-shallow):\n>>>\n>>> ...\n>>> + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n[snip]\n>> Well, commit-graphs are not supposed to do anything if we have\n>> shallow clones. We definitely don't load a commit-graph in that\n>> case. Seems like we need an extra check in write_commit_graph()\n>> to stop writing in the presence of shallow commits.\n\nHere, I assumed that the commit-graph wasn't checking the shallow status\nappropriately, but...\n\n> This rang a bell to me, too. There's a bug, but it's due to the mutative\n> side-effects of 'is_repository_shallow' along with '--update-shallow' (a\n> normal 'git fetch' works fine here, with or without\n> fetch.writeCommitGraph).\n\n...this makes sense as to why this particular case failed.\n\n(Please allow me this brief moment to communicate my extreme dislike\nof shallow clones. There, I'm done.)\n \n> Here's a patch that I didn't sign-off on that fixes the problem for me.\n> \n> --- >8 ---\n> \n> Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate\n> \n> In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,\n> 2019-01-10), the author noted that 'is_repository_shallow' produces\n> visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.\n> \n> This is a problem for e.g., fetching with '--update-shallow' in a\n> shallow repsoitory with 'fetch.writeCommitGraph' enabled, since the\n\nrepository\n\n> update to '.git/shallow' will cause Git to think that the repository\n> *isn't* shallow when it is, thereby circumventing the commit-graph\n> compatability check.\n\ncompatibility\n\n> This causes problems in shallow repositories with at least shallow refs\n> that have at least one ancestor (since the client won't have those\n> object(s), and therefore can't take the reachability closure over\n> commits to be written to the commit-graph).\n> \n> Address this by introducing 'reset_repository_shallow()', and calling it\n> when the shallow file is updated, forcing 'is_repository_shallow' to\n> re-evaluate whether the repository is still shallow after fetching in\n> the above scenario.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  commit.h     |  1 +\n>  fetch-pack.c |  1 +\n>  shallow.c    | 15 ++++++++-------\n>  3 files changed, 10 insertions(+), 7 deletions(-)\n> \n> diff --git a/commit.h b/commit.h\n> index 008a0fa4a0..ee1ba139d4 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid);\n>  int unregister_shallow(const struct object_id *oid);\n>  int for_each_commit_graft(each_commit_graft_fn, void *);\n>  int is_repository_shallow(struct repository *r);\n> +void reset_repository_shallow(struct repository *r);\n>  struct commit_list *get_shallow_commits(struct object_array *heads,\n>  \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n>  struct commit_list *get_shallow_commits_by_rev_list(\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 1734a573b0..051902ef6d 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args,\n>  \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n>  \t\t\tunlink_or_warn(git_path_shallow(the_repository));\n>  \t\t\trollback_lock_file(&shallow_lock);\n> +\t\t\treset_repository_shallow(the_repository);\n>  \t\t} else\n>  \t\t\tcommit_lock_file(&shallow_lock);\n>  \t\talternate_shallow_file = NULL;\n> diff --git a/shallow.c b/shallow.c\n> index 7fd04afed1..fac383dec9 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n> \n>  int is_repository_shallow(struct repository *r)\n>  {\n> -\t/*\n> -\t * NEEDSWORK: This function updates\n> -\t * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but\n> -\t * there is no corresponding function to clear them when the shallow\n> -\t * file is updated.\n> -\t */\n> -\n\nIt's always good to remove these NEEDSWORK comments. I wonder if the\nproblem is more complicated than your patch makes it seem, or else\nthe original author would have done it correctly at first.\n\nBut you are definitely closing out one dangling side-effect, which is\nan improvement.\n\n>  \tFILE *fp;\n>  \tchar buf[1024];\n>  \tconst char *path = r->parsed_objects->alternate_shallow_file;\n> @@ -79,6 +72,12 @@ int is_repository_shallow(struct repository *r)\n>  \treturn r->parsed_objects->is_shallow;\n>  }\n> \n> +void reset_repository_shallow(struct repository *r)\n> +{\n> +\tr->parsed_objects->is_shallow = -1;\n> +\tstat_validity_clear(r->parsed_objects->shallow_stat);\n> +}\n> +\n>  /*\n>   * TODO: use \"int\" elemtype instead of \"int *\" when/if commit-slab\n>   * supports a \"valid\" flag.\n> @@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n>  \t\t * shallow file\".\n>  \t\t */\n>  \t\t*alternate_shallow_file = \"\";\n> +\treset_repository_shallow(the_repository);\n>  \tstrbuf_release(&sb);\n>  }\n> \n> @@ -411,6 +411,7 @@ void prune_shallow(unsigned options)\n>  \t\t\tdie_errno(\"failed to write to %s\",\n>  \t\t\t\t  get_lock_file_path(&shallow_lock));\n>  \t\tcommit_lock_file(&shallow_lock);\n> +\t\treset_repository_shallow(the_repository);\n>  \t} else {\n\nThese are likely good places to call reset_repository_shallow(),\nbut perhaps we should also call it here in commit-graph.c?\n\n static int commit_graph_compatible(struct repository *r)\n {\n        if (!r->gitdir)\n                return 0;\n \n        if (read_replace_refs) {\n                prepare_replace_object(r);\n                if (hashmap_get_size(&r->objects->replace_map->map))\n                        return 0;\n        }\n \n        prepare_commit_graft(r);\n        if (r->parsed_objects && r->parsed_objects->grafts_nr)\n                return 0;\n+       reset_repository_shallow(r);\n        if (is_repository_shallow(r))\n                return 0;\n \n        return 1;\n }\n\nOr am I misunderstanding the state that reset_repository_shallow()\nputs us in? My expectation is that is_repository_shallow() will\nact as if the shallow variables had never been set and will look\nfor shallow data from disk.\n\nThanks,\n-Stolee\n"},{"id":"395510","messageId":"20200415155551.GB22823@syl.local","threadId":"53231","inReplyTo":"cdc80c0b-696f-19b8-d132-592782537b03@gmail.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T15:55:51Z","receivedAt":"2020-04-15T15:56:02Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 15, 2020 at 07:55:19AM -0400, Derrick Stolee wrote:\n> On 4/14/2020 7:50 PM, Taylor Blau wrote:\n> > On Tue, Apr 14, 2020 at 04:31:19PM -0400, Derrick Stolee wrote:\n> >> On 4/14/2020 4:22 PM, Elijah Newren wrote:\n> >>> Hi,\n> >>>\n> >>> I was building a version of git for internal use, and thought I'd try\n> >>> turning on features.experimental to get more testing of it.  The\n> >>> following test error in the testsuite scared me, though:\n> >>>\n> >>> t5537.9 (fetch --update-shallow):\n> >>>\n> >>> ...\n> >>> + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> [snip]\n> >> Well, commit-graphs are not supposed to do anything if we have\n> >> shallow clones. We definitely don't load a commit-graph in that\n> >> case. Seems like we need an extra check in write_commit_graph()\n> >> to stop writing in the presence of shallow commits.\n>\n> Here, I assumed that the commit-graph wasn't checking the shallow status\n> appropriately, but...\n>\n> > This rang a bell to me, too. There's a bug, but it's due to the mutative\n> > side-effects of 'is_repository_shallow' along with '--update-shallow' (a\n> > normal 'git fetch' works fine here, with or without\n> > fetch.writeCommitGraph).\n>\n> ...this makes sense as to why this particular case failed.\n>\n> (Please allow me this brief moment to communicate my extreme dislike\n> of shallow clones. There, I'm done.)\n\nNoted ;).\n\n> > Here's a patch that I didn't sign-off on that fixes the problem for me.\n> >\n> > --- >8 ---\n> >\n> > Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate\n> >\n> > In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,\n> > 2019-01-10), the author noted that 'is_repository_shallow' produces\n> > visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.\n> >\n> > This is a problem for e.g., fetching with '--update-shallow' in a\n> > shallow repsoitory with 'fetch.writeCommitGraph' enabled, since the\n>\n> repository\n>\n> > update to '.git/shallow' will cause Git to think that the repository\n> > *isn't* shallow when it is, thereby circumventing the commit-graph\n> > compatability check.\n>\n> compatibility\n\nWhoops, thanks for pointing these out.\n\n> > This causes problems in shallow repositories with at least shallow refs\n> > that have at least one ancestor (since the client won't have those\n> > object(s), and therefore can't take the reachability closure over\n> > commits to be written to the commit-graph).\n> >\n> > Address this by introducing 'reset_repository_shallow()', and calling it\n> > when the shallow file is updated, forcing 'is_repository_shallow' to\n> > re-evaluate whether the repository is still shallow after fetching in\n> > the above scenario.\n> >\n> > Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> > ---\n> >  commit.h     |  1 +\n> >  fetch-pack.c |  1 +\n> >  shallow.c    | 15 ++++++++-------\n> >  3 files changed, 10 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/commit.h b/commit.h\n> > index 008a0fa4a0..ee1ba139d4 100644\n> > --- a/commit.h\n> > +++ b/commit.h\n> > @@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid);\n> >  int unregister_shallow(const struct object_id *oid);\n> >  int for_each_commit_graft(each_commit_graft_fn, void *);\n> >  int is_repository_shallow(struct repository *r);\n> > +void reset_repository_shallow(struct repository *r);\n> >  struct commit_list *get_shallow_commits(struct object_array *heads,\n> >  \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n> >  struct commit_list *get_shallow_commits_by_rev_list(\n> > diff --git a/fetch-pack.c b/fetch-pack.c\n> > index 1734a573b0..051902ef6d 100644\n> > --- a/fetch-pack.c\n> > +++ b/fetch-pack.c\n> > @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args,\n> >  \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n> >  \t\t\tunlink_or_warn(git_path_shallow(the_repository));\n> >  \t\t\trollback_lock_file(&shallow_lock);\n> > +\t\t\treset_repository_shallow(the_repository);\n> >  \t\t} else\n> >  \t\t\tcommit_lock_file(&shallow_lock);\n> >  \t\talternate_shallow_file = NULL;\n> > diff --git a/shallow.c b/shallow.c\n> > index 7fd04afed1..fac383dec9 100644\n> > --- a/shallow.c\n> > +++ b/shallow.c\n> > @@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n> >\n> >  int is_repository_shallow(struct repository *r)\n> >  {\n> > -\t/*\n> > -\t * NEEDSWORK: This function updates\n> > -\t * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but\n> > -\t * there is no corresponding function to clear them when the shallow\n> > -\t * file is updated.\n> > -\t */\n> > -\n>\n> It's always good to remove these NEEDSWORK comments. I wonder if the\n> problem is more complicated than your patch makes it seem, or else\n> the original author would have done it correctly at first.\n>\n> But you are definitely closing out one dangling side-effect, which is\n> an improvement.\n\nYeah, I have no idea if there are other spots that I'm missing (I only\nspent a few minutes on this patch before leaving for the day to go eat\ndinner), hence why I CC'd Jonathan Tan to see if he had anything else in\nmind when he wrote this 'NEEDSWORK' comment.\n\nIf he feels that this is a generally good direction, I'm more than happy\nto look for other spots myself, address them, and then submit this as a\nreal patch.\n\n> >  \tFILE *fp;\n> >  \tchar buf[1024];\n> >  \tconst char *path = r->parsed_objects->alternate_shallow_file;\n> > @@ -79,6 +72,12 @@ int is_repository_shallow(struct repository *r)\n> >  \treturn r->parsed_objects->is_shallow;\n> >  }\n> >\n> > +void reset_repository_shallow(struct repository *r)\n> > +{\n> > +\tr->parsed_objects->is_shallow = -1;\n> > +\tstat_validity_clear(r->parsed_objects->shallow_stat);\n> > +}\n> > +\n> >  /*\n> >   * TODO: use \"int\" elemtype instead of \"int *\" when/if commit-slab\n> >   * supports a \"valid\" flag.\n> > @@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n> >  \t\t * shallow file\".\n> >  \t\t */\n> >  \t\t*alternate_shallow_file = \"\";\n> > +\treset_repository_shallow(the_repository);\n> >  \tstrbuf_release(&sb);\n> >  }\n> >\n> > @@ -411,6 +411,7 @@ void prune_shallow(unsigned options)\n> >  \t\t\tdie_errno(\"failed to write to %s\",\n> >  \t\t\t\t  get_lock_file_path(&shallow_lock));\n> >  \t\tcommit_lock_file(&shallow_lock);\n> > +\t\treset_repository_shallow(the_repository);\n> >  \t} else {\n>\n> These are likely good places to call reset_repository_shallow(),\n> but perhaps we should also call it here in commit-graph.c?\n>\n>  static int commit_graph_compatible(struct repository *r)\n>  {\n>         if (!r->gitdir)\n>                 return 0;\n>\n>         if (read_replace_refs) {\n>                 prepare_replace_object(r);\n>                 if (hashmap_get_size(&r->objects->replace_map->map))\n>                         return 0;\n>         }\n>\n>         prepare_commit_graft(r);\n>         if (r->parsed_objects && r->parsed_objects->grafts_nr)\n>                 return 0;\n> +       reset_repository_shallow(r);\n>         if (is_repository_shallow(r))\n>                 return 0;\n>\n>         return 1;\n>  }\n>\n> Or am I misunderstanding the state that reset_repository_shallow()\n> puts us in? My expectation is that is_repository_shallow() will\n> act as if the shallow variables had never been set and will look\n> for shallow data from disk.\n\nYou're not misunderstanding it at all. I don't think that this location\nis strictly necessary, since all of the other locations that change\n'.git/shallow' are already invaliding the shallow-ness checks, so by the\ntime we get to this point 'is_repository_shallow' has to take the slow\npath and redetermine whether or not we are actually shallow.\n\nSo, I don't think this location is strictly necessary, and it's\npotentially slowing us down a little bit, but it is hardening us against\nother parts of the code that may not call 'reset_repository_shallow'\nwhen they should have.\n\n> Thanks,\n> -Stolee\n\nThanks for the review.\n\nThanks,\nTaylor\n"},{"id":"395522","messageId":"CABPp-BExGQXERFFB0NPk=P12rEzJ=-NvJVZyG-R3S0ziOBdzBQ@mail.gmail.com","threadId":"53231","inReplyTo":"cdc80c0b-696f-19b8-d132-592782537b03@gmail.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-15T18:07:20Z","receivedAt":"2020-04-15T18:07:37Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 15, 2020 at 4:55 AM Derrick Stolee <stolee@gmail.com> wrote:\n\n> (Please allow me this brief moment to communicate my extreme dislike\n> of shallow clones. There, I'm done.)\n\nI share your extreme dislike, but I've merely succeeded in\ndiscouraging some people from using them, not in stopping their use\naltogether.\n\n\n(I don't have anything to add to the review of the patches, other than\nI'm happy to see folks trying to address the issue.)\n"},{"id":"395536","messageId":"20200415205442.GC216285@google.com","threadId":"53231","inReplyTo":"CABPp-BHGubUX5o9KsQaoh_UFjFh2PaMkkJhCao+5LGnFc0dQNg@mail.gmail.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-15T20:54:42Z","receivedAt":"2020-04-15T20:54:48Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nElijah Newren wrote:\n\n> I was building a version of git for internal use, and thought I'd try\n> turning on features.experimental to get more testing of it.  The\n> following test error in the testsuite scared me, though:\n>\n> t5537.9 (fetch --update-shallow):\n>\n> ...\n> + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> remote: Enumerating objects: 18, done.\n> remote: Counting objects: 100% (18/18), done.\n> remote: Compressing objects: 100% (6/6), done.\n> remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\n> Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\n> From ../shallow/\n>  * [new branch]      master     -> shallow/master\n>  * [new tag]         heavy-tag  -> heavy-tag\n>  * [new tag]         light-tag  -> light-tag\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n>\n> Passing -c fetch.writeCommitGraph=false to the fetch command in that\n> test makes it pass.\n\nOh!  Thanks for checking this.  At $DAYJOB this was the week we were\ngoing to roll out features.experimental.  Time to roll that back...\n\nHow did you go about the experiment?  Does Taylor's patch make it pass?\n\n(I'm thinking it would be nice to have a\nGIT_TEST_EXPERIMENTAL_FEATURES setting that uses test_config_global at\ntest init time.)\n\nThanks,\nJonathan\n"},{"id":"395548","messageId":"CABPp-BE+bC8aECT4Ak_kSObXV-euq7vCnYeUYBEjYmW4Pko=rw@mail.gmail.com","threadId":"53231","inReplyTo":"20200415205442.GC216285@google.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-15T22:54:09Z","receivedAt":"2020-04-15T22:54:28Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 15, 2020 at 1:54 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Hi,\n>\n> Elijah Newren wrote:\n>\n> > I was building a version of git for internal use, and thought I'd try\n> > turning on features.experimental to get more testing of it.  The\n> > following test error in the testsuite scared me, though:\n> >\n> > t5537.9 (fetch --update-shallow):\n> >\n> > ...\n> > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> > remote: Enumerating objects: 18, done.\n> > remote: Counting objects: 100% (18/18), done.\n> > remote: Compressing objects: 100% (6/6), done.\n> > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\n> > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\n> > From ../shallow/\n> >  * [new branch]      master     -> shallow/master\n> >  * [new tag]         heavy-tag  -> heavy-tag\n> >  * [new tag]         light-tag  -> light-tag\n> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n> >\n> > Passing -c fetch.writeCommitGraph=false to the fetch command in that\n> > test makes it pass.\n>\n> Oh!  Thanks for checking this.  At $DAYJOB this was the week we were\n> going to roll out features.experimental.  Time to roll that back...\n>\n> How did you go about the experiment?  Does Taylor's patch make it pass?\n\nYes, Taylor's patch makes the experiment pass.  My experiment was\npretty simple; modify the code so that features.experimental defaults\nto true:\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex a703e407a3..b39bc21b99 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -8,6 +8,7 @@ void prepare_repo_settings(struct repository *r)\n {\n        int value;\n        char *strval;\n+       int feature_experimental;\n\n        if (r->settings.initialized)\n                return;\n@@ -51,7 +54,10 @@ void prepare_repo_settings(struct repository *r)\n        }\n        if (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n                r->settings.fetch_write_commit_graph = value;\n-       if (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n+       feature_experimental = 1;\n+       if (!repo_config_get_bool(r, \"feature.experimental\", &value))\n+               feature_experimental = value;\n+       if (feature_experimental) {\n                UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n\nUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm,\nFETCH_NEGOTIATION_SKIPPING);\n                UPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n\n(based on 2.26.0; Stolee has since made pack_use_sparse default to\ntrue anyway) and then run the test suite and see what breaks.  For the\ntests that fail, check if the failure is expected based on different\ndefaults and the code still functions correctly with the new defaults\n(and update the test manually if so), or whether the test failure\nrepresents some potentially scary problem.  This was the only bug\nexposed by this little experiment.\n\n> (I'm thinking it would be nice to have a\n> GIT_TEST_EXPERIMENTAL_FEATURES setting that uses test_config_global at\n> test init time.)\n"},{"id":"395554","messageId":"20200416004734.GA36156@syl.local","threadId":"53231","inReplyTo":"CABPp-BE+bC8aECT4Ak_kSObXV-euq7vCnYeUYBEjYmW4Pko=rw@mail.gmail.com","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-16T00:47:34Z","receivedAt":"2020-04-16T00:47:52Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 15, 2020 at 03:54:09PM -0700, Elijah Newren wrote:\n> On Wed, Apr 15, 2020 at 1:54 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n> >\n> > Hi,\n> >\n> > Elijah Newren wrote:\n> >\n> > > I was building a version of git for internal use, and thought I'd try\n> > > turning on features.experimental to get more testing of it.  The\n> > > following test error in the testsuite scared me, though:\n> > >\n> > > t5537.9 (fetch --update-shallow):\n> > >\n> > > ...\n> > > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> > > remote: Enumerating objects: 18, done.\n> > > remote: Counting objects: 100% (18/18), done.\n> > > remote: Compressing objects: 100% (6/6), done.\n> > > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0\n> > > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.\n> > > From ../shallow/\n> > >  * [new branch]      master     -> shallow/master\n> > >  * [new tag]         heavy-tag  -> heavy-tag\n> > >  * [new tag]         light-tag  -> light-tag\n> > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5\n> > > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5\n> > >\n> > > Passing -c fetch.writeCommitGraph=false to the fetch command in that\n> > > test makes it pass.\n> >\n> > Oh!  Thanks for checking this.  At $DAYJOB this was the week we were\n> > going to roll out features.experimental.  Time to roll that back...\n> >\n> > How did you go about the experiment?  Does Taylor's patch make it pass?\n>\n> Yes, Taylor's patch makes the experiment pass.  My experiment was\n> pretty simple; modify the code so that features.experimental defaults\n> to true:\n\nI'm glad to hear that it is passing now. I would like to have Jonathan\nTan chime in on whether or not this patch is making sense, but if so,\nI'll prepare it for real and send it out.\n\nThanks,\nTaylor\n"},{"id":"395563","messageId":"20200416020509.225014-1-jonathantanmy@google.com","threadId":"53231","inReplyTo":"20200414235057.GA6863@syl.local","subject":"Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-16T02:05:09Z","receivedAt":"2020-04-16T02:05:33Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Here's a patch that I didn't sign-off on that fixes the problem for me.\n> \n> --- >8 ---\n> \n> Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate\n> \n> In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,\n> 2019-01-10), the author noted that 'is_repository_shallow' produces\n> visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.\n\nThanks for the reference to my commit. When I wrote that, if I remember\ncorrectly, I found it difficult to be able to guarantee that the\nclearing of is_shallow and shallow_stat was performed upon every\ncommit-lock (which creates a new shallow file) and\n<unlink+rollback-lock> (which removes the shallow file, potentially\nchanging the repo state from shallow to none), so I added the NEEDSWORK\ninstead. I see that this patch makes some progress in solving that.\n\n> @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args,\n>  \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n>  \t\t\tunlink_or_warn(git_path_shallow(the_repository));\n>  \t\t\trollback_lock_file(&shallow_lock);\n> +\t\t\treset_repository_shallow(the_repository);\n>  \t\t} else\n>  \t\t\tcommit_lock_file(&shallow_lock);\n>  \t\talternate_shallow_file = NULL;\n\nHere, do you need a reset in the \"else\" branch as well? (I.e. in both\nbranches, so you can put it outside the \"if\" block.) I didn't look at it\ntoo closely, but I would think that when the shallow_lock file is\ncommitted, there might have been no shallow beforehand, changing the\nstate from no-shallow to shallow.\n\n> @@ -411,6 +411,7 @@ void prune_shallow(unsigned options)\n>  \t\t\tdie_errno(\"failed to write to %s\",\n>  \t\t\t\t  get_lock_file_path(&shallow_lock));\n>  \t\tcommit_lock_file(&shallow_lock);\n> +\t\treset_repository_shallow(the_repository);\n>  \t} else {\n>  \t\tunlink(git_path_shallow(the_repository));\n>  \t\trollback_lock_file(&shallow_lock);\n\nAnd the opposite case here - reset in the \"else\" branch because the\nstate could have changed from shallow to no-shallow.\n\nIn any case, I think the commit message should discuss why\nreset_repository_shallow() is added only on the unlink+rollback side in\none \"if\" statement, but only on the opposite \"commit\" side in another\n\"if\" statement.\n"}]}