{"thread":{"id":"53278","subject":"[PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","startedAt":"2020-04-21T18:09:25Z","lastAt":"2020-06-04T17:45:48Z","messageCount":32,"participants":["Taylor Blau","Junio C Hamano","Jonathan Tan","Jonathan Nieder","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395841","messageId":"8d295389ea43c6b7e008514067b7af6eacba64a5.1587492422.git.me@ttaylorr.com","threadId":"53278","inReplyTo":null,"subject":"[PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-21T18:09:18Z","receivedAt":"2020-04-21T18:09:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 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\nisn'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\nobjects, and therefore can't take the reachability closure over commits\nwhen writing a commit-graph).\n\nAddress this by introducing 'reset_repository_shallow()', and calling\nit whenever the shallow files is updated. This happens in two cases:\n\n * during 'update_shallow', when either the repository is\n un-shallowing, or after commit_lock_file, when the contents of\n .git/shallow is changing, and\n\n * in 'prune_shallow', when the repository can go from shallow to\n un-shallow when the shallow file is updated, forcing\n 'is_repository_shallow' to re-evaluate whether the repository is\n still shallow after fetching in the above scenario.\n\nAs a result of the second change, 'prune_shallow' can now only be called\nonce (since 'check_shallow_file_for_update' will die after calling\n'reset_repository_shallow'). But, this is OK since we only call\n'prune_shallow' at most once per process.\n\nHelped-by: Jonathan Tan \nSigned-off-by: Taylor Blau \n---\n\nHere's a cleaned up version of the patch that we were originally\ndiscussing in\nhttps://lore.kernel.org/git/20200414235057.GA6863@syl.local/, which\naddresses some of Jonathan's feedback and adds a test to make sure that\nthe new behavior is working correctly.\n\n commit.h | 1 +\n fetch-pack.c | 1 +\n shallow.c | 15 ++++++++-------\n t/t5537-fetch-shallow.sh | 36 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 46 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..684868bc17 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1632,6 +1632,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \t\t\trollback_lock_file(&shallow_lock);\n \t\t} else\n \t\t\tcommit_lock_file(&shallow_lock);\n+\t\treset_repository_shallow(the_repository);\n \t\talternate_shallow_file = NULL;\n \t\treturn;\n \t}\ndiff --git a/shallow.c b/shallow.c\nindex 7fd04afed1..9d1304e786 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@@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n \t} else {\n \t\tunlink(git_path_shallow(the_repository));\n \t\trollback_lock_file(&shallow_lock);\n+\t\treset_repository_shallow(the_repository);\n \t}\n \tstrbuf_release(&sb);\n }\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 4f681dbbe1..c9c731c7a9 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -177,6 +177,42 @@ EOF\n \t)\n '\n\n+test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '\n+\t(\n+\tcd shallow &&\n+\tgit checkout master &&\n+\tcommit 8 &&\n+\tgit tag -m foo heavy-tag-for-graph HEAD^ &&\n+\tgit tag light-tag-for-graph HEAD^:tracked\n+\t) &&\n+\t(\n+\tcd notshallow &&\n+\ttest_config fetch.writeCommitGraph true &&\n+\tgit fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n+\tgit fsck &&\n+\tgit for-each-ref --sort=refname --format=\"%(refname)\" >actual.refs &&\n+\tcat <expect.refs &&\n+refs/remotes/shallow/master\n+refs/remotes/shallow/no-shallow\n+refs/tags/heavy-tag\n+refs/tags/heavy-tag-for-graph\n+refs/tags/light-tag\n+refs/tags/light-tag-for-graph\n+EOF\n+\ttest_cmp expect.refs actual.refs &&\n+\tgit log --format=%s shallow/master >actual &&\n+\tcat <expect &&\n+8\n+7\n+6\n+5\n+4\n+3\n+EOF\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n \tcp -R .git read-only.git &&\n \ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n--\n2.26.2.108.g048abe1751\n"},{"id":"395855","messageId":"xmqqeesgmuzv.fsf@gitster.c.googlers.com","threadId":"53278","inReplyTo":"8d295389ea43c6b7e008514067b7af6eacba64a5.1587492422.git.me@ttaylorr.com","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-21T20:41:56Z","receivedAt":"2020-04-21T20:42:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\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>\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> objects, and therefore can't take the reachability closure over commits\n> when writing a commit-graph).\n\nOK.\n\n> Address this by introducing 'reset_repository_shallow()', and calling\n> it whenever the shallow files is updated. This happens in two cases:\n>\n> * during 'update_shallow', when either the repository is\n> un-shallowing, or after commit_lock_file, when the contents of\n> .git/shallow is changing, and\n>\n> * in 'prune_shallow', when the repository can go from shallow to\n> un-shallow when the shallow file is updated, forcing\n> 'is_repository_shallow' to re-evaluate whether the repository is\n> still shallow after fetching in the above scenario.\n>\n> As a result of the second change, 'prune_shallow' can now only be called\n> once (since 'check_shallow_file_for_update' will die after calling\n> 'reset_repository_shallow'). But, this is OK since we only call\n> 'prune_shallow' at most once per process.\n>\n> Helped-by: Jonathan Tan \n> Signed-off-by: Taylor Blau \n> ---\n>\n> Here's a cleaned up version of the patch that we were originally\n> discussing in\n> https://lore.kernel.org/git/20200414235057.GA6863@syl.local/, which\n> addresses some of Jonathan's feedback and adds a test to make sure that\n> the new behavior is working correctly.\n>\n> commit.h | 1 +\n> fetch-pack.c | 1 +\n> shallow.c | 15 ++++++++-------\n> t/t5537-fetch-shallow.sh | 36 ++++++++++++++++++++++++++++++++++++\n> 4 files changed, 46 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..684868bc17 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1632,6 +1632,7 @@ static void update_shallow(struct fetch_pack_args *args,\n> \t\t\trollback_lock_file(&shallow_lock);\n> \t\t} else\n> \t\t\tcommit_lock_file(&shallow_lock);\n> +\t\treset_repository_shallow(the_repository);\n> \t\talternate_shallow_file = NULL;\n> \t\treturn;\n> \t}\n\nSo, after updating shallow file with \"fetch --update-shallow\" (or\nfailing to do so), we reset the in-core data.\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\nOK.\n\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\nAnd also after writing out the alternate shallow file (whether it is\nempty or polulated).\n\n> @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n> \t} else {\n> \t\tunlink(git_path_shallow(the_repository));\n> \t\trollback_lock_file(&shallow_lock);\n> +\t\treset_repository_shallow(the_repository);\n> \t}\n\nHere, we reset only after we realize we cannot write the updated\nshallow file. Intended?\n\n> +\tcat <expect.refs &&\n> +refs/remotes/shallow/master\n> +refs/remotes/shallow/no-shallow\n> +refs/tags/heavy-tag\n> +refs/tags/heavy-tag-for-graph\n> +refs/tags/light-tag\n> +refs/tags/light-tag-for-graph\n> +EOF\n\n\tcat <<-EOF >expect.refs &&\n\t... body can be indented by any number of TAB\n\t... to make it easier to view\n\tEOF\n\n> +\ttest_cmp expect.refs actual.refs &&\n> +\tgit log --format=%s shallow/master >actual &&\n> +\tcat <expect &&\n\nLikewise.\n"},{"id":"395858","messageId":"20200421204516.GA69777@syl.local","threadId":"53278","inReplyTo":"xmqqeesgmuzv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-21T20:45:16Z","receivedAt":"2020-04-21T20:45:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Junio,\n\nOn Tue, Apr 21, 2020 at 01:41:56PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\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\nWhoops, sorry about that.\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> > 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> > objects, and therefore can't take the reachability closure over commits\n> > when writing a commit-graph).\n>\n> OK.\n>\n> > Address this by introducing 'reset_repository_shallow()', and calling\n> > it whenever the shallow files is updated. This happens in two cases:\n> >\n> > * during 'update_shallow', when either the repository is\n> > un-shallowing, or after commit_lock_file, when the contents of\n> > .git/shallow is changing, and\n> >\n> > * in 'prune_shallow', when the repository can go from shallow to\n> > un-shallow when the shallow file is updated, forcing\n> > 'is_repository_shallow' to re-evaluate whether the repository is\n> > still shallow after fetching in the above scenario.\n> >\n> > As a result of the second change, 'prune_shallow' can now only be called\n> > once (since 'check_shallow_file_for_update' will die after calling\n> > 'reset_repository_shallow'). But, this is OK since we only call\n> > 'prune_shallow' at most once per process.\n> >\n> > Helped-by: Jonathan Tan \n> > Signed-off-by: Taylor Blau \n> > ---\n> >\n> > Here's a cleaned up version of the patch that we were originally\n> > discussing in\n> > https://lore.kernel.org/git/20200414235057.GA6863@syl.local/, which\n> > addresses some of Jonathan's feedback and adds a test to make sure that\n> > the new behavior is working correctly.\n> >\n> > commit.h | 1 +\n> > fetch-pack.c | 1 +\n> > shallow.c | 15 ++++++++-------\n> > t/t5537-fetch-shallow.sh | 36 ++++++++++++++++++++++++++++++++++++\n> > 4 files changed, 46 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..684868bc17 100644\n> > --- a/fetch-pack.c\n> > +++ b/fetch-pack.c\n> > @@ -1632,6 +1632,7 @@ static void update_shallow(struct fetch_pack_args *args,\n> > \t\t\trollback_lock_file(&shallow_lock);\n> > \t\t} else\n> > \t\t\tcommit_lock_file(&shallow_lock);\n> > +\t\treset_repository_shallow(the_repository);\n> > \t\talternate_shallow_file = NULL;\n> > \t\treturn;\n> > \t}\n>\n> So, after updating shallow file with \"fetch --update-shallow\" (or\n> failing to do so), we reset the in-core data.\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> OK.\n>\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> And also after writing out the alternate shallow file (whether it is\n> empty or polulated).\n>\n> > @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n> > \t} else {\n> > \t\tunlink(git_path_shallow(the_repository));\n> > \t\trollback_lock_file(&shallow_lock);\n> > +\t\treset_repository_shallow(the_repository);\n> > \t}\n>\n> Here, we reset only after we realize we cannot write the updated\n> shallow file. Intended?\n\nYes, see this earlier discussion I had about it with Jonathan:\nhttps://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/.\n\n> > +\tcat <expect.refs &&\n> > +refs/remotes/shallow/master\n> > +refs/remotes/shallow/no-shallow\n> > +refs/tags/heavy-tag\n> > +refs/tags/heavy-tag-for-graph\n> > +refs/tags/light-tag\n> > +refs/tags/light-tag-for-graph\n> > +EOF\n>\n> \tcat <<-EOF >expect.refs &&\n> \t... body can be indented by any number of TAB\n> \t... to make it easier to view\n> \tEOF\n>\n> > +\ttest_cmp expect.refs actual.refs &&\n> > +\tgit log --format=%s shallow/master >actual &&\n> > +\tcat <expect &&\n>\n> Likewise.\n\nI'd be happy to update these, but I am matching the (poor) style of the\nsurrounding tests. Are you OK with the inconsistency? Would you like\nanother preparatory patch to clean up the surrounding?\n\nThanks,\nTaylor\n"},{"id":"395859","messageId":"xmqq5zdsmuht.fsf@gitster.c.googlers.com","threadId":"53278","inReplyTo":"20200421204516.GA69777@syl.local","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-21T20:52:46Z","receivedAt":"2020-04-21T20:52:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> > @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n>> > \t} else {\n>> > \t\tunlink(git_path_shallow(the_repository));\n>> > \t\trollback_lock_file(&shallow_lock);\n>> > +\t\treset_repository_shallow(the_repository);\n>> > \t}\n>>\n>> Here, we reset only after we realize we cannot write the updated\n>> shallow file. Intended?\n>\n> Yes, see this earlier discussion I had about it with Jonathan:\n> https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/.\n\nI did, and then I asked the question, because I couldn't quite get\nif JTan was asking a question similar to the one he asked earlier in\nthe message (\"do you need a reset in the \"else\" branch as well?\"),\nor if he was saying what he sees there, \"the opposite case\", was\ngood.\n\nAlso, I was sort-of reacting to \"\"\"In any case, I think the commit\nmessage should discuss why reset_repository_shallow() is added only\non the unlink+rollback side in one \"if\" statement, but only on the\nopposite \"commit\" side in another \"if\" statement.\"\"\" in that\nmessage.\n\nThanks.\n"},{"id":"395868","messageId":"20200421222128.GA74697@syl.local","threadId":"53278","inReplyTo":"xmqq5zdsmuht.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-21T22:21:28Z","receivedAt":"2020-04-21T22:21:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 21, 2020 at 01:52:46PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> > @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n> >> > \t} else {\n> >> > \t\tunlink(git_path_shallow(the_repository));\n> >> > \t\trollback_lock_file(&shallow_lock);\n> >> > +\t\treset_repository_shallow(the_repository);\n> >> > \t}\n> >>\n> >> Here, we reset only after we realize we cannot write the updated\n> >> shallow file. Intended?\n> >\n> > Yes, see this earlier discussion I had about it with Jonathan:\n> > https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/.\n>\n> I did, and then I asked the question, because I couldn't quite get\n> if JTan was asking a question similar to the one he asked earlier in\n> the message (\"do you need a reset in the \"else\" branch as well?\"),\n> or if he was saying what he sees there, \"the opposite case\", was\n> good.\n\nSorry, I think that the linked message is confusing (at least, it\nconfused me the second time I read it because I wasn't sure which part\nof the mail Jonathan was asking about: my patch, or his response to my\npatch).\n\nI think that he was referring to how I had it in the original patch I\nsent in that thread, which was wrong. Based on my understanding of his\nmessage, his recommendations match what I have in _this_ patch.\n\n> Also, I was sort-of reacting to \"\"\"In any case, I think the commit\n> message should discuss why reset_repository_shallow() is added only\n> on the unlink+rollback side in one \"if\" statement, but only on the\n> opposite \"commit\" side in another \"if\" statement.\"\"\" in that\n> message.\n\nIs the description that I attached in the earlier patch sufficient? I\ncould certainly add more detail if it's not.\n\nIn either case, I'm sitting on another patch locally to improve the\nstyle of the surrounding tests, which is done as a preparatory step\nbefore this patch. I'll re-send after hearing back from you.\n\n> Thanks.\n\nThanks,\nTaylor\n"},{"id":"395871","messageId":"xmqqh7xcl9qo.fsf@gitster.c.googlers.com","threadId":"53278","inReplyTo":"20200421222128.GA74697@syl.local","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-21T23:06:23Z","receivedAt":"2020-04-21T23:06:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Sorry, I think that the linked message is confusing (at least, it\n> confused me the second time I read it because I wasn't sure which part\n> of the mail Jonathan was asking about: my patch, or his response to my\n> patch).\n>\n> I think that he was referring to how I had it in the original patch I\n> sent in that thread, which was wrong. Based on my understanding of his\n> message, his recommendations match what I have in _this_ patch.\n\nThanks for a clarification.\n\n> In either case, I'm sitting on another patch locally to improve the\n> style of the surrounding tests, which is done as a preparatory step\n> before this patch. I'll re-send after hearing back from you.\n\nAlright. Thanks.\n"},{"id":"395921","messageId":"20200422180245.196132-1-jonathantanmy@google.com","threadId":"53278","inReplyTo":"8d295389ea43c6b7e008514067b7af6eacba64a5.1587492422.git.me@ttaylorr.com","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-22T18:02:45Z","receivedAt":"2020-04-22T18:02:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Address this by introducing 'reset_repository_shallow()', and calling\n> it whenever the shallow files is updated. This happens in two cases:\n> \n> * during 'update_shallow', when either the repository is\n> un-shallowing, or after commit_lock_file, when the contents of\n> .git/shallow is changing, and\n> \n> * in 'prune_shallow', when the repository can go from shallow to\n> un-shallow when the shallow file is updated, forcing\n> 'is_repository_shallow' to re-evaluate whether the repository is\n> still shallow after fetching in the above scenario.\n\nFrom a cursory reading of the code, it seems that this happens in\nfetch-pack and receive-pack. Looking at those files, I found some more\noccasions when this happens. I have outlined them in the patch after the\nscissors (I hope I used the scissors correctly).\n\nMaybe instead of enumerating the cases (of which there are quite a few),\nsay that we do this when we unlink the shallow file, we modify or create\nthe shallow file, or when we change the value of alternate_shallow_file.\n\n> @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n> \t} else {\n> \t\tunlink(git_path_shallow(the_repository));\n> \t\trollback_lock_file(&shallow_lock);\n> +\t\treset_repository_shallow(the_repository);\n> \t}\n> \tstrbuf_release(&sb);\n> }\n\nThe \"if\" part (not quoted here) commits the shallow lock file, and thus\npossibly modifies (or creates) the shallow file, so I think we need to\nput reset_repository_shallow() outside the whole \"if\" block. I have done\nthat in the patch after the scissors.\n\n> +test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '\n> +\t(\n> +\tcd shallow &&\n> +\tgit checkout master &&\n> +\tcommit 8 &&\n> +\tgit tag -m foo heavy-tag-for-graph HEAD^ &&\n> +\tgit tag light-tag-for-graph HEAD^:tracked\n> +\t) &&\n> +\t(\n> +\tcd notshallow &&\n> +\ttest_config fetch.writeCommitGraph true &&\n\nWhen I patched onto master, this line causes the test to fail with a\nwarning that test_when_finished doesn't work in a subshell. I've\nreplaced it with a regular \"git config\" and it works.\n\nHere is the patch containing what I tried. I think that most of the new\nreset_repository_shallow() invocations don't change any functionality\n(we don't usually read shallow files so many times in a process), so\nthey can't be tested, but I think that it's better to include them for\ncompleteness, and to close the open question mentioned in bd0b42aed3\n(\"fetch-pack: do not take shallow lock unnecessarily\", 2019-01-10)\n(about the full solution involving clearing shallow information whenever\nwe commit the shallow lock - we find here that the full solution\ninvolves this and also clearing shallow information in other cases too).\n\n-- 8< --\nFrom 46a69a133db2e8e948d2bf296294656c9902e5ae Mon Sep 17 00:00:00 2001\nFrom: Jonathan Tan \nDate: Wed, 22 Apr 2020 10:53:30 -0700\nSubject: [PATCH] fixup\n\nSigned-off-by: Jonathan Tan \n---\n builtin/receive-pack.c | 1 +\n fetch-pack.c | 2 ++\n shallow.c | 2 +-\n t/t5537-fetch-shallow.sh | 2 +-\n 4 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2cc18bbffd..d61cbf60e2 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -878,6 +878,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \t}\n \n \tcommit_lock_file(&shallow_lock);\n+\treset_repository_shallow(the_repository);\n \n \t/*\n \t * Make sure setup_alternate_shallow() for the next ref does\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 684868bc17..9a1cec470c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1657,6 +1657,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \t\t\t\t\t\t&alternate_shallow_file,\n \t\t\t\t\t\t&extra);\n \t\t\tcommit_lock_file(&shallow_lock);\n+\t\t\treset_repository_shallow(the_repository);\n \t\t\talternate_shallow_file = NULL;\n \t\t}\n \t\toid_array_clear(&extra);\n@@ -1695,6 +1696,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \t\t\t\t\t&alternate_shallow_file,\n \t\t\t\t\t&extra);\n \t\tcommit_lock_file(&shallow_lock);\n+\t\treset_repository_shallow(the_repository);\n \t\toid_array_clear(&extra);\n \t\toid_array_clear(&ref);\n \t\talternate_shallow_file = NULL;\ndiff --git a/shallow.c b/shallow.c\nindex 9d1304e786..1a1ca71ffe 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -414,8 +414,8 @@ void prune_shallow(unsigned options)\n \t} else {\n \t\tunlink(git_path_shallow(the_repository));\n \t\trollback_lock_file(&shallow_lock);\n-\t\treset_repository_shallow(the_repository);\n \t}\n+\treset_repository_shallow(the_repository);\n \tstrbuf_release(&sb);\n }\n \ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex c9c731c7a9..c5c40fb8e7 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -187,7 +187,7 @@ test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '\n \t) &&\n \t(\n \tcd notshallow &&\n-\ttest_config fetch.writeCommitGraph true &&\n+\tgit config fetch.writeCommitGraph true &&\n \tgit fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n \tgit fsck &&\n \tgit for-each-ref --sort=refname --format=\"%(refname)\" >actual.refs &&\n-- \n2.26.1.301.g55bc3eb7cb9-goog\n\n"},{"id":"395922","messageId":"20200422180515.197584-1-jonathantanmy@google.com","threadId":"53278","inReplyTo":"xmqq5zdsmuht.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-22T18:05:15Z","receivedAt":"2020-04-22T18:05:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Taylor Blau writes:\n> \n> >> > @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n> >> > \t} else {\n> >> > \t\tunlink(git_path_shallow(the_repository));\n> >> > \t\trollback_lock_file(&shallow_lock);\n> >> > +\t\treset_repository_shallow(the_repository);\n> >> > \t}\n> >>\n> >> Here, we reset only after we realize we cannot write the updated\n> >> shallow file. Intended?\n> >\n> > Yes, see this earlier discussion I had about it with Jonathan:\n> > https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/.\n> \n> I did, and then I asked the question, because I couldn't quite get\n> if JTan was asking a question similar to the one he asked earlier in\n> the message (\"do you need a reset in the \"else\" branch as well?\"),\n> or if he was saying what he sees there, \"the opposite case\", was\n> good.\n\nSorry for not being clear. My intention was to ask a question similar to\nthe earlier one - in this case, and in the previous case, I think that\nthe reset should happen no matter whether we execute the \"if\" case or\nthe \"else\" case, so we should just put it right after the entire \"if\"\nstatement.\n"},{"id":"395923","messageId":"xmqqy2qnidyy.fsf@gitster.c.googlers.com","threadId":"53278","inReplyTo":"20200422180245.196132-1-jonathantanmy@google.com","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-22T18:15:33Z","receivedAt":"2020-04-22T18:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n>> \t} else {\n>> \t\tunlink(git_path_shallow(the_repository));\n>> \t\trollback_lock_file(&shallow_lock);\n>> +\t\treset_repository_shallow(the_repository);\n>> \t}\n>> \tstrbuf_release(&sb);\n>> }\n>\n> The \"if\" part (not quoted here) commits the shallow lock file, and thus\n> possibly modifies (or creates) the shallow file, so I think we need to\n> put reset_repository_shallow() outside the whole \"if\" block. I have done\n> that in the patch after the scissors.\n\nIs there any rollback_lock_file() or commit_lock_file() call on the\nshallow lock file in the files involved in this patch that does not\nneed a call to reset_repository_shallow() left after your work?\n\nWhat I am trying to get at is if it would be safer to have a pair of\nthin wrapper for rolling back or committing a new version of new\nshallow file, e.g. rollback_shallow_file() + commit_shallow_file(),\nand replace calls to {rollback,commit}_lock_file() with calls to\nthem.\n"},{"id":"395968","messageId":"20200423001438.GC19100@syl.local","threadId":"53278","inReplyTo":"xmqqy2qnidyy.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T00:14:38Z","receivedAt":"2020-04-23T00:14:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 22, 2020 at 11:15:33AM -0700, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n> >> @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)\n> >> \t} else {\n> >> \t\tunlink(git_path_shallow(the_repository));\n> >> \t\trollback_lock_file(&shallow_lock);\n> >> +\t\treset_repository_shallow(the_repository);\n> >> \t}\n> >> \tstrbuf_release(&sb);\n> >> }\n> >\n> > The \"if\" part (not quoted here) commits the shallow lock file, and thus\n> > possibly modifies (or creates) the shallow file, so I think we need to\n> > put reset_repository_shallow() outside the whole \"if\" block. I have done\n> > that in the patch after the scissors.\n>\n> Is there any rollback_lock_file() or commit_lock_file() call on the\n> shallow lock file in the files involved in this patch that does not\n> need a call to reset_repository_shallow() left after your work?\n>\n> What I am trying to get at is if it would be safer to have a pair of\n> thin wrapper for rolling back or committing a new version of new\n> shallow file, e.g. rollback_shallow_file() + commit_shallow_file(),\n> and replace calls to {rollback,commit}_lock_file() with calls to\n> them.\n\nVery elegant. Thanks for an excellent suggestion. v2 incoming just as\nsoon as 'make test' finishes...\n\nThanks,\nTaylor\n"},{"id":"395969","messageId":"cover.1587601501.git.me@ttaylorr.com","threadId":"53278","inReplyTo":"20200423001438.GC19100@syl.local","subject":"[PATCH v2 0/2] shallow.c: reset shallow-ness after updating","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T00:25:36Z","receivedAt":"2020-04-23T00:25:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere is an updated version of the patch that I sent above (which was\nitself an updated version of a patch that I sent to some other\nsub-thread...).\n\nIn this version, I added an extra patch at the beginning to clean up\nsome surrounding tests in t5537 so the new ones are added both in a good\nstyle, and in a way that is consistent with the surrounding code.\n\nI also adopted a suggestion from Junio that introduces\n'{commit,rollback}_shallow_file()' as thin wrappers around\n'{commit,rollback}_lock_file()', which makes this whole thing a lot\ntidier.\n\nThanks in advance for another look...\n\nTaylor Blau (2):\n t5537: use test_write_lines, indented heredocs for readability\n shallow.c: use '{commit,rollback}_shallow_file'\n\n builtin/receive-pack.c | 4 +-\n commit.h | 2 +\n fetch-pack.c | 10 ++--\n shallow.c | 30 ++++++++----\n t/t5537-fetch-shallow.sh | 99 +++++++++++++++++++---------------------\n 5 files changed, 77 insertions(+), 68 deletions(-)\n\n--\n2.26.0.113.ge9739cdccc\n"},{"id":"395970","messageId":"5c9217ad8fc594fbff46507c4be7961eb5a478e2.1587601501.git.me@ttaylorr.com","threadId":"53278","inReplyTo":"cover.1587601501.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T00:25:41Z","receivedAt":"2020-04-23T00:25:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"A number of spots in t5537 use the non-indented heredoc '<\nSigned-off-by: Taylor Blau \n---\n t/t5537-fetch-shallow.sh | 70 +++++++++++-----------------------------\n 1 file changed, 18 insertions(+), 52 deletions(-)\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 4f681dbbe1..d66b3656c0 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -25,10 +25,7 @@ test_expect_success 'setup' '\n test_expect_success 'setup shallow clone' '\n \tgit clone --no-local --depth=2 .git shallow &&\n \tgit --git-dir=shallow/.git log --format=%s >actual &&\n-\tcat <expect &&\n-4\n-3\n-EOF\n+\ttest_write_lines 4 3 >expect &&\n \ttest_cmp expect actual\n '\n \n@@ -38,10 +35,7 @@ test_expect_success 'clone from shallow clone' '\n \tcd shallow2 &&\n \tgit fsck &&\n \tgit log --format=%s >actual &&\n-\tcat <expect &&\n-4\n-3\n-EOF\n+\ttest_write_lines 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -56,11 +50,7 @@ test_expect_success 'fetch from shallow clone' '\n \tgit fetch &&\n \tgit fsck &&\n \tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-5\n-4\n-3\n-EOF\n+\ttest_write_lines 5 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -75,10 +65,7 @@ test_expect_success 'fetch --depth from shallow clone' '\n \tgit fetch --depth=2 &&\n \tgit fsck &&\n \tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-6\n-5\n-EOF\n+\ttest_write_lines 6 5 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -89,12 +76,7 @@ test_expect_success 'fetch --unshallow from shallow clone' '\n \tgit fetch --unshallow &&\n \tgit fsck &&\n \tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-6\n-5\n-4\n-3\n-EOF\n+\ttest_write_lines 6 5 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -111,15 +93,10 @@ test_expect_success 'fetch something upstream has but hidden by clients shallow\n \tgit fetch ../.git +refs/heads/master:refs/remotes/top/master &&\n \tgit fsck &&\n \tgit log --format=%s top/master >actual &&\n-\tcat <expect &&\n-add-1-back\n-4\n-3\n-EOF\n+\ttest_write_lines add-1-back 4 3 >expect &&\n \ttest_cmp expect actual\n \t) &&\n \tgit --git-dir=shallow2/.git cat-file blob $(echo 1|git hash-object --stdin) >/dev/null\n-\n '\n \n test_expect_success 'fetch that requires changes in .git/shallow is filtered' '\n@@ -133,14 +110,12 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '\n \tcd notshallow &&\n \tgit fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&\n \tgit for-each-ref --format=\"%(refname)\" >actual.refs &&\n-\tcat <expect.refs &&\n-refs/remotes/shallow/no-shallow\n-EOF\n+\techo refs/remotes/shallow/no-shallow >expect.refs &&\n \ttest_cmp expect.refs actual.refs &&\n \tgit log --format=%s shallow/no-shallow >actual &&\n-\tcat <expect &&\n-no-shallow\n-EOF\n+\tcat <<-EOF >expect &&\n+\tno-shallow\n+\tEOF\n \ttest_cmp expect actual\n \t)\n '\n@@ -158,21 +133,15 @@ test_expect_success 'fetch --update-shallow' '\n \tgit fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n \tgit fsck &&\n \tgit for-each-ref --sort=refname --format=\"%(refname)\" >actual.refs &&\n-\tcat <expect.refs &&\n-refs/remotes/shallow/master\n-refs/remotes/shallow/no-shallow\n-refs/tags/heavy-tag\n-refs/tags/light-tag\n-EOF\n+\tcat <<-EOF >expect.refs &&\n+\trefs/remotes/shallow/master\n+\trefs/remotes/shallow/no-shallow\n+\trefs/tags/heavy-tag\n+\trefs/tags/light-tag\n+\tEOF\n \ttest_cmp expect.refs actual.refs &&\n \tgit log --format=%s shallow/master >actual &&\n-\tcat <expect &&\n-7\n-6\n-5\n-4\n-3\n-EOF\n+\ttest_write_lines 7 6 5 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -183,10 +152,7 @@ test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n \tfind read-only.git -print | xargs chmod -w &&\n \tgit clone --no-local --depth=2 read-only.git from-read-only &&\n \tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n-\tcat >expect <expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"395971","messageId":"296e70790d7a391d471554b0bc5a58e2a091ce88.1587601501.git.me@ttaylorr.com","threadId":"53278","inReplyTo":"cover.1587601501.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T00:25:45Z","receivedAt":"2020-04-23T00:25:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 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 repository with 'fetch.writeCommitGraph' enabled, since the\nupdate to '.git/shallow' will cause Git to think that the repository\nisn't shallow when it is, thereby circumventing the commit-graph\ncompatibility 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\nobjects, and therefore can't take the reachability closure over commits\nwhen writing a commit-graph).\n\nAddress this by introducing thin wrappers over 'commit_lock_file' and\n'rollback_lock_file' for use specifically when the lock is held over\n'.git/shallow'. These wrappers (appropriately called\n'commit_shallow_file' and 'rollback_shallow_file') call into their\nrespective functions in 'lockfile.h', but additionally reset validity\nchecks used by the shallow machinery.\n\nReplace each instance of 'commit_lock_file' and 'rollback_lock_file'\nwith 'commit_shallow_file' and 'rollback_shallow_file' when the lock\nbeing held is over the '.git/shallow' file.\n\nAs a result, 'prune_shallow' can now only be called once (since\n'check_shallow_file_for_update' will die after calling\n'reset_repository_shallow'). But, this is OK since we only call\n'prune_shallow' at most once per process.\n\nHelped-by: Jonathan Tan \nHelped-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n builtin/receive-pack.c | 4 ++--\n commit.h | 2 ++\n fetch-pack.c | 10 +++++-----\n shallow.c | 30 +++++++++++++++++++++---------\n t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++\n 5 files changed, 59 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2cc18bbffd..652661fa99 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -872,12 +872,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \topt.env = tmp_objdir_env(tmp_objdir);\n \tsetup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);\n \tif (check_connected(command_singleton_iterator, cmd, &opt)) {\n-\t\trollback_lock_file(&shallow_lock);\n+\t\trollback_shallow_file(the_repository, &shallow_lock);\n \t\toid_array_clear(&extra);\n \t\treturn -1;\n \t}\n \n-\tcommit_lock_file(&shallow_lock);\n+\tcommit_shallow_file(the_repository, &shallow_lock);\n \n \t/*\n \t * Make sure setup_alternate_shallow() for the next ref does\ndiff --git a/commit.h b/commit.h\nindex 008a0fa4a0..ab91d21131 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -249,6 +249,8 @@ struct oid_array;\n struct ref;\n int register_shallow(struct repository *r, const struct object_id *oid);\n int unregister_shallow(const struct object_id *oid);\n+int commit_shallow_file(struct repository *r, struct lock_file *lk);\n+void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n int for_each_commit_graft(each_commit_graft_fn, void *);\n int is_repository_shallow(struct repository *r);\n struct commit_list *get_shallow_commits(struct object_array *heads,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1734a573b0..a618f3b029 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1629,9 +1629,9 @@ static void update_shallow(struct fetch_pack_args *args,\n \tif (args->deepen && alternate_shallow_file) {\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\trollback_shallow_file(the_repository, &shallow_lock);\n \t\t} else\n-\t\t\tcommit_lock_file(&shallow_lock);\n+\t\t\tcommit_shallow_file(the_repository, &shallow_lock);\n \t\talternate_shallow_file = NULL;\n \t\treturn;\n \t}\n@@ -1655,7 +1655,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \t\t\tsetup_alternate_shallow(&shallow_lock,\n \t\t\t\t\t\t&alternate_shallow_file,\n \t\t\t\t\t\t&extra);\n-\t\t\tcommit_lock_file(&shallow_lock);\n+\t\t\tcommit_shallow_file(the_repository, &shallow_lock);\n \t\t\talternate_shallow_file = NULL;\n \t\t}\n \t\toid_array_clear(&extra);\n@@ -1693,7 +1693,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \t\tsetup_alternate_shallow(&shallow_lock,\n \t\t\t\t\t&alternate_shallow_file,\n \t\t\t\t\t&extra);\n-\t\tcommit_lock_file(&shallow_lock);\n+\t\tcommit_shallow_file(the_repository, &shallow_lock);\n \t\toid_array_clear(&extra);\n \t\toid_array_clear(&ref);\n \t\talternate_shallow_file = NULL;\n@@ -1785,7 +1785,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t\terror(_(\"remote did not send all necessary objects\"));\n \t\t\tfree_refs(ref_cpy);\n \t\t\tref_cpy = NULL;\n-\t\t\trollback_lock_file(&shallow_lock);\n+\t\t\trollback_shallow_file(the_repository, &shallow_lock);\n \t\t\tgoto cleanup;\n \t\t}\n \t\targs->connectivity_checked = 1;\ndiff --git a/shallow.c b/shallow.c\nindex 7fd04afed1..5010a6c732 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,25 @@ int is_repository_shallow(struct repository *r)\n \treturn r->parsed_objects->is_shallow;\n }\n \n+static 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+int commit_shallow_file(struct repository *r, struct lock_file *lk)\n+{\n+\tint res = commit_lock_file(lk);\n+\treset_repository_shallow(r);\n+\treturn res;\n+}\n+\n+void rollback_shallow_file(struct repository *r, struct lock_file *lk)\n+{\n+\trollback_lock_file(lk);\n+\treset_repository_shallow(r);\n+}\n+\n /*\n * TODO: use \"int\" elemtype instead of \"int *\" when/if commit-slab\n * supports a \"valid\" flag.\n@@ -410,10 +422,10 @@ void prune_shallow(unsigned options)\n \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\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\tcommit_shallow_file(the_repository, &shallow_lock);\n \t} else {\n \t\tunlink(git_path_shallow(the_repository));\n-\t\trollback_lock_file(&shallow_lock);\n+\t\trollback_shallow_file(the_repository, &shallow_lock);\n \t}\n \tstrbuf_release(&sb);\n }\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex d66b3656c0..a51c4b39d9 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -146,6 +146,35 @@ test_expect_success 'fetch --update-shallow' '\n \t)\n '\n \n+test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '\n+\t(\n+\tcd shallow &&\n+\tgit checkout master &&\n+\tcommit 8 &&\n+\tgit tag -m foo heavy-tag-for-graph HEAD^ &&\n+\tgit tag light-tag-for-graph HEAD^:tracked\n+\t) &&\n+\ttest_config -C notshallow fetch.writeCommitGraph true &&\n+\t(\n+\tcd notshallow &&\n+\tgit fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n+\tgit fsck &&\n+\tgit for-each-ref --sort=refname --format=\"%(refname)\" >actual.refs &&\n+\tcat <<-EOF >expect.refs &&\n+\trefs/remotes/shallow/master\n+\trefs/remotes/shallow/no-shallow\n+\trefs/tags/heavy-tag\n+\trefs/tags/heavy-tag-for-graph\n+\trefs/tags/light-tag\n+\trefs/tags/light-tag-for-graph\n+\tEOF\n+\ttest_cmp expect.refs actual.refs &&\n+\tgit log --format=%s shallow/master >actual &&\n+\ttest_write_lines 8 7 6 5 4 3 >expect &&\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n \tcp -R .git read-only.git &&\n \ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n-- \n2.26.0.113.ge9739cdccc\n"},{"id":"395972","messageId":"20200423011444.GG140314@google.com","threadId":"53278","inReplyTo":"5c9217ad8fc594fbff46507c4be7961eb5a478e2.1587601501.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-23T01:14:44Z","receivedAt":"2020-04-23T01:14:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTaylor Blau wrote:\n\n> A number of spots in t5537 use the non-indented heredoc '< they would benefit from instead using '<<-EOF' or simply\n> test_write_lines.\n>\n> In preparation for adding new tests in a good style and being consistent\n> with the surrounding code, update the existing tests to improve their\n> readability.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Taylor Blau \n> ---\n> t/t5537-fetch-shallow.sh | 70 +++++++++++-----------------------------\n> 1 file changed, 18 insertions(+), 52 deletions(-)\n\nSounds like a good idea. Some nitpicks --- please don't act on them\nall, but only the ones that seem appropriate to you:\n\n[...]\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -25,10 +25,7 @@ test_expect_success 'setup' '\n> test_expect_success 'setup shallow clone' '\n> \tgit clone --no-local --depth=2 .git shallow &&\n> \tgit --git-dir=shallow/.git log --format=%s >actual &&\n> -\tcat <expect &&\n> -4\n> -3\n> -EOF\n> +\ttest_write_lines 4 3 >expect &&\n> \ttest_cmp expect actual\n> '\n\nNice.\n\n[...]\n> @@ -133,14 +110,12 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '\n[...]\n> -\tcat <expect &&\n> -no-shallow\n> -EOF\n> +\tcat <<-EOF >expect &&\n> +\tno-shallow\n> +\tEOF\n\nCan this use \"echo\"? Or if using cat, please quote the EOF in <<-EOF\nso the reader doesn't have to check for $substitutions in the body:\n\n\t\tcat >expect <<-\\EOF &&\n\n[...]\n> @@ -158,21 +133,15 @@ test_expect_success 'fetch --update-shallow' '\n> \tgit fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n> \tgit fsck &&\n> \tgit for-each-ref --sort=refname --format=\"%(refname)\" >actual.refs &&\n> -\tcat <expect.refs &&\n> -refs/remotes/shallow/master\n> -refs/remotes/shallow/no-shallow\n> -refs/tags/heavy-tag\n> -refs/tags/light-tag\n> -EOF\n> +\tcat <<-EOF >expect.refs &&\n\nLikewise (missing \\ before EOF).\n\nA few more nits, that probably don't belong in the same patch:\n\n- the code in subshells would be more readable if indented\n- existing <<-EOF here blocks should \\quote the EOF\n- the resulting history would be more realistic if it uses test_tick\n before running \"git commit\". Or perhaps this can use the\n test_commit helper to handle that\n- should use test_must_fail in preference to ! git\n- might be simpler if http tests go in a different file\n\nThanks,\nJonathan\n"},{"id":"395975","messageId":"20200423012305.GH140314@google.com","threadId":"53278","inReplyTo":"296e70790d7a391d471554b0bc5a58e2a091ce88.1587601501.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-23T01:23:05Z","receivedAt":"2020-04-23T01:23:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -872,12 +872,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> \topt.env = tmp_objdir_env(tmp_objdir);\n> \tsetup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);\n> \tif (check_connected(command_singleton_iterator, cmd, &opt)) {\n> -\t\trollback_lock_file(&shallow_lock);\n> +\t\trollback_shallow_file(the_repository, &shallow_lock);\n\nI like it.\n\nI wonder, is there a way we can make it more difficult to accidentally\nuse rollback_lock_file where rollback_shallow_file is needed? For\nexample, what if shallow_lock has a different type \"struct\nshallow_lock\" so one would have to reach in to its lock_file member to\nbypass the shallow_file interface?\n\n[...]\n> \t\toid_array_clear(&extra);\n> \t\treturn -1;\n> \t}\n> \n> -\tcommit_lock_file(&shallow_lock);\n> +\tcommit_shallow_file(the_repository, &shallow_lock);\n> \n> \t/*\n> \t * Make sure setup_alternate_shallow() for the next ref does\n> diff --git a/commit.h b/commit.h\n> index 008a0fa4a0..ab91d21131 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -249,6 +249,8 @@ struct oid_array;\n> struct ref;\n> int register_shallow(struct repository *r, const struct object_id *oid);\n> int unregister_shallow(const struct object_id *oid);\n> +int commit_shallow_file(struct repository *r, struct lock_file *lk);\n> +void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n\noptional: might make sense to put this near setup_alternate_shallow\nfor discoverability\n\nCould this have an API doc comment?\n\n[...]\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\nNot about this patch: it might make sense to split out a shallow.h\nheader / API.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"396019","messageId":"20200423180905.195407-1-jonathantanmy@google.com","threadId":"53278","inReplyTo":"296e70790d7a391d471554b0bc5a58e2a091ce88.1587601501.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-23T18:09:05Z","receivedAt":"2020-04-23T18:09:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Replace each instance of 'commit_lock_file' and 'rollback_lock_file'\n> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock\n> being held is over the '.git/shallow' file.\n\nI think Jonathan Nieder already covered 1/2 so I'll just close the loop\non this patch. There was one potential issue in that a previous version\nof this patch also called reset_repository_shallow() in\nsetup_alternate_shallow(), but this version does not. But after looking\ninto it, this looks fine - setup_alternate_shallow() deals with a\npassed-in alternate_shallow_file variable, which is different from the\nr->parsed_objects->alternate_shallow_file that is_repository_shallow()\nuses to set the global variables. (I might have confused the two during\nearlier reviews.) Also, setup_alternate_shallow() is called either\nbefore any shallow processing (empirically demonstrating that no\nresetting is needed in this case, because it has been working), or right\nbefore a commit or rollback of the lock file (so the global variables\nare being reset anyway, so we do not need to call\nreset_repository_shallow()).\n\nSo,\nReviewed-by: Jonathan Tan \n"},{"id":"396022","messageId":"xmqq5zdqhvkk.fsf@gitster.c.googlers.com","threadId":"53278","inReplyTo":"20200423001438.GC19100@syl.local","subject":"Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-23T19:05:15Z","receivedAt":"2020-04-23T19:05:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> What I am trying to get at is if it would be safer to have a pair of\n>> thin wrapper for rolling back or committing a new version of new\n>> shallow file, e.g. rollback_shallow_file() + commit_shallow_file(),\n>> and replace calls to {rollback,commit}_lock_file() with calls to\n>> them.\n>\n> Very elegant. Thanks for an excellent suggestion. v2 incoming just as\n> soon as 'make test' finishes...\n\nNote that I didn't verify there is a case where we want not to call\nreset_repository_shallow() after committing or rolling back, either\nfor performance or correctness purposes. As long as the experts on\nthe codepaths involved are happy with the idea, I'd be happy, too.\n\nJNieder raised the idea of using a different type to avoid calling\nthe bare rollback/commit functions by mistake. It appears that, in\naddition to these two functions, setup_alternate_shallow() needs to\nbe updated if we wanted to go that route, and it sounds like a good\nidea to gain safety with minimal cost.\n\nThanks.\n\n\n"},{"id":"396031","messageId":"xmqqr1weey0g.fsf@gitster.c.googlers.com","threadId":"53278","inReplyTo":"20200423180905.195407-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-23T20:40:47Z","receivedAt":"2020-04-23T20:40:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> Replace each instance of 'commit_lock_file' and 'rollback_lock_file'\n>> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock\n>> being held is over the '.git/shallow' file.\n>\n> I think Jonathan Nieder already covered 1/2 so I'll just close the loop\n> on this patch. There was one potential issue in that a previous version\n> of this patch also called reset_repository_shallow() in\n> setup_alternate_shallow(), but this version does not. But after looking\n> into it, this looks fine - setup_alternate_shallow() deals with a\n> passed-in alternate_shallow_file variable, which is different from the\n> r->parsed_objects->alternate_shallow_file that is_repository_shallow()\n> uses to set the global variables. (I might have confused the two during\n> earlier reviews.) Also, setup_alternate_shallow() is called either\n> before any shallow processing (empirically demonstrating that no\n> resetting is needed in this case, because it has been working), or right\n> before a commit or rollback of the lock file (so the global variables\n> are being reset anyway, so we do not need to call\n> reset_repository_shallow()).\n>\n> So,\n> Reviewed-by: Jonathan Tan \n\nThanks for a review.\n\nAnd of course, thanks Taylor. Will queue.\n"},{"id":"396154","messageId":"20200424171139.GA61470@syl.local","threadId":"53278","inReplyTo":"20200423011444.GG140314@google.com","subject":"Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-24T17:11:39Z","receivedAt":"2020-04-24T17:11:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 22, 2020 at 06:14:44PM -0700, Jonathan Nieder wrote:\n> Hi,\n>\n> Taylor Blau wrote:\n>\n> > A number of spots in t5537 use the non-indented heredoc '< > they would benefit from instead using '<<-EOF' or simply\n> > test_write_lines.\n> >\n> > In preparation for adding new tests in a good style and being consistent\n> > with the surrounding code, update the existing tests to improve their\n> > readability.\n> >\n> > Suggested-by: Junio C Hamano \n> > Signed-off-by: Taylor Blau \n> > ---\n> > t/t5537-fetch-shallow.sh | 70 +++++++++++-----------------------------\n> > 1 file changed, 18 insertions(+), 52 deletions(-)\n>\n> Sounds like a good idea. Some nitpicks --- please don't act on them\n> all, but only the ones that seem appropriate to you:\n\nThanks -- here's an updated venison of this patch with some of the more\nminor points addressed. I agree that there is room for cleaning up these\ntests more substantially, but I think that those can be done in other\npatches.\n\nJunio -- would you please used the patch below as PATCH 1/2 instead of\nthe original here? Thanks.\n\n-- 8< --\n\nSubject: [PATCH] t5537: use test_write_lines, indented heredocs for readability\n\nA number of spots in t5537 use the non-indented heredoc '<\nSigned-off-by: Taylor Blau \n---\n t/t5537-fetch-shallow.sh | 70 ++++++++++------------------------------\n 1 file changed, 17 insertions(+), 53 deletions(-)\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 4f681dbbe1..d02f9b5ae8 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -16,7 +16,7 @@ test_expect_success 'setup' '\n \tcommit 3 &&\n \tcommit 4 &&\n \tgit config --global transfer.fsckObjects true &&\n-\ttest_oid_cache <<-EOF\n+\ttest_oid_cache <<-\\EOF\n \tperl sha1:s/0034shallow %s/0036unshallow %s/\n \tperl sha256:s/004cshallow %s/004eunshallow %s/\n \tEOF\n@@ -25,10 +25,7 @@ test_expect_success 'setup' '\n test_expect_success 'setup shallow clone' '\n \tgit clone --no-local --depth=2 .git shallow &&\n \tgit --git-dir=shallow/.git log --format=%s >actual &&\n-\tcat <expect &&\n-4\n-3\n-EOF\n+\ttest_write_lines 4 3 >expect &&\n \ttest_cmp expect actual\n '\n\n@@ -38,10 +35,7 @@ test_expect_success 'clone from shallow clone' '\n \tcd shallow2 &&\n \tgit fsck &&\n \tgit log --format=%s >actual &&\n-\tcat <expect &&\n-4\n-3\n-EOF\n+\ttest_write_lines 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -56,11 +50,7 @@ test_expect_success 'fetch from shallow clone' '\n \tgit fetch &&\n \tgit fsck &&\n \tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-5\n-4\n-3\n-EOF\n+\ttest_write_lines 5 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -75,10 +65,7 @@ test_expect_success 'fetch --depth from shallow clone' '\n \tgit fetch --depth=2 &&\n \tgit fsck &&\n \tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-6\n-5\n-EOF\n+\ttest_write_lines 6 5 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -89,12 +76,7 @@ test_expect_success 'fetch --unshallow from shallow clone' '\n \tgit fetch --unshallow &&\n \tgit fsck &&\n \tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-6\n-5\n-4\n-3\n-EOF\n+\ttest_write_lines 6 5 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -111,15 +93,10 @@ test_expect_success 'fetch something upstream has but hidden by clients shallow\n \tgit fetch ../.git +refs/heads/master:refs/remotes/top/master &&\n \tgit fsck &&\n \tgit log --format=%s top/master >actual &&\n-\tcat <expect &&\n-add-1-back\n-4\n-3\n-EOF\n+\ttest_write_lines add-1-back 4 3 >expect &&\n \ttest_cmp expect actual\n \t) &&\n \tgit --git-dir=shallow2/.git cat-file blob $(echo 1|git hash-object --stdin) >/dev/null\n-\n '\n\n test_expect_success 'fetch that requires changes in .git/shallow is filtered' '\n@@ -133,14 +110,10 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '\n \tcd notshallow &&\n \tgit fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&\n \tgit for-each-ref --format=\"%(refname)\" >actual.refs &&\n-\tcat <expect.refs &&\n-refs/remotes/shallow/no-shallow\n-EOF\n+\techo refs/remotes/shallow/no-shallow >expect.refs &&\n \ttest_cmp expect.refs actual.refs &&\n \tgit log --format=%s shallow/no-shallow >actual &&\n-\tcat <expect &&\n-no-shallow\n-EOF\n+\techo no-shallow >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -158,21 +131,15 @@ test_expect_success 'fetch --update-shallow' '\n \tgit fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n \tgit fsck &&\n \tgit for-each-ref --sort=refname --format=\"%(refname)\" >actual.refs &&\n-\tcat <expect.refs &&\n-refs/remotes/shallow/master\n-refs/remotes/shallow/no-shallow\n-refs/tags/heavy-tag\n-refs/tags/light-tag\n-EOF\n+\tcat <<-\\EOF >expect.refs &&\n+\trefs/remotes/shallow/master\n+\trefs/remotes/shallow/no-shallow\n+\trefs/tags/heavy-tag\n+\trefs/tags/light-tag\n+\tEOF\n \ttest_cmp expect.refs actual.refs &&\n \tgit log --format=%s shallow/master >actual &&\n-\tcat <expect &&\n-7\n-6\n-5\n-4\n-3\n-EOF\n+\ttest_write_lines 7 6 5 4 3 >expect &&\n \ttest_cmp expect actual\n \t)\n '\n@@ -183,10 +150,7 @@ test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n \tfind read-only.git -print | xargs chmod -w &&\n \tgit clone --no-local --depth=2 read-only.git from-read-only &&\n \tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n-\tcat >expect <expect &&\n \ttest_cmp expect actual\n '\n\n--\n2.26.0.113.ge9739cdccc\n\n"},{"id":"396155","messageId":"20200424171301.GB61470@syl.local","threadId":"53278","inReplyTo":"xmqqr1weey0g.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-24T17:13:01Z","receivedAt":"2020-04-24T17:13:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 23, 2020 at 01:40:47PM -0700, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n> >> Replace each instance of 'commit_lock_file' and 'rollback_lock_file'\n> >> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock\n> >> being held is over the '.git/shallow' file.\n> >\n> > I think Jonathan Nieder already covered 1/2 so I'll just close the loop\n> > on this patch. There was one potential issue in that a previous version\n> > of this patch also called reset_repository_shallow() in\n> > setup_alternate_shallow(), but this version does not. But after looking\n> > into it, this looks fine - setup_alternate_shallow() deals with a\n> > passed-in alternate_shallow_file variable, which is different from the\n> > r->parsed_objects->alternate_shallow_file that is_repository_shallow()\n> > uses to set the global variables. (I might have confused the two during\n> > earlier reviews.) Also, setup_alternate_shallow() is called either\n> > before any shallow processing (empirically demonstrating that no\n> > resetting is needed in this case, because it has been working), or right\n> > before a commit or rollback of the lock file (so the global variables\n> > are being reset anyway, so we do not need to call\n> > reset_repository_shallow()).\n> >\n> > So,\n> > Reviewed-by: Jonathan Tan \n>\n> Thanks for a review.\n>\n> And of course, thanks Taylor. Will queue.\n\nThanks, both. I'll send some more patches on top to introduce a\n'shallow_lock' type.\n\nThanks,\nTaylor\n"},{"id":"396156","messageId":"20200424171738.GA17217@google.com","threadId":"53278","inReplyTo":"20200424171139.GA61470@syl.local","subject":"Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-24T17:17:38Z","receivedAt":"2020-04-24T17:17:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n> Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability\n>\n> A number of spots in t5537 use the non-indented heredoc '< they would benefit from instead using '<<-EOF' or simply\n> test_write_lines.\n>\n> In preparation for adding new tests in a good style and being consistent\n> with the surrounding code, update the existing tests to improve their\n> readability.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Taylor Blau \n> ---\n> t/t5537-fetch-shallow.sh | 70 ++++++++++------------------------------\n> 1 file changed, 17 insertions(+), 53 deletions(-)\n\nThe commit message is missing the mention of \\EOF quoting.\n\nSee \"git log --grep='modernize style'\" for some examples about how to\nwrite this kind of commit message without it getting tedious.\n\nThat said, I think it's fine --- just mentioning that for the future.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"396180","messageId":"xmqqpnbwbojr.fsf@gitster.c.googlers.com","threadId":"53278","inReplyTo":"20200424171139.GA61470@syl.local","subject":"Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T20:45:44Z","receivedAt":"2020-04-24T20:45:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability\n\nAssuming that \"indented\" is not a verb in the past tense, but\nadjective on the noun \"heredocs\", this reads well, but \"s/,/ and/\"\nwould make it more clear in that case. Or \"for readability, use X\nand indent Y\" would work equally well.\n\nBut the original is probably OK as-is.\n\n> -\tcat <expect &&\n> -4\n> -3\n> -EOF\n> +\ttest_write_lines 4 3 >expect &&\n\nYeah, becomes much nicer with test_write_lines.\n\nThanks.\n"},{"id":"399065","messageId":"20200603034213.GB253041@google.com","threadId":"53278","inReplyTo":"296e70790d7a391d471554b0bc5a58e2a091ce88.1587601501.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-06-03T03:42:13Z","receivedAt":"2020-06-03T03:42:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTaylor Blau wrote:\n\n> Signed-off-by: Taylor Blau \n> ---\n> builtin/receive-pack.c | 4 ++--\n> commit.h | 2 ++\n> fetch-pack.c | 10 +++++-----\n> shallow.c | 30 +++++++++++++++++++++---------\n> t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++\n> 5 files changed, 59 insertions(+), 16 deletions(-)\n\nI haven't investigated the cause yet, but I've run into an interesting\nbug that bisects to this commit. Jay Conrod (cc-ed) reports:\n\n| I believe this is also the cause of Go toolchain test failures we've\n| been seeing. Go uses git to fetch dependencies.\n|\n| The problem we're seeing can be reproduced with the script below. It\n| should print \"success\". Instead, the git merge-base command fails\n| because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c\n| (origin/master) has no history.\n\n-- 8< --\n#!/bin/bash\n\nset -euxo pipefail\nif [ -d legacytest ]; then\n echo \"legacytest directory already exists\" >&2\n exit 1\nfi\nmkdir legacytest\ncd legacytest\ngit init --bare\ngit config protocol.version 2\ngit config fetch.writeCommitGraph true\ngit remote add origin -- https://github.com/rsc/legacytest\ngit fetch -f --depth=1 origin refs/heads/master:refs/heads/master\ngit fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'\ngit fetch --unshallow -f origin\ngit merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c\necho success\n-- >8 --\n\nThe fetch.writeCommitGraph part is interesting. When does a commit\ngraph file get written in this sequence of operations? In an\nunshallow operation, does the usual guard against writing a commit\ngraph in a shallow repo get missed?\n\n\"rm -fr objects/info/commit-graphs\" recovers the full history in the\nrepo, so this is not a case of writing the wrong shallows --- it's\nonly a commit graph issue.\n\nI'll take a closer look, but thought I'd give others a chance to look\nto in case there's something obvious. :)\n\nThanks,\nJonathan\n"},{"id":"399067","messageId":"20200603045248.GA20266@syl.local","threadId":"53278","inReplyTo":"20200603034213.GB253041@google.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-03T04:52:48Z","receivedAt":"2020-06-03T04:52:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Jonathan,\n\nOn Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:\n> Hi,\n>\n> Taylor Blau wrote:\n>\n> > Signed-off-by: Taylor Blau \n> > ---\n> > builtin/receive-pack.c | 4 ++--\n> > commit.h | 2 ++\n> > fetch-pack.c | 10 +++++-----\n> > shallow.c | 30 +++++++++++++++++++++---------\n> > t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++\n> > 5 files changed, 59 insertions(+), 16 deletions(-)\n>\n> I haven't investigated the cause yet, but I've run into an interesting\n> bug that bisects to this commit. Jay Conrod (cc-ed) reports:\n>\n> | I believe this is also the cause of Go toolchain test failures we've\n> | been seeing. Go uses git to fetch dependencies.\n> |\n> | The problem we're seeing can be reproduced with the script below. It\n> | should print \"success\". Instead, the git merge-base command fails\n> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c\n> | (origin/master) has no history.\n>\n> -- 8< --\n> #!/bin/bash\n>\n> set -euxo pipefail\n> if [ -d legacytest ]; then\n> echo \"legacytest directory already exists\" >&2\n> exit 1\n> fi\n> mkdir legacytest\n> cd legacytest\n> git init --bare\n> git config protocol.version 2\n> git config fetch.writeCommitGraph true\n> git remote add origin -- https://github.com/rsc/legacytest\n> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master\n> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'\n> git fetch --unshallow -f origin\n> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c\n> echo success\n> -- >8 --\n\nThanks to you and Jay for the report and reproduction script. Indeed, I\ncan reproduce this on the tip of master (which is equivalent to v2.27.0\nat the time of writing).\n\n> The fetch.writeCommitGraph part is interesting. When does a commit\n> graph file get written in this sequence of operations? In an\n> unshallow operation, does the usual guard against writing a commit\n> graph in a shallow repo get missed?\n\nThe last 'git fetch' is the one that writes the commit-graph. You can\nverify this by sticking a 'ls objects/info' after each 'git' invocation\nin your script.\n\nHere's where things get weird, though. Prior to this patch, Git would\npick up that the repository is shallow before unshallowing, but never\ninvalidate this fact after unshallowing. That means that once we got to\n'write_commit_graph', we'd exit immediately since it appears as if the\nrepository is shallow.\n\nIn this patch, we don't do that anymore, since we rightly unset the fact\nthat we are (were) shallow.\n\nIn a debugger, I ran your script and a 'git commit-graph write --split\n--reachable' side-by-side, and found an interesting discrepancy: some\ncommits (loaded from 'copy_oids_to_commits') *don't* have their parents\nset when invoked from 'git fetch', but *do* when invoked as 'git\ncommit-graph write ...'.\n\nI'm not an expert in the object cache, but my hunch is that when we\nfetch these objects they're marked as parsed without having loaded their\nparents. When we load them again via 'lookup_object', we get objects\nthat look parsed, but don't have parents where they otherwise should.\n\nI'm going to CC Stolee to see if he has any thoughts on how to handle\nthis and/or if my idea is on the right track.\n\n> \"rm -fr objects/info/commit-graphs\" recovers the full history in the\n> repo, so this is not a case of writing the wrong shallows --- it's\n> only a commit graph issue.\n>\n> I'll take a closer look, but thought I'd give others a chance to look\n> to in case there's something obvious. :)\n>\n> Thanks,\n> Jonathan\n\nThanks,\nTaylor\n"},{"id":"399068","messageId":"20200603051631.GA20678@syl.local","threadId":"53278","inReplyTo":"20200603045248.GA20266@syl.local","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-03T05:16:31Z","receivedAt":"2020-06-03T05:16:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote:\n> Hi Jonathan,\n>\n> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:\n> > Hi,\n> >\n> > Taylor Blau wrote:\n> >\n> > > Signed-off-by: Taylor Blau \n> > > ---\n> > > builtin/receive-pack.c | 4 ++--\n> > > commit.h | 2 ++\n> > > fetch-pack.c | 10 +++++-----\n> > > shallow.c | 30 +++++++++++++++++++++---------\n> > > t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++\n> > > 5 files changed, 59 insertions(+), 16 deletions(-)\n> >\n> > I haven't investigated the cause yet, but I've run into an interesting\n> > bug that bisects to this commit. Jay Conrod (cc-ed) reports:\n> >\n> > | I believe this is also the cause of Go toolchain test failures we've\n> > | been seeing. Go uses git to fetch dependencies.\n> > |\n> > | The problem we're seeing can be reproduced with the script below. It\n> > | should print \"success\". Instead, the git merge-base command fails\n> > | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c\n> > | (origin/master) has no history.\n> >\n> > -- 8< --\n> > #!/bin/bash\n> >\n> > set -euxo pipefail\n> > if [ -d legacytest ]; then\n> > echo \"legacytest directory already exists\" >&2\n> > exit 1\n> > fi\n> > mkdir legacytest\n> > cd legacytest\n> > git init --bare\n> > git config protocol.version 2\n> > git config fetch.writeCommitGraph true\n> > git remote add origin -- https://github.com/rsc/legacytest\n> > git fetch -f --depth=1 origin refs/heads/master:refs/heads/master\n> > git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'\n> > git fetch --unshallow -f origin\n> > git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c\n> > echo success\n> > -- >8 --\n>\n> Thanks to you and Jay for the report and reproduction script. Indeed, I\n> can reproduce this on the tip of master (which is equivalent to v2.27.0\n> at the time of writing).\n>\n> > The fetch.writeCommitGraph part is interesting. When does a commit\n> > graph file get written in this sequence of operations? In an\n> > unshallow operation, does the usual guard against writing a commit\n> > graph in a shallow repo get missed?\n>\n> The last 'git fetch' is the one that writes the commit-graph. You can\n> verify this by sticking a 'ls objects/info' after each 'git' invocation\n> in your script.\n>\n> Here's where things get weird, though. Prior to this patch, Git would\n> pick up that the repository is shallow before unshallowing, but never\n> invalidate this fact after unshallowing. That means that once we got to\n> 'write_commit_graph', we'd exit immediately since it appears as if the\n> repository is shallow.\n>\n> In this patch, we don't do that anymore, since we rightly unset the fact\n> that we are (were) shallow.\n>\n> In a debugger, I ran your script and a 'git commit-graph write --split\n> --reachable' side-by-side, and found an interesting discrepancy: some\n> commits (loaded from 'copy_oids_to_commits') *don't* have their parents\n> set when invoked from 'git fetch', but *do* when invoked as 'git\n> commit-graph write ...'.\n>\n> I'm not an expert in the object cache, but my hunch is that when we\n> fetch these objects they're marked as parsed without having loaded their\n> parents. When we load them again via 'lookup_object', we get objects\n> that look parsed, but don't have parents where they otherwise should.\n\nAh, this only sort of has to do with the object cache. In\n'parse_commit_buffer()' we stop parsing parents in the case that the\nrepository is shallow (this goes back to 7f3140cd23 (git repack: keep\ncommits hidden by a graft, 2009-07-23)).\n\nThat makes me somewhat nervous. We're going to keep any objects opened\nprior to unshallowing in the cache, along with their hidden parents. I\nsuspect that this is why Git has kept the shallow bit as sticky for so\nlong.\n\nI'm not quite sure what to do here. I think that any of the following\nwould work:\n\n * Keep the shallow bit sticky, at least for fetch.writeCommitGraph\n (i.e., pretend as if fetch.writecommitgraph=0 in the case of\n '--unshallow').\n\n * Dump the object cache upon un-shallowing, forcing us to re-discover\n the parents when they are no longer hidden behind a graft.\n\nThe latter seems like the most complete feasible fix. The former should\nwork fine to address this case, but I wonder if there are other\ncall-sites that are affected by this behavior. My hunch is that this is\na unique case, since it requires going from shallow to unshallow in the\nsame process.\n\nI have yet to create a smaller test case, but the following should be\nsufficient to dump the cache of parsed objects upon shallowing or\nun-shallowing:\n\ndiff --git a/shallow.c b/shallow.c\nindex b826de9b67..06db857f53 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -90,6 +90,9 @@ static 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+\tparsed_object_pool_clear(r->parsed_objects);\n+\tr->parsed_objects = parsed_object_pool_new();\n }\n\n int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\n\nIs this something we want to go forward with? Are there some\nfar-reaching implications that I'm missing?\n\n> I'm going to CC Stolee to see if he has any thoughts on how to handle\n> this and/or if my idea is on the right track.\n>\n> > \"rm -fr objects/info/commit-graphs\" recovers the full history in the\n> > repo, so this is not a case of writing the wrong shallows --- it's\n> > only a commit graph issue.\n> >\n> > I'll take a closer look, but thought I'd give others a chance to look\n> > to in case there's something obvious. :)\n> >\n> > Thanks,\n> > Jonathan\n>\n> Thanks,\n> Taylor\n\nThanks,\nTaylor\n"},{"id":"399074","messageId":"1253efb6-f1bc-0a16-68e3-c1bc07e1bc18@gmail.com","threadId":"53278","inReplyTo":"20200603051631.GA20678@syl.local","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-03T13:08:26Z","receivedAt":"2020-06-03T13:08:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/3/2020 1:16 AM, Taylor Blau wrote:\n> On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote:\n>> Hi Jonathan,\n>>\n>> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:\n>>> Hi,\n>>>\n>>> Taylor Blau wrote:\n>>>\n>>>> Signed-off-by: Taylor Blau \n>>>> ---\n>>>> builtin/receive-pack.c | 4 ++--\n>>>> commit.h | 2 ++\n>>>> fetch-pack.c | 10 +++++-----\n>>>> shallow.c | 30 +++++++++++++++++++++---------\n>>>> t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++\n>>>> 5 files changed, 59 insertions(+), 16 deletions(-)\n>>>\n>>> I haven't investigated the cause yet, but I've run into an interesting\n>>> bug that bisects to this commit. Jay Conrod (cc-ed) reports:\n>>>\n>>> | I believe this is also the cause of Go toolchain test failures we've\n>>> | been seeing. Go uses git to fetch dependencies.\n>>> |\n>>> | The problem we're seeing can be reproduced with the script below. It\n>>> | should print \"success\". Instead, the git merge-base command fails\n>>> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c\n>>> | (origin/master) has no history.\n>>>\n>>> -- 8< --\n>>> #!/bin/bash\n>>>\n>>> set -euxo pipefail\n>>> if [ -d legacytest ]; then\n>>> echo \"legacytest directory already exists\" >&2\n>>> exit 1\n>>> fi\n>>> mkdir legacytest\n>>> cd legacytest\n>>> git init --bare\n>>> git config protocol.version 2\n>>> git config fetch.writeCommitGraph true\n>>> git remote add origin -- https://github.com/rsc/legacytest\n>>> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master\n>>> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'\n>>> git fetch --unshallow -f origin\n>>> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c\n>>> echo success\n>>> -- >8 --\n>>\n>> Thanks to you and Jay for the report and reproduction script. Indeed, I\n>> can reproduce this on the tip of master (which is equivalent to v2.27.0\n>> at the time of writing).\n>>\n>>> The fetch.writeCommitGraph part is interesting. When does a commit\n>>> graph file get written in this sequence of operations? In an\n>>> unshallow operation, does the usual guard against writing a commit\n>>> graph in a shallow repo get missed?\n>>\n>> The last 'git fetch' is the one that writes the commit-graph. You can\n>> verify this by sticking a 'ls objects/info' after each 'git' invocation\n>> in your script.\n>>\n>> Here's where things get weird, though. Prior to this patch, Git would\n>> pick up that the repository is shallow before unshallowing, but never\n>> invalidate this fact after unshallowing. That means that once we got to\n>> 'write_commit_graph', we'd exit immediately since it appears as if the\n>> repository is shallow.\n>>\n>> In this patch, we don't do that anymore, since we rightly unset the fact\n>> that we are (were) shallow.\n>>\n>> In a debugger, I ran your script and a 'git commit-graph write --split\n>> --reachable' side-by-side, and found an interesting discrepancy: some\n>> commits (loaded from 'copy_oids_to_commits') *don't* have their parents\n>> set when invoked from 'git fetch', but *do* when invoked as 'git\n>> commit-graph write ...'.\n>>\n>> I'm not an expert in the object cache, but my hunch is that when we\n>> fetch these objects they're marked as parsed without having loaded their\n>> parents. When we load them again via 'lookup_object', we get objects\n>> that look parsed, but don't have parents where they otherwise should.\n> \n> Ah, this only sort of has to do with the object cache. In\n> 'parse_commit_buffer()' we stop parsing parents in the case that the\n> repository is shallow (this goes back to 7f3140cd23 (git repack: keep\n> commits hidden by a graft, 2009-07-23)).\n> \n> That makes me somewhat nervous. We're going to keep any objects opened\n> prior to unshallowing in the cache, along with their hidden parents. I\n> suspect that this is why Git has kept the shallow bit as sticky for so\n> long.\n> \n> I'm not quite sure what to do here. I think that any of the following\n> would work:\n> \n> * Keep the shallow bit sticky, at least for fetch.writeCommitGraph\n> (i.e., pretend as if fetch.writecommitgraph=0 in the case of\n> '--unshallow').\n\nI'm in favor of this option, if possible. Anything that alters the\ncommit history in-memory at any point in the Git process is unsafe to\ncombine with a commit-graph read _or_ write. I'm sorry that the guards\nin commit_graph_compatible() are not enough here.\n\n> * Dump the object cache upon un-shallowing, forcing us to re-discover\n> the parents when they are no longer hidden behind a graft.\n> \n> The latter seems like the most complete feasible fix. The former should\n> work fine to address this case, but I wonder if there are other\n> call-sites that are affected by this behavior. My hunch is that this is\n> a unique case, since it requires going from shallow to unshallow in the\n> same process.\n\nThe latter would solve issues that could arise outside of the commit-graph\nspace. But it also presents an opportunity for another gap if someone edits\nthe shallow logic without putting in the proper guards.\n\nTo be extra safe, I'd be in favor of adding an \"if (grafts_ever_existed)\"\ncondition in commit_graph_compatible() based on a global that is assigned\na non-zero value whenever grafts are loaded at any point in the process,\nmostly because it would be easy to guarantee that it is safe. It could\neven be localized to the repository struct.\n \n> I have yet to create a smaller test case, but the following should be\n> sufficient to dump the cache of parsed objects upon shallowing or\n> un-shallowing:\n> \n> diff --git a/shallow.c b/shallow.c\n> index b826de9b67..06db857f53 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -90,6 +90,9 @@ static 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> +\tparsed_object_pool_clear(r->parsed_objects);\n> +\tr->parsed_objects = parsed_object_pool_new();\n> }\n> \n> int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\n> \n> Is this something we want to go forward with? Are there some\n> far-reaching implications that I'm missing?\n\nI'd like to see the extra-careful check, in addition to this one. This\nis such a rarely-used and narrowly-tested case that we need to be really\nreally careful to avoid regressions.\n\nThanks,\n-Stolee\n\n"},{"id":"399084","messageId":"20200603192608.GB24049@syl.local","threadId":"53278","inReplyTo":"1253efb6-f1bc-0a16-68e3-c1bc07e1bc18@gmail.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-03T19:26:08Z","receivedAt":"2020-06-03T19:26:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Stolee,\n\nOn Wed, Jun 03, 2020 at 09:08:26AM -0400, Derrick Stolee wrote:\n> On 6/3/2020 1:16 AM, Taylor Blau wrote:\n> > On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote:\n> >> Hi Jonathan,\n> >>\n> >> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:\n> >>> Hi,\n> >>>\n> >>> Taylor Blau wrote:\n> >>>\n> >>>> Signed-off-by: Taylor Blau \n> >>>> ---\n> >>>> builtin/receive-pack.c | 4 ++--\n> >>>> commit.h | 2 ++\n> >>>> fetch-pack.c | 10 +++++-----\n> >>>> shallow.c | 30 +++++++++++++++++++++---------\n> >>>> t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++\n> >>>> 5 files changed, 59 insertions(+), 16 deletions(-)\n> >>>\n> >>> I haven't investigated the cause yet, but I've run into an interesting\n> >>> bug that bisects to this commit. Jay Conrod (cc-ed) reports:\n> >>>\n> >>> | I believe this is also the cause of Go toolchain test failures we've\n> >>> | been seeing. Go uses git to fetch dependencies.\n> >>> |\n> >>> | The problem we're seeing can be reproduced with the script below. It\n> >>> | should print \"success\". Instead, the git merge-base command fails\n> >>> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c\n> >>> | (origin/master) has no history.\n> >>>\n> >>> -- 8< --\n> >>> #!/bin/bash\n> >>>\n> >>> set -euxo pipefail\n> >>> if [ -d legacytest ]; then\n> >>> echo \"legacytest directory already exists\" >&2\n> >>> exit 1\n> >>> fi\n> >>> mkdir legacytest\n> >>> cd legacytest\n> >>> git init --bare\n> >>> git config protocol.version 2\n> >>> git config fetch.writeCommitGraph true\n> >>> git remote add origin -- https://github.com/rsc/legacytest\n> >>> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master\n> >>> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'\n> >>> git fetch --unshallow -f origin\n> >>> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c\n> >>> echo success\n> >>> -- >8 --\n> >>\n> >> Thanks to you and Jay for the report and reproduction script. Indeed, I\n> >> can reproduce this on the tip of master (which is equivalent to v2.27.0\n> >> at the time of writing).\n> >>\n> >>> The fetch.writeCommitGraph part is interesting. When does a commit\n> >>> graph file get written in this sequence of operations? In an\n> >>> unshallow operation, does the usual guard against writing a commit\n> >>> graph in a shallow repo get missed?\n> >>\n> >> The last 'git fetch' is the one that writes the commit-graph. You can\n> >> verify this by sticking a 'ls objects/info' after each 'git' invocation\n> >> in your script.\n> >>\n> >> Here's where things get weird, though. Prior to this patch, Git would\n> >> pick up that the repository is shallow before unshallowing, but never\n> >> invalidate this fact after unshallowing. That means that once we got to\n> >> 'write_commit_graph', we'd exit immediately since it appears as if the\n> >> repository is shallow.\n> >>\n> >> In this patch, we don't do that anymore, since we rightly unset the fact\n> >> that we are (were) shallow.\n> >>\n> >> In a debugger, I ran your script and a 'git commit-graph write --split\n> >> --reachable' side-by-side, and found an interesting discrepancy: some\n> >> commits (loaded from 'copy_oids_to_commits') *don't* have their parents\n> >> set when invoked from 'git fetch', but *do* when invoked as 'git\n> >> commit-graph write ...'.\n> >>\n> >> I'm not an expert in the object cache, but my hunch is that when we\n> >> fetch these objects they're marked as parsed without having loaded their\n> >> parents. When we load them again via 'lookup_object', we get objects\n> >> that look parsed, but don't have parents where they otherwise should.\n> >\n> > Ah, this only sort of has to do with the object cache. In\n> > 'parse_commit_buffer()' we stop parsing parents in the case that the\n> > repository is shallow (this goes back to 7f3140cd23 (git repack: keep\n> > commits hidden by a graft, 2009-07-23)).\n> >\n> > That makes me somewhat nervous. We're going to keep any objects opened\n> > prior to unshallowing in the cache, along with their hidden parents. I\n> > suspect that this is why Git has kept the shallow bit as sticky for so\n> > long.\n> >\n> > I'm not quite sure what to do here. I think that any of the following\n> > would work:\n> >\n> > * Keep the shallow bit sticky, at least for fetch.writeCommitGraph\n> > (i.e., pretend as if fetch.writecommitgraph=0 in the case of\n> > '--unshallow').\n>\n> I'm in favor of this option, if possible. Anything that alters the\n> commit history in-memory at any point in the Git process is unsafe to\n> combine with a commit-graph read _or_ write. I'm sorry that the guards\n> in commit_graph_compatible() are not enough here.\n>\n> > * Dump the object cache upon un-shallowing, forcing us to re-discover\n> > the parents when they are no longer hidden behind a graft.\n> >\n> > The latter seems like the most complete feasible fix. The former should\n> > work fine to address this case, but I wonder if there are other\n> > call-sites that are affected by this behavior. My hunch is that this is\n> > a unique case, since it requires going from shallow to unshallow in the\n> > same process.\n>\n> The latter would solve issues that could arise outside of the commit-graph\n> space. But it also presents an opportunity for another gap if someone edits\n> the shallow logic without putting in the proper guards.\n>\n> To be extra safe, I'd be in favor of adding an \"if (grafts_ever_existed)\"\n> condition in commit_graph_compatible() based on a global that is assigned\n> a non-zero value whenever grafts are loaded at any point in the process,\n> mostly because it would be easy to guarantee that it is safe. It could\n> even be localized to the repository struct.\n>\n> > I have yet to create a smaller test case, but the following should be\n> > sufficient to dump the cache of parsed objects upon shallowing or\n> > un-shallowing:\n> >\n> > diff --git a/shallow.c b/shallow.c\n> > index b826de9b67..06db857f53 100644\n> > --- a/shallow.c\n> > +++ b/shallow.c\n> > @@ -90,6 +90,9 @@ static 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> > +\tparsed_object_pool_clear(r->parsed_objects);\n> > +\tr->parsed_objects = parsed_object_pool_new();\n> > }\n> >\n> > int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\n> >\n> > Is this something we want to go forward with? Are there some\n> > far-reaching implications that I'm missing?\n>\n> I'd like to see the extra-careful check, in addition to this one. This\n> is such a rarely-used and narrowly-tested case that we need to be really\n> really careful to avoid regressions.\n\nI'm a little confused at which suggestion you're in favor of ;-). For\nclarity, are you suggesting that we add a new 'r->grafts_ever_existed'\nbit in addition to doing a hard reset of the object pool?\n\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"399091","messageId":"20200603205151.GC253041@google.com","threadId":"53278","inReplyTo":"20200603051631.GA20678@syl.local","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-06-03T20:51:51Z","receivedAt":"2020-06-03T20:51:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n\n> Ah, this only sort of has to do with the object cache. In\n> 'parse_commit_buffer()' we stop parsing parents in the case that the\n> repository is shallow (this goes back to 7f3140cd23 (git repack: keep\n> commits hidden by a graft, 2009-07-23)).\n\nAh, good analysis. (In fact, the behavior is older: it's from\n5da5c8f4cf4 (Teach parse_commit_buffer about grafting., 2005-07-30).)\nSo this is additional \"cached\" data that needs to be invalidated by\nreset_repository_shallow.\n\nSo the question is, what other information falls into that category?\n\n[...]\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -90,6 +90,9 @@ static 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(nit: the above two lines wouldn't be needed if r->parsed_objects is\nbeing thrown away.)\n\n> +\tparsed_object_pool_clear(r->parsed_objects);\n> +\tr->parsed_objects = parsed_object_pool_new();\n> }\n\nShallows don't affect the ref store. They only affect object walks.\nSo r->parsed_objects does seem like the only place that could be\naffected.\n\nThat said, with this change I'd worry about use-after-free from any\nexisting references to objects in the pool.\n\nStepping back, what I think I would like to see is to *not* have\ngrafts and shallow state affect the in-memory persisted parsed\nobjects. Instead, act as an overlay in accessors that traverse over\nthem.\n\nLacking that, I like the idea of a \"dirty bit\" that gets written as\nsoon as we have started lying in the parsed object pool. Something\nlike this. What do you think?\n\ndiff --git i/commit-graph.c w/commit-graph.c\nindex 2ff042fbf4f..84b49ce903b 100644\n--- i/commit-graph.c\n+++ w/commit-graph.c\n@@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)\n \t}\n \n \tprepare_commit_graft(r);\n-\tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n+\tif (r->parsed_objects &&\n+\t (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))\n \t\treturn 0;\n \tif (is_repository_shallow(r))\n \t\treturn 0;\ndiff --git i/commit.c w/commit.c\nindex 87686a7055b..762f09e53ae 100644\n--- i/commit.c\n+++ w/commit.c\n@@ -423,6 +423,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tpptr = &item->parents;\n \n \tgraft = lookup_commit_graft(r, &item->object.oid);\n+\tif (graft)\n+\t\tr->parsed_objects->substituted_parent = 1;\n \twhile (bufptr + parent_entry_len < tail && !memcmp(bufptr, \"parent \", 7)) {\n \t\tstruct commit *new_parent;\n \n@@ -447,6 +449,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tif (graft) {\n \t\tint i;\n \t\tstruct commit *new_parent;\n+\n \t\tfor (i = 0; i < graft->nr_parent; i++) {\n \t\t\tnew_parent = lookup_commit(r,\n \t\t\t\t\t\t &graft->parent[i]);\ndiff --git i/object.h w/object.h\nindex b22328b8383..db02fdcd6b2 100644\n--- i/object.h\n+++ w/object.h\n@@ -26,6 +26,7 @@ struct parsed_object_pool {\n \tchar *alternate_shallow_file;\n \n \tint commit_graft_prepared;\n+\tint substituted_parent;\n \n \tstruct buffer_slab *buffer_slab;\n };\n\nThanks,\nJonathan\n"},{"id":"399093","messageId":"20200603212336.GD253041@google.com","threadId":"53278","inReplyTo":"1253efb6-f1bc-0a16-68e3-c1bc07e1bc18@gmail.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-06-03T21:23:36Z","receivedAt":"2020-06-03T21:23:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDerrick Stolee wrote:\n> On 6/3/2020 1:16 AM, Taylor Blau wrote:\n\n>> * Keep the shallow bit sticky, at least for fetch.writeCommitGraph\n>> (i.e., pretend as if fetch.writecommitgraph=0 in the case of\n>> '--unshallow').\n>\n> I'm in favor of this option, if possible. Anything that alters the\n> commit history in-memory at any point in the Git process is unsafe to\n> combine with a commit-graph read _or_ write. I'm sorry that the guards\n> in commit_graph_compatible() are not enough here.\n\nAs described in [1], I agree --- this kind of \"dirty bit\" is a good\noption and seems like the right thing to do here.\n\nAnd I'm glad you said read _or_ write here. I hadn't realized that\nthis check already applies for reads, and I'm very happy it does.\n\n[...]\n>> * Dump the object cache upon un-shallowing, forcing us to re-discover\n>> the parents when they are no longer hidden behind a graft.\n>>\n>> The latter seems like the most complete feasible fix. The former should\n>> work fine to address this case, but I wonder if there are other\n>> call-sites that are affected by this behavior. My hunch is that this is\n>> a unique case, since it requires going from shallow to unshallow in the\n>> same process.\n>\n> The latter would solve issues that could arise outside of the commit-graph\n> space. But it also presents an opportunity for another gap if someone edits\n> the shallow logic without putting in the proper guards.\n\nThis, however, I don't agree with.\n\nI'm a strong believer in having clear invariants --- without them,\ncode can only be understood empirically, and\nhttps://ieeexplore.ieee.org/document/9068304 describes how painful of\na world that can be.\n\nSo because shallow is specifically about changing the set of parents\nin objects used for traversal, I want to make sure that we as\nreviewers will push back on any potential other new meaning of\nshallow. *If* we had a safe way to invalidate the object cache, it\nwould be sufficient and would be the right thing to do. As described\nin [1], we unfortunately don't have such a thing.\n\nOkay, that's all an aside. Now for the actual reason I'm replying:\n\nI had been wondering why this wasn't caught at read time, but of\ncourse --unshallow clears away the shallows so there was no way for\nthat to happen. So what I wonder is, why isn't this caught by fsck?\nCan \"git fsck\" run \"git commit-graph verify\" automatically and include\nits result as part of what it reports?\n\nThanks,\nJonathan\n\n[1] https://lore.kernel.org/git/20200603205151.GC253041@google.com/\n"},{"id":"399094","messageId":"20200603221453.GA36237@syl.local","threadId":"53278","inReplyTo":"20200603205151.GC253041@google.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-03T22:14:53Z","receivedAt":"2020-06-03T22:14:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:\n> Taylor Blau wrote:\n>\n> > Ah, this only sort of has to do with the object cache. In\n> > 'parse_commit_buffer()' we stop parsing parents in the case that the\n> > repository is shallow (this goes back to 7f3140cd23 (git repack: keep\n> > commits hidden by a graft, 2009-07-23)).\n>\n> Ah, good analysis. (In fact, the behavior is older: it's from\n> 5da5c8f4cf4 (Teach parse_commit_buffer about grafting., 2005-07-30).)\n> So this is additional \"cached\" data that needs to be invalidated by\n> reset_repository_shallow.\n>\n> So the question is, what other information falls into that category?\n>\n> [...]\n> > --- a/shallow.c\n> > +++ b/shallow.c\n> > @@ -90,6 +90,9 @@ static 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> (nit: the above two lines wouldn't be needed if r->parsed_objects is\n> being thrown away.)\n\nRight, thanks. I don't think that it matters since you point out a\nlegitimate issue with dangling references, but serves me right for\nworking on this so late at night ;-).\n\n> > +\tparsed_object_pool_clear(r->parsed_objects);\n> > +\tr->parsed_objects = parsed_object_pool_new();\n> > }\n>\n> Shallows don't affect the ref store. They only affect object walks.\n> So r->parsed_objects does seem like the only place that could be\n> affected.\n>\n> That said, with this change I'd worry about use-after-free from any\n> existing references to objects in the pool.\n>\n> Stepping back, what I think I would like to see is to *not* have\n> grafts and shallow state affect the in-memory persisted parsed\n> objects. Instead, act as an overlay in accessors that traverse over\n> them.\n>\n> Lacking that, I like the idea of a \"dirty bit\" that gets written as\n> soon as we have started lying in the parsed object pool. Something\n> like this. What do you think?\n>\n> diff --git i/commit-graph.c w/commit-graph.c\n> index 2ff042fbf4f..84b49ce903b 100644\n> --- i/commit-graph.c\n> +++ w/commit-graph.c\n> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)\n> \t}\n>\n> \tprepare_commit_graft(r);\n> -\tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n> +\tif (r->parsed_objects &&\n> +\t (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))\n\nThis is a little tricky. Why would we set substituted_parent without\nalso incrementing grafts_nr? That seems like the real bug here: if we\nincremented grafts_nr, then we would return a non-zero value from\n'commit_graph_compatible' and rightly stop even without this sticky-bit.\n\nI don't quite understand this myself. If it's an oversight, it's a\nremarkably long-lived one. Do you have a better sense of this?\n\n> \t\treturn 0;\n> \tif (is_repository_shallow(r))\n> \t\treturn 0;\n> diff --git i/commit.c w/commit.c\n> index 87686a7055b..762f09e53ae 100644\n> --- i/commit.c\n> +++ w/commit.c\n> @@ -423,6 +423,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n> \tpptr = &item->parents;\n>\n> \tgraft = lookup_commit_graft(r, &item->object.oid);\n> +\tif (graft)\n> +\t\tr->parsed_objects->substituted_parent = 1;\n> \twhile (bufptr + parent_entry_len < tail && !memcmp(bufptr, \"parent \", 7)) {\n> \t\tstruct commit *new_parent;\n>\n> @@ -447,6 +449,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n> \tif (graft) {\n> \t\tint i;\n> \t\tstruct commit *new_parent;\n> +\n\nNit: unnecessary whitespace change, but I doubt it really matters much.\n\n> \t\tfor (i = 0; i < graft->nr_parent; i++) {\n> \t\t\tnew_parent = lookup_commit(r,\n> \t\t\t\t\t\t &graft->parent[i]);\n> diff --git i/object.h w/object.h\n> index b22328b8383..db02fdcd6b2 100644\n> --- i/object.h\n> +++ w/object.h\n> @@ -26,6 +26,7 @@ struct parsed_object_pool {\n> \tchar *alternate_shallow_file;\n>\n> \tint commit_graft_prepared;\n> +\tint substituted_parent;\n>\n> \tstruct buffer_slab *buffer_slab;\n> };\n>\n> Thanks,\n> Jonathan\n\nThanks,\nTaylor\n"},{"id":"399096","messageId":"20200603230611.GA1925@gmail.com","threadId":"53278","inReplyTo":"20200603221453.GA36237@syl.local","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-06-03T23:06:11Z","receivedAt":"2020-06-03T23:06:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n> On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:\n\n>> --- i/commit-graph.c\n>> +++ w/commit-graph.c\n>> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)\n>> \t}\n>>\n>> \tprepare_commit_graft(r);\n>> -\tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n>> +\tif (r->parsed_objects &&\n>> +\t (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))\n>\n> This is a little tricky. Why would we set substituted_parent without\n> also incrementing grafts_nr? That seems like the real bug here: if we\n> incremented grafts_nr, then we would return a non-zero value from\n> 'commit_graph_compatible' and rightly stop even without this sticky-bit.\n> \n> I don't quite understand this myself. If it's an oversight, it's a\n> remarkably long-lived one. Do you have a better sense of this?\n\nThe idea here is to check for two different things:\n\n 1. Do we have grafts (either from a grafts file or from a shallow\n file)? If so, this repository is not commit graph compatible\n because we could encounter one of them.\n\n 2. Have cached parsed objects taken any history modifications into\n account? If so, this in-memory state is not commit graph\n compatible because we could encounter one of them.\n\nThe check (1) might seem sufficient if the set of grafts is constant\nfor the lifetime of a git process. But since 37b9dcabfc (shallow.c:\nuse '{commit,rollback}_shallow_file', 2020-04-22), it is not constant\nfor the process lifetime, so we need the check (2) as well.\n\nWe might want a similar check for replace refs as well some day, but\nnot today (there is not a way to remove entries from replace_map\nduring the life of a process).\n\nI can try sending a proper patch with commit message and tests\ntomorrow morning (or if someone else takes care of it, that's fine,\ntoo). Thanks, both, for your help --- it was nice seeing a clear\nexplanation of the cause already figured out and explained when I woke\nup.\n\nRegards,\nJonathan\n"},{"id":"399129","messageId":"20200604174542.GA58239@syl.local","threadId":"53278","inReplyTo":"20200603230611.GA1925@gmail.com","subject":"Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-04T17:45:42Z","receivedAt":"2020-06-04T17:45:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Jonathan,\n\nOn Wed, Jun 03, 2020 at 04:06:11PM -0700, Jonathan Nieder wrote:\n> Taylor Blau wrote:\n> > On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:\n>\n> >> --- i/commit-graph.c\n> >> +++ w/commit-graph.c\n> >> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)\n> >> \t}\n> >>\n> >> \tprepare_commit_graft(r);\n> >> -\tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n> >> +\tif (r->parsed_objects &&\n> >> +\t (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))\n> >\n> > This is a little tricky. Why would we set substituted_parent without\n> > also incrementing grafts_nr? That seems like the real bug here: if we\n> > incremented grafts_nr, then we would return a non-zero value from\n> > 'commit_graph_compatible' and rightly stop even without this sticky-bit.\n> >\n> > I don't quite understand this myself. If it's an oversight, it's a\n> > remarkably long-lived one. Do you have a better sense of this?\n>\n> The idea here is to check for two different things:\n>\n> 1. Do we have grafts (either from a grafts file or from a shallow\n> file)? If so, this repository is not commit graph compatible\n> because we could encounter one of them.\n>\n> 2. Have cached parsed objects taken any history modifications into\n> account? If so, this in-memory state is not commit graph\n> compatible because we could encounter one of them.\n\nAh, breaking it up like this makes it clearer. The gist is that it is\npossible to generate a situation which has (1) no grafts explicitly, but\n(2) *does* have history modifications.\n\n> The check (1) might seem sufficient if the set of grafts is constant\n> for the lifetime of a git process. But since 37b9dcabfc (shallow.c:\n> use '{commit,rollback}_shallow_file', 2020-04-22), it is not constant\n> for the process lifetime, so we need the check (2) as well.\n>\n> We might want a similar check for replace refs as well some day, but\n> not today (there is not a way to remove entries from replace_map\n> during the life of a process).\n\nRight.\n\n> I can try sending a proper patch with commit message and tests\n> tomorrow morning (or if someone else takes care of it, that's fine,\n> too). Thanks, both, for your help --- it was nice seeing a clear\n> explanation of the cause already figured out and explained when I woke\n> up.\n\nIf you are interested, by all means -- I'd certainly be appreciative.\nI'm on vacation next week, and so it may be better if you are able to\nshepherd the patches through. Otherwise, I'd be happy to do it the week\nafter when I get back.\n\n> Regards,\n> Jonathan\n\nThanks,\nTaylor\n"}]}