{"thread":{"id":"57579","subject":"[PATCH] shallow: reset commit grafts when shallow is reset","startedAt":"2022-03-17T18:24:57Z","lastAt":"2022-03-18T00:51:54Z","messageCount":3,"participants":["Jonathan Tan","Glen Choo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"451587","messageId":"20220317182448.1633847-1-jonathantanmy@google.com","threadId":"57579","inReplyTo":null,"subject":"[PATCH] shallow: reset commit grafts when shallow is reset","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-03-17T18:24:47Z","receivedAt":"2022-03-17T18:24:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When reset_repository_shallow() is called, Git clears its cache of\nshallow information, so that if shallow information is re-requested, Git\nwill read fresh data from disk instead of reusing its stale cached data.\nHowever, the cache of commit grafts is not likewise cleared, even though\nthere are commit grafts created from shallow information.\n\nThis means that if on-disk shallow information were to be updated and\nthen a commit-graft-using codepath were run (for example, a revision\nwalk), Git would be using stale commit graft information. This can be\nseen from the test in this patch, in which Git performs a revision walk\n(to check for changed submodules) after a fetch with --update-shallow.\n\nTherefore, clear the cache of commit grafts whenever\nreset_repository_shallow() is called.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis is the same bug noticed by Glen Choo, as can be seen by the patch\nhere [1]. (That patch doesn't seem to be carried forward to subsequent\nversions of the patch set, presumably because it is not needed in that\npatch set.)\n\nI have verified that Glen's test also works with the fix in this patch.\nBut I have written a test in t5537-fetch-shallow instead, as I think\nthat this is more about updating shallows than fetching submodules.\n\n[1] https://lore.kernel.org/git/20220215172318.73533-9-chooglen@google.com/\n---\n commit.c                 | 10 ++++++++++\n commit.h                 |  1 +\n shallow.c                |  1 +\n submodule.c              |  1 +\n t/t5537-fetch-shallow.sh |  9 +++++++++\n 5 files changed, 22 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex d400f5dfa2..f8405fe46e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -249,6 +249,16 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)\n \treturn ret;\n }\n \n+void reset_commit_grafts(struct repository *r)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < r->parsed_objects->grafts_nr; i++)\n+\t\tfree(r->parsed_objects->grafts[i]);\n+\tr->parsed_objects->grafts_nr = 0;\n+\tr->parsed_objects->commit_graft_prepared = 0;\n+}\n+\n struct commit_buffer {\n \tvoid *buffer;\n \tunsigned long size;\ndiff --git a/commit.h b/commit.h\nindex 38cc542661..336e0bfa42 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -249,6 +249,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);\n int register_commit_graft(struct repository *r, struct commit_graft *, int);\n void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n+void reset_commit_grafts(struct repository *r);\n \n struct commit *get_fork_point(const char *refname, struct commit *commit);\n \ndiff --git a/shallow.c b/shallow.c\nindex 71e5876f37..e158be58b0 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -90,6 +90,7 @@ 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+\treset_commit_grafts(r);\n }\n \n int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\ndiff --git a/submodule.c b/submodule.c\nindex 5ace18a7d9..7a0515913c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -22,6 +22,7 @@\n #include \"parse-options.h\"\n #include \"object-store.h\"\n #include \"commit-reach.h\"\n+#include \"shallow.h\"\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n static int initialized_fetch_ref_tips;\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 11d5ea54a9..92948de7a0 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -161,6 +161,15 @@ test_expect_success 'fetch --update-shallow' '\n \t)\n '\n \n+test_expect_success 'fetch --update-shallow into a repo with submodules' '\n+\tgit init a-submodule &&\n+\ttest_commit -C a-submodule foo &&\n+\tgit init repo-with-sub &&\n+\tgit -C repo-with-sub submodule add ../a-submodule a-submodule &&\n+\tgit -C repo-with-sub commit -m \"added submodule\" &&\n+\tgit -C repo-with-sub fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n+'\n+\n test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '\n \t(\n \tcd shallow &&\n-- \n2.35.1.894.gb6a874cedc-goog\n\n"},{"id":"451599","messageId":"kl6l4k3wkz5e.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57579","inReplyTo":"20220317182448.1633847-1-jonathantanmy@google.com","subject":"Re: [PATCH] shallow: reset commit grafts when shallow is reset","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-03-17T22:11:25Z","receivedAt":"2022-03-17T22:11:40Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nI'm not an expert on shallows or commit grafts, so I hesitate to give a\n\"Reviewed-by\", but the description of the bug and fix make sense.\n\nJonathan Tan <jonathantanmy@google.com> writes:\n\n> When reset_repository_shallow() is called, Git clears its cache of\n> shallow information, so that if shallow information is re-requested, Git\n> will read fresh data from disk instead of reusing its stale cached data.\n> However, the cache of commit grafts is not likewise cleared, even though\n> there are commit grafts created from shallow information.\n>\n> This means that if on-disk shallow information were to be updated and\n> then a commit-graft-using codepath were run (for example, a revision\n> walk), Git would be using stale commit graft information. This can be\n> seen from the test in this patch, in which Git performs a revision walk\n> (to check for changed submodules) after a fetch with --update-shallow.\n\nHm, as a non-expert, I wasn't able to immediately draw the link between\n\"reset_repository_shallow() doesn't do enough\" to \"we reproduce this bug\nusing fetch --update-shallow with submodules\".\n\nWhat would make this sequence of events clearer to me is describing\nwhy reset_repository_shallow() is called. From reading receive-pack.c\nand shallow.c, I believe we call reset_repository_shallow() whenever a\nshallow file is written (or more specifically, whenever we commit or\nrollback a shallow lock file), invalidating the cache of shallow\ninformation.\n\nOne thing I noticed when writing my test case is that fetch without\n--update-shallow would never run into this bug. But if one reads \nDocumentation/fetch-options.txt:\n\n  --update-shallow::\n    By default when fetching from a shallow repository,\n    `git fetch` refuses refs that require updating\n    .git/shallow. This option updates .git/shallow and accept such\n    refs.\n\nit becomes pretty easy to put two and two together - fetch with\n--update-shallow writes shallow files, clearing the cache of\nshallows (but not commit grafts), so it thinks it has all of the correct\ncommit graft info. It then tries to perform a rev walk and fails.\n\nIt is also worth noting that we only do this rev walk if Git thinks that\nthere are submodules, as indicated by the presence of .gitmodules (or\nafter gc/fetch-unused-submodules, whether .git/modules is nonempty).\nThis would also explain why we never noticed the bug before - it only\noccurs when we have both --update-shallow and submodules.\n\n> Therefore, clear the cache of commit grafts whenever\n> reset_repository_shallow() is called.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This is the same bug noticed by Glen Choo, as can be seen by the patch\n> here [1]. (That patch doesn't seem to be carried forward to subsequent\n> versions of the patch set, presumably because it is not needed in that\n> patch set.)\n>\n> I have verified that Glen's test also works with the fix in this patch.\n> But I have written a test in t5537-fetch-shallow instead, as I think\n> that this is more about updating shallows than fetching submodules.\n\nMakes sense, this is more about shallows.\n\n> [1] https://lore.kernel.org/git/20220215172318.73533-9-chooglen@google.com/\n> ---\n>  commit.c                 | 10 ++++++++++\n>  commit.h                 |  1 +\n>  shallow.c                |  1 +\n>  submodule.c              |  1 +\n>  t/t5537-fetch-shallow.sh |  9 +++++++++\n>  5 files changed, 22 insertions(+)\n>\n> diff --git a/commit.c b/commit.c\n> index d400f5dfa2..f8405fe46e 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -249,6 +249,16 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)\n>  \treturn ret;\n>  }\n>  \n> +void reset_commit_grafts(struct repository *r)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < r->parsed_objects->grafts_nr; i++)\n> +\t\tfree(r->parsed_objects->grafts[i]);\n> +\tr->parsed_objects->grafts_nr = 0;\n> +\tr->parsed_objects->commit_graft_prepared = 0;\n> +}\n> +\n>  struct commit_buffer {\n>  \tvoid *buffer;\n>  \tunsigned long size;\n> diff --git a/commit.h b/commit.h\n> index 38cc542661..336e0bfa42 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -249,6 +249,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);\n>  int register_commit_graft(struct repository *r, struct commit_graft *, int);\n>  void prepare_commit_graft(struct repository *r);\n>  struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n> +void reset_commit_grafts(struct repository *r);\n>  \n>  struct commit *get_fork_point(const char *refname, struct commit *commit);\n>  \n> diff --git a/shallow.c b/shallow.c\n> index 71e5876f37..e158be58b0 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -90,6 +90,7 @@ 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> +\treset_commit_grafts(r);\n>  }\n>  \n\nMakes sense. [un]register_shallow(), register and unregister commit\ngrafts correctly, so it makes sense that reset_repository_shallow()\nshould reset the commit grafts too. I don't see any existing functions\nthat reset commit grafts.\n\n>  int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\n> diff --git a/submodule.c b/submodule.c\n> index 5ace18a7d9..7a0515913c 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -22,6 +22,7 @@\n>  #include \"parse-options.h\"\n>  #include \"object-store.h\"\n>  #include \"commit-reach.h\"\n> +#include \"shallow.h\"\n>  \n>  static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n>  static int initialized_fetch_ref_tips;\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index 11d5ea54a9..92948de7a0 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -161,6 +161,15 @@ test_expect_success 'fetch --update-shallow' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'fetch --update-shallow into a repo with submodules' '\n> +\tgit init a-submodule &&\n> +\ttest_commit -C a-submodule foo &&\n> +\tgit init repo-with-sub &&\n> +\tgit -C repo-with-sub submodule add ../a-submodule a-submodule &&\n> +\tgit -C repo-with-sub commit -m \"added submodule\" &&\n> +\tgit -C repo-with-sub fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*\n> +'\n> +\n>  test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '\n>  \t(\n>  \tcd shallow &&\n> -- \n> 2.35.1.894.gb6a874cedc-goog\n"},{"id":"451611","messageId":"xmqqsfrgcccz.fsf@gitster.g","threadId":"57579","inReplyTo":"20220317182448.1633847-1-jonathantanmy@google.com","subject":"Re: [PATCH] shallow: reset commit grafts when shallow is reset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-18T00:50:52Z","receivedAt":"2022-03-18T00:51:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When reset_repository_shallow() is called, Git clears its cache of\n> shallow information, so that if shallow information is re-requested, Git\n> will read fresh data from disk instead of reusing its stale cached data.\n> However, the cache of commit grafts is not likewise cleared, even though\n> there are commit grafts created from shallow information.\n>\n> This means that if on-disk shallow information were to be updated and\n> then a commit-graft-using codepath were run (for example, a revision\n> walk), Git would be using stale commit graft information. This can be\n> seen from the test in this patch, in which Git performs a revision walk\n> (to check for changed submodules) after a fetch with --update-shallow.\n>\n> Therefore, clear the cache of commit grafts whenever\n> reset_repository_shallow() is called.\n\nBecause this helper function resets not just grafts_nr, but\ncommit_graft_prepared member, the next time prepare_commit_graft()\nis called, we'd recover the grafts (not derived from shallows, but\nread locally from \"info/grafts\") from the file, too, which makes it\nsound right ;-)\n\nAnd the implementation looks quite straight-forward.\n\nWill queue.  Thanks.\n"}]}