{"thread":{"id":"53810","subject":"[PATCH] commit.c: don't persist substituted parents when unshallowing","startedAt":"2020-07-07T14:42:27Z","lastAt":"2020-07-09T01:21:39Z","messageCount":8,"participants":["Taylor Blau","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401120","messageId":"82939831ad88f7750b1d024b2031f688ecdf6755.1594132839.git.me@ttaylorr.com","threadId":"53810","inReplyTo":null,"subject":"[PATCH] commit.c: don't persist substituted parents when unshallowing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-07T14:42:22Z","receivedAt":"2020-07-07T14:42:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 37b9dcabfc (shallow.c: use '{commit,rollback}_shallow_file',\n2020-04-22), Git learned how to reset stat-validity checks for the\n'$GIT_DIR/shallow' file, allowing it to change between a shallow and\nnon-shallow state in the same process (e.g., in the case of 'git fetch\n--unshallow').\n\nHowever, 37b9dcabfc does not alter or remove any grafts nor substituted\nparents. This produces a problem when unshallowing if another part of\nthe code relies on the un-grafted and/or non-substituted parentage for\ncommits after, say, fetching.\n\nThis can arise 'fetch.writeCommitGraph' is true. Ordinarily (and\ncertainly previous to 37b9dcabfc), 'commit_graph_compatible()' would\nreturn 0, indicating that the repository should not generate\ncommit-graphs (since at one point in the same process it was shallow).\nBut with 37b9dcabfc, that check succeeds.\n\nThis is where the bug occurs: even though the repository isn't shallow\nany longer (that is, we have all of the objects), the in-core\nrepresentation of those objects still has munged parents at the shallow\nboundaries. If a commit-graph write proceeds, we will use the incorrect\nparentage, producing wrong results.\n\n(Prior to this patch, there were two ways of fixing this: either (1)\nset 'fetch.writeCommitGraph' to 'false', or (2) drop the commit-graph\nafter unshallowing).\n\nOne way to fix this would be to reset the parsed object pool entirely\n(flushing the cache and thus preventing subsequent reads from modifying\ntheir parents) after unshallowing. This can produce a problem when\ncallers have a now-stale reference to the old pool, and so this patch\nimplements a different approach. Instead, we attach a new bit to the\npool, 'substituted_parent' which indicates if the repository *ever*\nstored a commit which had its parents modified (i.e., the shallow\nboundary *before* unshallowing).\n\nThis bit is sticky, since all subsequent reads after modifying a\ncommit's parent are unreliable when unshallowing. This patch modifies\nthe check in 'commit_graph_compatible' to take this bit into account,\nand correctly avoid generating commit-graphs in this case.\n\nHelped-by: Derrick Stolee \nHelped-by: Jonathan Nieder \nReported-by: Jay Conrod \nSigned-off-by: Taylor Blau \n---\nThis is a follow-up to Jonathan Nieder's recent message; this patch\nfixes the persistent-shallow issue originally reported by Jay Conrod in:\n\n https://lore.kernel.org/git/20200603034213.GB253041@google.com/\n\nLike Jonathan, I am also late to send this with -rc0 so close around the\ncorner. I think that this *could* wait until v2.28.1 or v2.29.0 since\nfetch.writeCommitGraph is no longer implied by feature.experimental, but\nI figure that it is probably better to get this into v2.28.0 since it\nfixes the issue once and for all, so long as there is consensus that the\npatch is good.\n\nThanks in advance for a review.\n\n commit-graph.c | 3 ++-\n commit.c | 2 ++\n object.h | 1 +\n t/t5537-fetch-shallow.sh | 14 ++++++++++++++\n 4 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex fdd1c4fa7c..328ab06fd4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -203,7 +203,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 a/commit.c b/commit.c\nindex 43d29a800d..7128895c3a 100644\n--- a/commit.c\n+++ b/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\ndiff --git a/object.h b/object.h\nindex 38dc2d5a6c..96a2105859 100644\n--- a/object.h\n+++ b/object.h\n@@ -25,6 +25,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 };\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex d427a2d7f7..a55202d2d3 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -81,6 +81,20 @@ test_expect_success 'fetch --unshallow from shallow clone' '\n \t)\n '\n\n+test_expect_success 'fetch --unshallow from a full clone' '\n+\tgit clone --no-local --depth=2 .git shallow3 &&\n+\t(\n+\tcd shallow3 &&\n+\tgit log --format=%s >actual &&\n+\ttest_write_lines 4 3 >expect &&\n+\ttest_cmp expect actual &&\n+\tgit -c fetch.writeCommitGraph fetch --unshallow &&\n+\tgit log origin/master --format=%s >actual &&\n+\ttest_write_lines 4 3 2 1 >expect &&\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'fetch something upstream has but hidden by clients shallow boundaries' '\n \t# the blob \"1\" is available in .git but hidden by the\n \t# shallow2/.git/shallow and it should be resent\n--\n2.27.0.225.g9fa765a71d\n"},{"id":"401121","messageId":"20200707144338.GA26342@syl.lan","threadId":"53810","inReplyTo":"82939831ad88f7750b1d024b2031f688ecdf6755.1594132839.git.me@ttaylorr.com","subject":"Re: [PATCH] commit.c: don't persist substituted parents when unshallowing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-07T14:43:38Z","receivedAt":"2020-07-07T14:43:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"...oops; I was scripting around my git-who script this morning, and\nclearly munged a few of the recipient's emails. They are fixed in this\nemail.\n\nOn Tue, Jul 07, 2020 at 10:42:22AM -0400, Taylor Blau wrote:\n> In 37b9dcabfc (shallow.c: use '{commit,rollback}_shallow_file',\n> 2020-04-22), Git learned how to reset stat-validity checks for the\n> '$GIT_DIR/shallow' file, allowing it to change between a shallow and\n> non-shallow state in the same process (e.g., in the case of 'git fetch\n> --unshallow').\n>\n> However, 37b9dcabfc does not alter or remove any grafts nor substituted\n> parents. This produces a problem when unshallowing if another part of\n> the code relies on the un-grafted and/or non-substituted parentage for\n> commits after, say, fetching.\n>\n> This can arise 'fetch.writeCommitGraph' is true. Ordinarily (and\n> certainly previous to 37b9dcabfc), 'commit_graph_compatible()' would\n> return 0, indicating that the repository should not generate\n> commit-graphs (since at one point in the same process it was shallow).\n> But with 37b9dcabfc, that check succeeds.\n>\n> This is where the bug occurs: even though the repository isn't shallow\n> any longer (that is, we have all of the objects), the in-core\n> representation of those objects still has munged parents at the shallow\n> boundaries. If a commit-graph write proceeds, we will use the incorrect\n> parentage, producing wrong results.\n>\n> (Prior to this patch, there were two ways of fixing this: either (1)\n> set 'fetch.writeCommitGraph' to 'false', or (2) drop the commit-graph\n> after unshallowing).\n>\n> One way to fix this would be to reset the parsed object pool entirely\n> (flushing the cache and thus preventing subsequent reads from modifying\n> their parents) after unshallowing. This can produce a problem when\n> callers have a now-stale reference to the old pool, and so this patch\n> implements a different approach. Instead, we attach a new bit to the\n> pool, 'substituted_parent' which indicates if the repository *ever*\n> stored a commit which had its parents modified (i.e., the shallow\n> boundary *before* unshallowing).\n>\n> This bit is sticky, since all subsequent reads after modifying a\n> commit's parent are unreliable when unshallowing. This patch modifies\n> the check in 'commit_graph_compatible' to take this bit into account,\n> and correctly avoid generating commit-graphs in this case.\n>\n> Helped-by: Derrick Stolee \n> Helped-by: Jonathan Nieder \n> Reported-by: Jay Conrod \n> Signed-off-by: Taylor Blau \n> ---\n> This is a follow-up to Jonathan Nieder's recent message; this patch\n> fixes the persistent-shallow issue originally reported by Jay Conrod in:\n>\n> https://lore.kernel.org/git/20200603034213.GB253041@google.com/\n>\n> Like Jonathan, I am also late to send this with -rc0 so close around the\n> corner. I think that this *could* wait until v2.28.1 or v2.29.0 since\n> fetch.writeCommitGraph is no longer implied by feature.experimental, but\n> I figure that it is probably better to get this into v2.28.0 since it\n> fixes the issue once and for all, so long as there is consensus that the\n> patch is good.\n>\n> Thanks in advance for a review.\n>\n> commit-graph.c | 3 ++-\n> commit.c | 2 ++\n> object.h | 1 +\n> t/t5537-fetch-shallow.sh | 14 ++++++++++++++\n> 4 files changed, 19 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index fdd1c4fa7c..328ab06fd4 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -203,7 +203,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;\n> diff --git a/commit.c b/commit.c\n> index 43d29a800d..7128895c3a 100644\n> --- a/commit.c\n> +++ b/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> diff --git a/object.h b/object.h\n> index 38dc2d5a6c..96a2105859 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -25,6 +25,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> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index d427a2d7f7..a55202d2d3 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -81,6 +81,20 @@ test_expect_success 'fetch --unshallow from shallow clone' '\n> \t)\n> '\n>\n> +test_expect_success 'fetch --unshallow from a full clone' '\n> +\tgit clone --no-local --depth=2 .git shallow3 &&\n> +\t(\n> +\tcd shallow3 &&\n> +\tgit log --format=%s >actual &&\n> +\ttest_write_lines 4 3 >expect &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c fetch.writeCommitGraph fetch --unshallow &&\n> +\tgit log origin/master --format=%s >actual &&\n> +\ttest_write_lines 4 3 2 1 >expect &&\n> +\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_expect_success 'fetch something upstream has but hidden by clients shallow boundaries' '\n> \t# the blob \"1\" is available in .git but hidden by the\n> \t# shallow2/.git/shallow and it should be resent\n> --\n> 2.27.0.225.g9fa765a71d\nThanks,\nTaylor\n"},{"id":"401179","messageId":"20200708054112.GA118756@google.com","threadId":"53810","inReplyTo":"82939831ad88f7750b1d024b2031f688ecdf6755.1594132839.git.me@ttaylorr.com","subject":"Re: [PATCH] commit.c: don't persist substituted parents when unshallowing","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-08T05:41:12Z","receivedAt":"2020-07-08T05:41:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n\n> In 37b9dcabfc (shallow.c: use '{commit,rollback}_shallow_file',\n> 2020-04-22), Git learned how to reset stat-validity checks for the\n> '$GIT_DIR/shallow' file, allowing it to change between a shallow and\n> non-shallow state in the same process (e.g., in the case of 'git fetch\n> --unshallow').\n>\n> However, 37b9dcabfc does not alter or remove any grafts nor substituted\n> parents.\n[...]\n> even though the repository isn't shallow\n> any longer (that is, we have all of the objects), the in-core\n> representation of those objects still has munged parents at the shallow\n> boundaries. If a commit-graph write proceeds, we will use the incorrect\n> parentage, producing wrong results.\n>\n> (Prior to this patch, there were two ways of fixing this: either (1)\n> set 'fetch.writeCommitGraph' to 'false', or (2) drop the commit-graph\n> after unshallowing).\n\nnit: It wasn't obvious to me on first reading which patch \"this patch\"\nrefers to --- does it mean 37b9dcabfc or the patch I am reading?\n\nThe approach described in Documentation/SubmittingPatches is to treat\nthe commit message kind of like a bug report:\n\n . start with the problem the change tries to solve, i.e. what is wrong\n with the current code without the change.\n\n . request the change, justifying the way it solves the problem, i.e.\n why the result with the change is better.\n\n . then move on to alternate solutions considered but discarded, if any.\n\nHere, I think that would be similar to what you have written:\n\n Since 37b9dcabfc (shallow.c: [...]), Git knows how to reset\n stat-validity checks for the $GIT_DIR/shallow file, allowing it to\n [etc]. However, when $GIT_DIR/shallow changes, Git does not alter or\n remove any grafts nor substituted parents in memory.\n\n This comes up in a \"git fetch --unshallow\" with fetch.writeCommitGraph\n set to true. Ordinarily in a shallow repository (and before 37b9dcabfc,\n even in this case), commit_graph_compatible() would return false,\n indicating that the repository should not be used to write a\n commit-graphs (since commit-graph files cannot represent a shallow\n history). But since 37b9dcabfc, in an --unshallow operation that check\n succeeds.\n\n Thus even though the repository isn't shallow any longer (that is, we\n have all of the objects), the in-core representation of those objects\n [...]. When the commit-graph write proceeds, we use the incorrect\n parentage, producing wrong results.\n\n There are two ways for a user to work around this: either (1) set\n 'fetch.writeCommitGraph' to 'false', or (2) drop the commit-graph\n after unshallowing.\n\n> One way to fix this would be to reset the parsed object pool entirely\n> (flushing the cache and thus preventing subsequent reads from modifying\n> their parents) after unshallowing. This can produce a problem when\n\nnit: seems easier to read with s/This can/That would/, since it's\ndescribing the road not taken\n\n> callers have a now-stale reference to the old pool, and so this patch\n> implements a different approach. Instead, we attach a new bit to the\n\nnit: s/we attach/attach/, to avoid ambiguity with the previous part of\nthe commit message that describes the current, unpatched behavior.\n\n> pool, 'substituted_parent' which indicates if the repository *ever*\n> stored a commit which had its parents modified (i.e., the shallow\n> boundary *before* unshallowing).\n>\n> This bit is sticky, since all subsequent reads after modifying a\n> commit's parent are unreliable when unshallowing. This patch modifies\n> the check in 'commit_graph_compatible' to take this bit into account,\n> and correctly avoid generating commit-graphs in this case.\n\nlikewise:\n\n This bit needs to be sticky because [...]\n\n> Helped-by: Derrick Stolee \n> Helped-by: Jonathan Nieder \n> Reported-by: Jay Conrod \n> Signed-off-by: Taylor Blau \n> ---\n> This is a follow-up to Jonathan Nieder's recent message; this patch\n> fixes the persistent-shallow issue originally reported by Jay Conrod in:\n>\n> https://lore.kernel.org/git/20200603034213.GB253041@google.com/\n>\n> Like Jonathan, I am also late to send this with -rc0 so close around the\n> corner. I think that this *could* wait until v2.28.1 or v2.29.0 since\n> fetch.writeCommitGraph is no longer implied by feature.experimental, but\n> I figure that it is probably better to get this into v2.28.0 since it\n> fixes the issue once and for all, so long as there is consensus that the\n> patch is good.\n\nI'm of course inclined to like it, so before reading through the rest my\npreference would be to include both patches (this fix and the patch I\nsent to reduce user exposure) in -rc0. But let's see if the patch\nchanges my opinion. ;-)\n\n[...]\n> commit-graph.c | 3 ++-\n> commit.c | 2 ++\n> object.h | 1 +\n> t/t5537-fetch-shallow.sh | 14 ++++++++++++++\n> 4 files changed, 19 insertions(+), 1 deletion(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index fdd1c4fa7c..328ab06fd4 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -203,7 +203,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\nA subtlety: we can't *just* check for substituted parents, because\nthere could be a graft affecting a commit we haven't parsed yet. So\nwe'd have to check for both grafts yet to be applied (grafts_nr) and\ngrafts already applied (substituted_parent).\n\nGood.\n\n[...]\n> --- a/commit.c\n> +++ b/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\nThis applies right away at the only place in commit parsing where\ngrafts can be discovered.\n\nThe only other callers to lookup_commit_graft are get_shallow_commits\nand prepare_shallow_info. Those are protocol code and they don't\naffect the in-core history.\n\n[...]\n> --- a/object.h\n> +++ b/object.h\n> @@ -25,6 +25,7 @@ struct parsed_object_pool {\n> \tchar *alternate_shallow_file;\n> \n> \tint commit_graft_prepared;\n> +\tint substituted_parent;\n\nparsed_object_pool is zero-initialized on creation (in\nparsed_object_pool_new), so this gets correctly initialized to false.\n\n[...]\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -81,6 +81,20 @@ test_expect_success 'fetch --unshallow from shallow clone' '\n> \t)\n> '\n> \n> +test_expect_success 'fetch --unshallow from a full clone' '\n> +\tgit clone --no-local --depth=2 .git shallow3 &&\n> +\t(\n> +\tcd shallow3 &&\n> +\tgit log --format=%s >actual &&\n> +\ttest_write_lines 4 3 >expect &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c fetch.writeCommitGraph fetch --unshallow &&\n> +\tgit log origin/master --format=%s >actual &&\n> +\ttest_write_lines 4 3 2 1 >expect &&\n> +\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n\nThe indentation is odd here, but it's consistent with the rest of the\nfile.\n\nA fairly straightforward test that demonstrates the bug being fixed and\nonly relies on what was set up in the initial 'setup' test (4 commits).\nThanks for tying that loose end.\n\nWith or without commit message tweaks along the lines described above,\nReviewed-by: Jonathan Nieder \n\nThanks for your thoughtful work, as always.\n"},{"id":"401197","messageId":"xmqqk0zd20cf.fsf@gitster.c.googlers.com","threadId":"53810","inReplyTo":"20200708054112.GA118756@google.com","subject":"Re: [PATCH] commit.c: don't persist substituted parents when unshallowing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-08T20:55:28Z","receivedAt":"2020-07-08T20:55:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> A fairly straightforward test that demonstrates the bug being fixed and\n> only relies on what was set up in the initial 'setup' test (4 commits).\n> Thanks for tying that loose end.\n>\n> With or without commit message tweaks along the lines described above,\n> Reviewed-by: Jonathan Nieder \n>\n> Thanks for your thoughtful work, as always.\n\nThanks for a nicely written review. It would be very nice if we can\nsee a reroll before I have to tag -rc0.\n\nThanks.\n\n"},{"id":"401199","messageId":"1f769bbbb4f1b4ad67d29ee7b3e5282446e4cc0f.1594242582.git.me@ttaylorr.com","threadId":"53810","inReplyTo":"20200707144338.GA26342@syl.lan","subject":"[PATCH v2] commit.c: don't persist substituted parents when unshallowing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-08T21:10:53Z","receivedAt":"2020-07-08T21:10:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since 37b9dcabfc (shallow.c: use '{commit,rollback}_shallow_file',\n2020-04-22), Git knows how to reset stat-validity checks for the\n$GIT_DIR/shallow file, allowing it to change between a shallow and\nnon-shallow state in the same process (e.g., in the case of 'git fetch\n--unshallow').\n\nHowever, when $GIT_DIR/shallow changes, Git does not alter or remove any\ngrafts (nor substituted parents) in memory.\n\nThis comes up in a \"git fetch --unshallow\" with fetch.writeCommitGraph\nset to true. Ordinarily in a shallow repository (and before 37b9dcabfc,\neven in this case), commit_graph_compatible() would return false,\nindicating that the repository should not be used to write a\ncommit-graphs (since commit-graph files cannot represent a shallow\nhistory). But since 37b9dcabfc, in an --unshallow operation that check\nsucceeds.\n\nThus even though the repository isn't shallow any longer (that is, we\nhave all of the objects), the in-core representation of those objects\nstill has munged parents at the shallow boundaries. When the\ncommit-graph write proceeds, we use the incorrect parentage, producing\nwrong results.\n\nThere are two ways for a user to work around this: either (1) set\n'fetch.writeCommitGraph' to 'false', or (2) drop the commit-graph after\nunshallowing.\n\nOne way to fix this would be to reset the parsed object pool entirely\n(flushing the cache and thus preventing subsequent reads from modifying\ntheir parents) after unshallowing. That would produce a problem when\ncallers have a now-stale reference to the old pool, and so this patch\nimplements a different approach. Instead, attach a new bit to the pool,\n'substituted_parent', which indicates if the repository *ever* stored a\ncommit which had its parents modified (i.e., the shallow boundary\nprior to unshallowing).\n\nThis bit needs to be sticky because all reads subsequent to modifying a\ncommit's parents are unreliable when unshallowing. Modify the check in\n'commit_graph_compatible' to take this bit into account, and correctly\navoid generating commit-graphs in this case, thus solving the bug.\n\nHelped-by: Derrick Stolee \nHelped-by: Jonathan Nieder \nReported-by: Jay Conrod \nReviewed-by: Jonathan Nieder \nSigned-off-by: Taylor Blau \n---\nThe patch contents are unchanged from v1, but the patch's message is\nwordsmithed with jrn's help. This should be ready for -rc0 now.\n\nRange-diff below for easier review:\n\nRange-diff against v1:\n1: 82939831ad ! 1: 1f769bbbb4 commit.c: don't persist substituted parents when unshallowing\n @@ Metadata\n ## Commit message ##\n commit.c: don't persist substituted parents when unshallowing\n\n - In 37b9dcabfc (shallow.c: use '{commit,rollback}_shallow_file',\n - 2020-04-22), Git learned how to reset stat-validity checks for the\n - '$GIT_DIR/shallow' file, allowing it to change between a shallow and\n + Since 37b9dcabfc (shallow.c: use '{commit,rollback}_shallow_file',\n + 2020-04-22), Git knows how to reset stat-validity checks for the\n + $GIT_DIR/shallow file, allowing it to change between a shallow and\n non-shallow state in the same process (e.g., in the case of 'git fetch\n --unshallow').\n\n - However, 37b9dcabfc does not alter or remove any grafts nor substituted\n - parents. This produces a problem when unshallowing if another part of\n - the code relies on the un-grafted and/or non-substituted parentage for\n - commits after, say, fetching.\n + However, when $GIT_DIR/shallow changes, Git does not alter or remove any\n + grafts (nor substituted parents) in memory.\n\n - This can arise 'fetch.writeCommitGraph' is true. Ordinarily (and\n - certainly previous to 37b9dcabfc), 'commit_graph_compatible()' would\n - return 0, indicating that the repository should not generate\n - commit-graphs (since at one point in the same process it was shallow).\n - But with 37b9dcabfc, that check succeeds.\n + This comes up in a \"git fetch --unshallow\" with fetch.writeCommitGraph\n + set to true. Ordinarily in a shallow repository (and before 37b9dcabfc,\n + even in this case), commit_graph_compatible() would return false,\n + indicating that the repository should not be used to write a\n + commit-graphs (since commit-graph files cannot represent a shallow\n + history). But since 37b9dcabfc, in an --unshallow operation that check\n + succeeds.\n\n - This is where the bug occurs: even though the repository isn't shallow\n - any longer (that is, we have all of the objects), the in-core\n - representation of those objects still has munged parents at the shallow\n - boundaries. If a commit-graph write proceeds, we will use the incorrect\n - parentage, producing wrong results.\n + Thus even though the repository isn't shallow any longer (that is, we\n + have all of the objects), the in-core representation of those objects\n + still has munged parents at the shallow boundaries. When the\n + commit-graph write proceeds, we use the incorrect parentage, producing\n + wrong results.\n\n - (Prior to this patch, there were two ways of fixing this: either (1)\n - set 'fetch.writeCommitGraph' to 'false', or (2) drop the commit-graph\n - after unshallowing).\n + There are two ways for a user to work around this: either (1) set\n + 'fetch.writeCommitGraph' to 'false', or (2) drop the commit-graph after\n + unshallowing.\n\n One way to fix this would be to reset the parsed object pool entirely\n (flushing the cache and thus preventing subsequent reads from modifying\n - their parents) after unshallowing. This can produce a problem when\n + their parents) after unshallowing. That would produce a problem when\n callers have a now-stale reference to the old pool, and so this patch\n - implements a different approach. Instead, we attach a new bit to the\n - pool, 'substituted_parent' which indicates if the repository *ever*\n - stored a commit which had its parents modified (i.e., the shallow\n - boundary *before* unshallowing).\n + implements a different approach. Instead, attach a new bit to the pool,\n + 'substituted_parent', which indicates if the repository *ever* stored a\n + commit which had its parents modified (i.e., the shallow boundary\n + prior to unshallowing).\n\n - This bit is sticky, since all subsequent reads after modifying a\n - commit's parent are unreliable when unshallowing. This patch modifies\n - the check in 'commit_graph_compatible' to take this bit into account,\n - and correctly avoid generating commit-graphs in this case.\n + This bit needs to be sticky because all reads subsequent to modifying a\n + commit's parents are unreliable when unshallowing. Modify the check in\n + 'commit_graph_compatible' to take this bit into account, and correctly\n + avoid generating commit-graphs in this case, thus solving the bug.\n\n Helped-by: Derrick Stolee \n Helped-by: Jonathan Nieder \n Reported-by: Jay Conrod \n + Reviewed-by: Jonathan Nieder \n Signed-off-by: Taylor Blau \n\n ## commit-graph.c ##\n\n commit-graph.c | 3 ++-\n commit.c | 2 ++\n object.h | 1 +\n t/t5537-fetch-shallow.sh | 14 ++++++++++++++\n 4 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex fdd1c4fa7c..328ab06fd4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -203,7 +203,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 a/commit.c b/commit.c\nindex 43d29a800d..7128895c3a 100644\n--- a/commit.c\n+++ b/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\ndiff --git a/object.h b/object.h\nindex 38dc2d5a6c..96a2105859 100644\n--- a/object.h\n+++ b/object.h\n@@ -25,6 +25,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 };\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex d427a2d7f7..a55202d2d3 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -81,6 +81,20 @@ test_expect_success 'fetch --unshallow from shallow clone' '\n \t)\n '\n\n+test_expect_success 'fetch --unshallow from a full clone' '\n+\tgit clone --no-local --depth=2 .git shallow3 &&\n+\t(\n+\tcd shallow3 &&\n+\tgit log --format=%s >actual &&\n+\ttest_write_lines 4 3 >expect &&\n+\ttest_cmp expect actual &&\n+\tgit -c fetch.writeCommitGraph fetch --unshallow &&\n+\tgit log origin/master --format=%s >actual &&\n+\ttest_write_lines 4 3 2 1 >expect &&\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'fetch something upstream has but hidden by clients shallow boundaries' '\n \t# the blob \"1\" is available in .git but hidden by the\n \t# shallow2/.git/shallow and it should be resent\n--\n2.27.0.225.g9fa765a71d\n"},{"id":"401200","messageId":"20200708212210.GA17015@syl.lan","threadId":"53810","inReplyTo":"xmqqk0zd20cf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit.c: don't persist substituted parents when unshallowing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-08T21:22:10Z","receivedAt":"2020-07-08T21:22:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 08, 2020 at 01:55:28PM -0700, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n> > A fairly straightforward test that demonstrates the bug being fixed and\n> > only relies on what was set up in the initial 'setup' test (4 commits).\n> > Thanks for tying that loose end.\n> >\n> > With or without commit message tweaks along the lines described above,\n> > Reviewed-by: Jonathan Nieder \n> >\n> > Thanks for your thoughtful work, as always.\n>\n> Thanks for a nicely written review. It would be very nice if we can\n> see a reroll before I have to tag -rc0.\n\nOf course! I just sent one here[1]. Sorry for the last-minute addition\nto -rc0, and thanks for fast-tracking it down.\n\n> Thanks.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/1f769bbbb4f1b4ad67d29ee7b3e5282446e4cc0f.1594242582.git.me@ttaylorr.com/\n"},{"id":"401211","messageId":"20200709010042.GA8720@google.com","threadId":"53810","inReplyTo":"1f769bbbb4f1b4ad67d29ee7b3e5282446e4cc0f.1594242582.git.me@ttaylorr.com","subject":"Re: [PATCH v2] commit.c: don't persist substituted parents when unshallowing","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-09T01:00:42Z","receivedAt":"2020-07-09T01:00:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n\n> Reported-by: Jay Conrod \n> Reviewed-by: Jonathan Nieder \n> Signed-off-by: Taylor Blau \n> ---\n\nThis is indeed\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"401212","messageId":"xmqqpn95zdnm.fsf@gitster.c.googlers.com","threadId":"53810","inReplyTo":"20200709010042.GA8720@google.com","subject":"Re: [PATCH v2] commit.c: don't persist substituted parents when unshallowing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-09T01:21:33Z","receivedAt":"2020-07-09T01:21:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Taylor Blau wrote:\n>\n>> Reported-by: Jay Conrod \n>> Reviewed-by: Jonathan Nieder \n>> Signed-off-by: Taylor Blau \n>> ---\n>\n> This is indeed\n> Reviewed-by: Jonathan Nieder \n\nThanks, both.\n"}]}