{"thread":{"id":"55131","subject":"[PATCH 0/2] Fix fsck --name-objects bug","startedAt":"2021-02-10T18:09:53Z","lastAt":"2021-02-11T00:11:02Z","messageCount":7,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"416646","messageId":"pull.874.git.1612980090.gitgitgadget@gmail.com","threadId":"55131","inReplyTo":null,"subject":"[PATCH 0/2] Fix fsck --name-objects bug","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-10T18:01:28Z","receivedAt":"2021-02-10T18:09:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As described in https://github.com/gitgitgadget/git/pull/873, I managed to\ncorrupt my Git checkout in a rather thorough manner yesterday, and it took\nme a long time to undo the damage. One of my tools for that should have been\ngit fsck --name-objects, but that command produced bogus output: It said\nthat a commit with the name ~2 had a missing tree, but ~2 is not even a\nlegal rev name.\n\nTurns out that this is an ancient bug, and the fact that nobody complained\nabout it suggests to me that the --name-objects probably has exactly one\nuser, and he uses it only every four years, when he manages to hose his Git\ncheckout.\n\nJohannes Schindelin (2):\n t1450: robustify `remove_object()`\n fsck --name-objects: be more careful parsing generation numbers\n\n fsck.c | 5 +++++\n t/t1450-fsck.sh | 26 ++++++++++++--------------\n 2 files changed, 17 insertions(+), 14 deletions(-)\n\n\nbase-commit: 7397ca33730626f682845f8691b39c305535611e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-874%2Fdscho%2Ffix-fsck-name-objects-generation-parsing-bug-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-874/dscho/fix-fsck-name-objects-generation-parsing-bug-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/874\n-- \ngitgitgadget\n"},{"id":"416647","messageId":"24d43d121162a9052f31c760a5fc929fdaad76b5.1612980090.git.gitgitgadget@gmail.com","threadId":"55131","inReplyTo":"pull.874.git.1612980090.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t1450: robustify `remove_object()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-10T18:01:29Z","receivedAt":"2021-02-10T18:09:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis function can be simplified by using the `test_oid_to_path()`\nhelper, which incidentally also makes it more robust by not relying on\nthe exact file system layout of the loose object files.\n\nWhile at it, do not define those functions in a test case, it buys us\nnothing.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t1450-fsck.sh | 16 ++++++----------\n 1 file changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 02478bc4ece2..779f700ac4a0 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -41,17 +41,13 @@ test_expect_success 'HEAD is part of refs, valid objects appear valid' '\n # specific corruption you test afterwards, lest a later test trip over\n # it.\n \n-test_expect_success 'setup: helpers for corruption tests' '\n-\tsha1_file() {\n-\t\tremainder=${1#??} &&\n-\t\tfirsttwo=${1%$remainder} &&\n-\t\techo \".git/objects/$firsttwo/$remainder\"\n-\t} &&\n+sha1_file () {\n+\tgit rev-parse --git-path objects/$(test_oid_to_path \"$1\")\n+}\n \n-\tremove_object() {\n-\t\trm \"$(sha1_file \"$1\")\"\n-\t}\n-'\n+remove_object() {\n+\trm \"$(sha1_file \"$1\")\"\n+}\n \n test_expect_success 'object with bad sha1' '\n \tsha=$(echo blob | git hash-object -w --stdin) &&\n-- \ngitgitgadget\n\n"},{"id":"416648","messageId":"03efb76b31e46ffe340fe0c6ab5fc4d804b2c273.1612980090.git.gitgitgadget@gmail.com","threadId":"55131","inReplyTo":"pull.874.git.1612980090.gitgitgadget@gmail.com","subject":"[PATCH 2/2] fsck --name-objects: be more careful parsing generation numbers","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-10T18:01:30Z","receivedAt":"2021-02-10T18:10:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 7b35efd734e (fsck_walk(): optionally name objects on the go,\n2016-07-17), the `fsck` machinery learned to optionally name the\nobjects, so that it is easier to see what part of the repository is in a\nbad shape, say, when objects are missing.\n\nTo save on complexity, this machinery uses a parser to determine the\nname of a parent given a commit's name: any `~` suffix is parsed and\nthe parent's name is formed from the prefix together with `~`.\n\nHowever, this parser has a bug: if it finds a suffix `` that is _not_\n`~`, it will mistake the empty string for the prefix and `` for\nthe generation number. In other words, it will generate a name of the\nform `~`.\n\nLet's fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 5 +++++\n t/t1450-fsck.sh | 10 ++++++----\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 73f30773f28a..83d727c6fe33 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -461,6 +461,11 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \t\t\t\tgeneration += power * (name[--len] - '0');\n \t\t\tif (power > 1 && len && name[len - 1] == '~')\n \t\t\t\tname_prefix_len = len - 1;\n+\t\t\telse {\n+\t\t\t\t/* Maybe a non-first parent, e.g. HEAD^2 */\n+\t\t\t\tgeneration = 0;\n+\t\t\t\tname_prefix_len = len;\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 779f700ac4a0..bfa3588f37ab 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -607,13 +607,15 @@ test_expect_success 'fsck --name-objects' '\n \tgit init name-objects &&\n \t(\n \t\tcd name-objects &&\n+\t\tgit config core.logAllRefUpdates false &&\n \t\ttest_commit julius caesar.t &&\n-\t\ttest_commit augustus &&\n-\t\ttest_commit caesar &&\n+\t\ttest_commit augustus44 &&\n+\t\ttest_commit caesar &&\n \t\tremove_object $(git rev-parse julius:caesar.t) &&\n-\t\ttest_must_fail git fsck --name-objects >out &&\n \t\ttree=$(git rev-parse --verify julius:) &&\n-\t\ttest_i18ngrep \"$tree (refs/tags/julius:\" out\n+\t\tgit tag -d julius &&\n+\t\ttest_must_fail git fsck --name-objects >out &&\n+\t\ttest_i18ngrep \"$tree (refs/tags/augustus44\\\\^:\" out\n \t)\n '\n \n-- \ngitgitgadget\n"},{"id":"416659","messageId":"xmqqtuqj1wzg.fsf@gitster.c.googlers.com","threadId":"55131","inReplyTo":"24d43d121162a9052f31c760a5fc929fdaad76b5.1612980090.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t1450: robustify `remove_object()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-10T20:36:19Z","receivedAt":"2021-02-10T20:37:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> -test_expect_success 'setup: helpers for corruption tests' '\n> -\tsha1_file() {\n> -\t\tremainder=${1#??} &&\n> -\t\tfirsttwo=${1%$remainder} &&\n> -\t\techo \".git/objects/$firsttwo/$remainder\"\n> -\t} &&\n> +sha1_file () {\n> +\tgit rev-parse --git-path objects/$(test_oid_to_path \"$1\")\n> +}\n\nYeah, back when 90cf590f (fsck: optionally show more helpful info\nfor broken links, 2016-07-17) originally introduced this pattern,\nwe didn't have nicely abstracted helper, but now we do, and there\nis no reason not to use it. Nice.\n\n> -\tremove_object() {\n> -\t\trm \"$(sha1_file \"$1\")\"\n> -\t}\n> -'\n> +remove_object() {\n\nJust like you did for the other one, let's insert SP before () for\nconsistency here.\n\n> +\trm \"$(sha1_file \"$1\")\"\n> +}\n> \n> test_expect_success 'object with bad sha1' '\n> \tsha=$(echo blob | git hash-object -w --stdin) &&\n\nNicely done. \n\nReviewed-by: Junio C Hamano \n"},{"id":"416660","messageId":"xmqqpn171ww7.fsf@gitster.c.googlers.com","threadId":"55131","inReplyTo":"03efb76b31e46ffe340fe0c6ab5fc4d804b2c273.1612980090.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] fsck --name-objects: be more careful parsing generation numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-10T20:38:16Z","receivedAt":"2021-02-10T20:39:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In 7b35efd734e (fsck_walk(): optionally name objects on the go,\n> 2016-07-17), the `fsck` machinery learned to optionally name the\n> objects, so that it is easier to see what part of the repository is in a\n> bad shape, say, when objects are missing.\n>\n> To save on complexity, this machinery uses a parser to determine the\n> name of a parent given a commit's name: any `~` suffix is parsed and\n> the parent's name is formed from the prefix together with `~`.\n>\n> However, this parser has a bug: if it finds a suffix `` that is _not_\n> `~`, it will mistake the empty string for the prefix and `` for\n> the generation number. In other words, it will generate a name of the\n> form `~`.\n>\n> Let's fix this.\n\nThanks; will queue.\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> fsck.c | 5 +++++\n> t/t1450-fsck.sh | 10 ++++++----\n> 2 files changed, 11 insertions(+), 4 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 73f30773f28a..83d727c6fe33 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -461,6 +461,11 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n> \t\t\t\tgeneration += power * (name[--len] - '0');\n> \t\t\tif (power > 1 && len && name[len - 1] == '~')\n> \t\t\t\tname_prefix_len = len - 1;\n> +\t\t\telse {\n> +\t\t\t\t/* Maybe a non-first parent, e.g. HEAD^2 */\n> +\t\t\t\tgeneration = 0;\n> +\t\t\t\tname_prefix_len = len;\n> +\t\t\t}\n> \t\t}\n> \t}\n> \n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 779f700ac4a0..bfa3588f37ab 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -607,13 +607,15 @@ test_expect_success 'fsck --name-objects' '\n> \tgit init name-objects &&\n> \t(\n> \t\tcd name-objects &&\n> +\t\tgit config core.logAllRefUpdates false &&\n> \t\ttest_commit julius caesar.t &&\n> -\t\ttest_commit augustus &&\n> -\t\ttest_commit caesar &&\n> +\t\ttest_commit augustus44 &&\n> +\t\ttest_commit caesar &&\n> \t\tremove_object $(git rev-parse julius:caesar.t) &&\n> -\t\ttest_must_fail git fsck --name-objects >out &&\n> \t\ttree=$(git rev-parse --verify julius:) &&\n> -\t\ttest_i18ngrep \"$tree (refs/tags/julius:\" out\n> +\t\tgit tag -d julius &&\n> +\t\ttest_must_fail git fsck --name-objects >out &&\n> +\t\ttest_i18ngrep \"$tree (refs/tags/augustus44\\\\^:\" out\n> \t)\n> '\n"},{"id":"416682","messageId":"YCRqOVS/iXp/d6Rc@nand.local","threadId":"55131","inReplyTo":"xmqqtuqj1wzg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] t1450: robustify `remove_object()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-10T23:20:25Z","receivedAt":"2021-02-10T23:21:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 10, 2021 at 12:36:19PM -0800, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > -test_expect_success 'setup: helpers for corruption tests' '\n> > -\tsha1_file() {\n> > -\t\tremainder=${1#??} &&\n> > -\t\tfirsttwo=${1%$remainder} &&\n> > -\t\techo \".git/objects/$firsttwo/$remainder\"\n> > -\t} &&\n> > +sha1_file () {\n> > +\tgit rev-parse --git-path objects/$(test_oid_to_path \"$1\")\n> > +}\n>\n> Yeah, back when 90cf590f (fsck: optionally show more helpful info\n> for broken links, 2016-07-17) originally introduced this pattern,\n> we didn't have nicely abstracted helper, but now we do, and there\n> is no reason not to use it. Nice.\n\nThis has nothing to do with this series, but I do notice a number of\nother uses of test_oid_to_path that are doing this exact thing. In fact,\nmany of them don't use \"git rev-parse --git-path\", which would be\nbetter.\n\nI wonder if it's worth a clean-up on top to consolidate all of those\n\"combine the loose object path of the object with xyz OID and the\n$GIT_DIR/objects directory\".\n\nIn either case -- and I think I'm pretty clearly being pedantic at this\npoint -- do you suppose it's worthwhile to rename sha1_file to something\nthat doesn't have sha1 in it?\n\nThanks,\nTaylor\n"},{"id":"416684","messageId":"xmqqsg63zcpl.fsf@gitster.c.googlers.com","threadId":"55131","inReplyTo":"YCRqOVS/iXp/d6Rc@nand.local","subject":"Re: [PATCH 1/2] t1450: robustify `remove_object()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-11T00:10:14Z","receivedAt":"2021-02-11T00:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Wed, Feb 10, 2021 at 12:36:19PM -0800, Junio C Hamano wrote:\n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>>\n>> > -test_expect_success 'setup: helpers for corruption tests' '\n>> > -\tsha1_file() {\n>> > -\t\tremainder=${1#??} &&\n>> > -\t\tfirsttwo=${1%$remainder} &&\n>> > -\t\techo \".git/objects/$firsttwo/$remainder\"\n>> > -\t} &&\n>> > +sha1_file () {\n>> > +\tgit rev-parse --git-path objects/$(test_oid_to_path \"$1\")\n>> > +}\n>>\n>> Yeah, back when 90cf590f (fsck: optionally show more helpful info\n>> for broken links, 2016-07-17) originally introduced this pattern,\n>> we didn't have nicely abstracted helper, but now we do, and there\n>> is no reason not to use it. Nice.\n>\n> This has nothing to do with this series, but I do notice a number of\n> other uses of test_oid_to_path that are doing this exact thing. In fact,\n> many of them don't use \"git rev-parse --git-path\", which would be\n> better.\n>\n> I wonder if it's worth a clean-up on top to consolidate all of those\n> \"combine the loose object path of the object with xyz OID and the\n> $GIT_DIR/objects directory\".\n>\n> In either case -- and I think I'm pretty clearly being pedantic at this\n> point -- do you suppose it's worthwhile to rename sha1_file to something\n> that doesn't have sha1 in it?\n\nPossibly. That is probably outside the scope of this topic, but we\nsee such SHA -> HASH clean-up patches in different places, and this\ncertainly is a fair game for such a clean-up, I would think.\n\nThanks.\n"}]}