{"thread":{"id":"55803","subject":"[PATCH] [GSOC] cat-file: fix --batch report changed-type bug","startedAt":"2021-05-30T08:29:33Z","lastAt":"2021-06-01T16:42:17Z","messageCount":15,"participants":["ZheNing Hu via GitGitGadget","Jeff King","Junio C Hamano","ZheNing Hu","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"425909","messageId":"pull.965.git.1622363366722.gitgitgadget@gmail.com","threadId":"55803","inReplyTo":null,"subject":"[PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-30T08:29:26Z","receivedAt":"2021-05-30T08:29:33Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen we use `--batch` with no atoms formatting and use\n`--batch-all-objects` at the same time (e.g.\n`git cat-file --batch=\"batman\" --batch-all-objects`),\nGit will exit and report \"object xxx changed type!?\".\n\nThis is because we have a format string which does not\ncontain any atoms, so skip_object_info option will be\nset if we also use --batch-all-objects, and then\n`oid_object_info_extended()` will be skipped in\n`batch_object_write()`, it cause object type to not be\ncollected. Therefore, it reported object type has changed.\n\nSo avoid checking changes in type and size when all_objects\nand skip_object_info options are set at the same time.\n\nSigned-off-by: ZheNing Hu \n---\n [GSOC] cat-file: fix --batch report changed-type bug\n \n \"git cat-file --batch-all-objects --batch=batman\" will trigger a bug:\n \"fatal: object xxx changed type!?\"\n \n Although we will replace the logic here in the future, this patch can\n help the old git maintain its functionality.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-965%2Fadlternative%2Fcat-file-batch-bug-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-965/adlternative/cat-file-batch-bug-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/965\n\n builtin/cat-file.c | 11 ++++++-----\n t/t1006-cat-file.sh | 6 ++++++\n 2 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 5ebf13359e83..5f9578f9b86b 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -345,11 +345,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\tcontents = read_object_file(oid, &type, &size);\n \t\tif (!contents)\n \t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n-\t\tif (type != data->type)\n-\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n-\t\tif (data->info.sizep && size != data->size)\n-\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n-\n+\t\tif (!(opt->all_objects && data->skip_object_info)) {\n+\t\t\tif (type != data->type)\n+\t\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n+\t\t\tif (data->info.sizep && size != data->size)\n+\t\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n+\t\t}\n \t\tbatch_write(opt, contents, size);\n \t\tfree(contents);\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 5d2dc99b74ad..9b0f1ae5ef8b 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -586,4 +586,10 @@ test_expect_success 'cat-file --unordered works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" | wc -l >expect &&\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"batman\" | wc -l >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5\n-- \ngitgitgadget\n"},{"id":"425937","messageId":"YLP/GEN0qIXvWEUn@coredump.intra.peff.net","threadId":"55803","inReplyTo":"pull.965.git.1622363366722.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-30T21:09:44Z","receivedAt":"2021-05-30T21:09:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 30, 2021 at 08:29:26AM +0000, ZheNing Hu via GitGitGadget wrote:\n\n> From: ZheNing Hu \n> \n> When we use `--batch` with no atoms formatting and use\n> `--batch-all-objects` at the same time (e.g.\n> `git cat-file --batch=\"batman\" --batch-all-objects`),\n> Git will exit and report \"object xxx changed type!?\".\n> \n> This is because we have a format string which does not\n> contain any atoms, so skip_object_info option will be\n> set if we also use --batch-all-objects, and then\n> `oid_object_info_extended()` will be skipped in\n> `batch_object_write()`, it cause object type to not be\n> collected. Therefore, it reported object type has changed.\n\nGood find. I think this bug is mostly my fault, as I added the\nskip_object_info flag but never thought to use it with --batch.\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 5ebf13359e83..5f9578f9b86b 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -345,11 +345,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\tcontents = read_object_file(oid, &type, &size);\n> \t\tif (!contents)\n> \t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n> -\t\tif (type != data->type)\n> -\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n> -\t\tif (data->info.sizep && size != data->size)\n> -\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n> -\n> +\t\tif (!(opt->all_objects && data->skip_object_info)) {\n> +\t\t\tif (type != data->type)\n> +\t\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n> +\t\t\tif (data->info.sizep && size != data->size)\n> +\t\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n> +\t\t}\n\nWouldn't checking data->skip_object_info be sufficient? It's only set\nwhen opt->all_objects is set anyway. But more importantly, it is the\nmost direct root of the problem: we did not find out the type and size\nearlier, so comparing anything against data->type is useless.\n\nBut that leads me to a follow-up question: what if we did give a format,\nso skip_object_info isn't set, but it didn't include the type or size?\n\nIn the size code, it looks like we explicitly protect against this by\nchecking if data->info.sizep is set (i.e., did we request the size from\noid_object_info_extended). But it's not for the type.\n\nSo the assumption is that we do always fill in the type, even if the\nuser didn't ask for it. And that assumption is actually violated much\nearlier. These two bits of code from the setup are out of order:\n\n if (opt->all_objects) {\n struct object_info empty = OBJECT_INFO_INIT;\n if (!memcmp(&data.info, &empty, sizeof(empty)))\n data.skip_object_info = 1;\n }\n\n /*\n * If we are printing out the object, then always fill in the type,\n * since we will want to decide whether or not to stream.\n */\n if (opt->print_contents)\n data.info.typep = &data.type;\n\nWe should not let skip_object_info kick in at all if opt->print_contents\nis requested. And that causes other bugs outside of the spot you found.\nWe look at data->type earlier in print_object_or_die() to decide whether\nor not to stream the contents, but we'll see garbage (fortunately we\nzero-initialize the expand_data struct, so it's at least predictably\nzero, and not random undefined behavior).\n\nBut I think we'd want to solve it by swapping the two conditionals I\nshowed above, which restores the assumption made in print_object_or_die().\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 5d2dc99b74ad..9b0f1ae5ef8b 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -586,4 +586,10 @@ test_expect_success 'cat-file --unordered works' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n> +\tgit -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" | wc -l >expect &&\n> +\tgit -C all-two cat-file --batch-all-objects --batch=\"batman\" | wc -l >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nIs it worth testing both of these? The %(objectname) one will fail in\nthe same way (because we do not need to run oid_object_info() to get the\noid, which we already have). I'm OK doing both for better coverage, but\nit may be worth mentioning either in a comment or in the commit message\nthat we expect both to fail, and why.\n\n-Peff\n"},{"id":"425938","messageId":"xmqqy2bv3ovf.fsf@gitster.g","threadId":"55803","inReplyTo":"pull.965.git.1622363366722.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-30T21:15:00Z","receivedAt":"2021-05-30T21:15:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> When we use `--batch` with no atoms formatting and use\n> `--batch-all-objects` at the same time (e.g.\n> `git cat-file --batch=\"batman\" --batch-all-objects`),\n> Git will exit and report \"object xxx changed type!?\".\n>\n> This is because we have a format string which does not\n> contain any atoms, so skip_object_info option will be\n> set if we also use --batch-all-objects, and then\n> `oid_object_info_extended()` will be skipped in\n> `batch_object_write()`, it cause object type to not be\n> collected. Therefore, it reported object type has changed.\n\nThe above analysis on how these die()s get hit makes sense, but ...\n\n> So avoid checking changes in type and size when all_objects\n> and skip_object_info options are set at the same time.\n\n... it is not immediately clear how the above conclusion follows.\n\nAn obvious alternative, however, is to avoid skipping object info\nwhen all objects is in use---but that goes directly against why this\n\"skip\" mechanism was added at 845de33a (cat-file: avoid noop calls\nto sha1_object_info_extended, 2016-05-18).\n\nWhich makes me suspect that the solution presented here would be\ngoing in the right direction.\n\nThere however is one curious thing about this. The log message of\nthe original commit that introduced this optimization does use the\nbatch-check and batch-all-objects at the same time. Was this\nbreakage not there when the original was written and we broke it in\na later update? If so, with what commit? Can that commit have\nbroken other places in cat-file in a similar manner?\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 5ebf13359e83..5f9578f9b86b 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -345,11 +345,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\tcontents = read_object_file(oid, &type, &size);\n> \t\tif (!contents)\n> \t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n> -\t\tif (type != data->type)\n> -\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n> -\t\tif (data->info.sizep && size != data->size)\n> -\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n> -\n> +\t\tif (!(opt->all_objects && data->skip_object_info)) {\n> +\t\t\tif (type != data->type)\n> +\t\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n\nWhen skip_object_info bit is set, we know data->type and date->size\nare unusable and should not be checked, regardless of the reason why\nskip_object_info bit is set, don't we?\n\n> +\t\t\tif (data->info.sizep && size != data->size)\n> +\t\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n\nDoes this check need to be modified at all? Would info.sizep ever\nbe set to non-NULL if skip_object_info is set (hence we are not\ncalling object-info)?\n\n> +\t\t}\n\nIn other words, shouldn't this patch just this one liner?\n\n-\t\tif (type != data->type)\n+\t\tif (data->skip_object_info && type != data->type)\n\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 5d2dc99b74ad..9b0f1ae5ef8b 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -586,4 +586,10 @@ test_expect_success 'cat-file --unordered works' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n> +\tgit -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" | wc -l >expect &&\n> +\tgit -C all-two cat-file --batch-all-objects --batch=\"batman\" | wc -l >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n>\n> base-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5\n"},{"id":"425939","messageId":"YLQFRU2avYFBfdPu@coredump.intra.peff.net","threadId":"55803","inReplyTo":"xmqqy2bv3ovf.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-30T21:36:05Z","receivedAt":"2021-05-30T21:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 31, 2021 at 06:15:00AM +0900, Junio C Hamano wrote:\n\n> There however is one curious thing about this. The log message of\n> the original commit that introduced this optimization does use the\n> batch-check and batch-all-objects at the same time. Was this\n> breakage not there when the original was written and we broke it in\n> a later update? If so, with what commit? Can that commit have\n> broken other places in cat-file in a similar manner?\n\nThe subtlety here is that it is broken with --batch, not --batch-check.\nOnly the former calls print_object_or_die(), which is after all the\ndifference between the two. :)\n\n> [...]\n\nEverything else you noted was quite reasonable, but I think we can get\nfurther at the root of the issue; see my other email.\n\n-Peff\n"},{"id":"425999","messageId":"CAOLTT8Q0zbxh8X03ZmgAzHadTbE4-Af+AB3POOUF2n22u8RExw@mail.gmail.com","threadId":"55803","inReplyTo":"YLP/GEN0qIXvWEUn@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-31T13:20:34Z","receivedAt":"2021-05-31T13:28:02Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年5月31日周一 上午5:09写道：\n\n> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> > index 5ebf13359e83..5f9578f9b86b 100644\n> > --- a/builtin/cat-file.c\n> > +++ b/builtin/cat-file.c\n> > @@ -345,11 +345,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> > contents = read_object_file(oid, &type, &size);\n> > if (!contents)\n> > die(\"object %s disappeared\", oid_to_hex(oid));\n> > - if (type != data->type)\n> > - die(\"object %s changed type!?\", oid_to_hex(oid));\n> > - if (data->info.sizep && size != data->size)\n> > - die(\"object %s changed size!?\", oid_to_hex(oid));\n> > -\n> > + if (!(opt->all_objects && data->skip_object_info)) {\n> > + if (type != data->type)\n> > + die(\"object %s changed type!?\", oid_to_hex(oid));\n> > + if (data->info.sizep && size != data->size)\n> > + die(\"object %s changed size!?\", oid_to_hex(oid));\n> > + }\n>\n> Wouldn't checking data->skip_object_info be sufficient? It's only set\n> when opt->all_objects is set anyway. But more importantly, it is the\n> most direct root of the problem: we did not find out the type and size\n> earlier, so comparing anything against data->type is useless.\n>\n> But that leads me to a follow-up question: what if we did give a format,\n> so skip_object_info isn't set, but it didn't include the type or size?\n>\n\nIndeed, such a situation may arise with some format atom e.g. %(objectname)\nand %(rest). However %(deltabase) and %(objectdisk:size) don't have this\nproblem because they filled typep in `packed_object_info()`.\n\n> In the size code, it looks like we explicitly protect against this by\n> checking if data->info.sizep is set (i.e., did we request the size from\n> oid_object_info_extended). But it's not for the type.\n>\n\nYes, because we set typep while setting print_contents for\nprint_object_or_die() to check different object type case, this\nleads to inconsistent behavior between sizep and typep.\n\n> So the assumption is that we do always fill in the type, even if the\n> user didn't ask for it. And that assumption is actually violated much\n> earlier. These two bits of code from the setup are out of order:\n>\n> if (opt->all_objects) {\n> struct object_info empty = OBJECT_INFO_INIT;\n> if (!memcmp(&data.info, &empty, sizeof(empty)))\n> data.skip_object_info = 1;\n> }\n>\n> /*\n> * If we are printing out the object, then always fill in the type,\n> * since we will want to decide whether or not to stream.\n> */\n> if (opt->print_contents)\n> data.info.typep = &data.type;\n>\n> We should not let skip_object_info kick in at all if opt->print_contents\n> is requested. And that causes other bugs outside of the spot you found.\n> We look at data->type earlier in print_object_or_die() to decide whether\n> or not to stream the contents, but we'll see garbage (fortunately we\n> zero-initialize the expand_data struct, so it's at least predictably\n> zero, and not random undefined behavior).\n>\n> But I think we'd want to solve it by swapping the two conditionals I\n> showed above, which restores the assumption made in print_object_or_die().\n>\n\nThis method is correct. This will ensure that skip_object_info will not\nbe set when print_contents is set.\n\nBy the way, maybe we can merge this two \"if (opt->all_objects)\" block to one.\n\n if (opt->all_objects) {\n struct object_info empty = OBJECT_INFO_INIT;\n if (!memcmp(&data.info, &empty, sizeof(empty)))\n data.skip_object_info = 1;\n }\n\n if (opt->all_objects) {\n struct object_cb_data cb;\n\n if (has_promisor_remote())\n warning(\"This repository uses promisor\nremotes. Some objects may not be loaded.\");\n\nAfter the merger:\n\n if (opt->all_objects) {\n struct object_cb_data cb;\n struct object_info empty = OBJECT_INFO_INIT;\n\n if (!memcmp(&data.info, &empty, sizeof(empty)))\n data.skip_object_info = 1;\n\n if (has_promisor_remote())\n warning(\"This repository uses promisor\nremotes. Some objects may not be loaded.\");\n\n\n> > +test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n> > + git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" | wc -l >expect &&\n> > + git -C all-two cat-file --batch-all-objects --batch=\"batman\" | wc -l >actual &&\n> > + test_cmp expect actual\n> > +'\n>\n> Is it worth testing both of these? The %(objectname) one will fail in\n> the same way (because we do not need to run oid_object_info() to get the\n> oid, which we already have). I'm OK doing both for better coverage, but\n> it may be worth mentioning either in a comment or in the commit message\n> that we expect both to fail, and why.\n>\n\nYes, these damages need to be pointed out in the commit message.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"426003","messageId":"CAOLTT8Qz5NBXKp0iF5+mFK7evhqiTH_TUf+Ve3BuO5iV1DESaQ@mail.gmail.com","threadId":"55803","inReplyTo":"xmqqy2bv3ovf.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-31T13:55:38Z","receivedAt":"2021-05-31T14:45:59Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年5月31日周一 上午5:15写道：\n> > So avoid checking changes in type and size when all_objects\n> > and skip_object_info options are set at the same time.\n>\n> ... it is not immediately clear how the above conclusion follows.\n>\n\nMy origin idea is: Now that it is known that the first \"data->type\" is 0,\nand the second \"type\" obtained is the real type of the object, they are\ndifferent, so we can skip the comparison here.\n\n> An obvious alternative, however, is to avoid skipping object info\n> when all objects is in use---but that goes directly against why this\n> \"skip\" mechanism was added at 845de33a (cat-file: avoid noop calls\n> to sha1_object_info_extended, 2016-05-18).\n>\n> Which makes me suspect that the solution presented here would be\n> going in the right direction.\n>\n\nLooking at it now, Peff's solution may be correct.\n\n> There however is one curious thing about this. The log message of\n> the original commit that introduced this optimization does use the\n> batch-check and batch-all-objects at the same time. Was this\n> breakage not there when the original was written and we broke it in\n> a later update? If so, with what commit? Can that commit have\n> broken other places in cat-file in a similar manner?\n>\n\nBecause it's bug of --batch with --batch-all-objects, not the bug of\n --batch-check with --batch-all-objects. The extra broken atoms\nare %(rest), %(objectname).\n\n> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> > index 5ebf13359e83..5f9578f9b86b 100644\n> > --- a/builtin/cat-file.c\n> > +++ b/builtin/cat-file.c\n> > @@ -345,11 +345,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> > contents = read_object_file(oid, &type, &size);\n> > if (!contents)\n> > die(\"object %s disappeared\", oid_to_hex(oid));\n> > - if (type != data->type)\n> > - die(\"object %s changed type!?\", oid_to_hex(oid));\n> > - if (data->info.sizep && size != data->size)\n> > - die(\"object %s changed size!?\", oid_to_hex(oid));\n> > -\n> > + if (!(opt->all_objects && data->skip_object_info)) {\n> > + if (type != data->type)\n> > + die(\"object %s changed type!?\", oid_to_hex(oid));\n>\n> When skip_object_info bit is set, we know data->type and date->size\n> are unusable and should not be checked, regardless of the reason why\n> skip_object_info bit is set, don't we?\n\nYes... We don't need to consider \"opt->all_objects\".\n\n>\n> > + if (data->info.sizep && size != data->size)\n> > + die(\"object %s changed size!?\", oid_to_hex(oid));\n>\n> Does this check need to be modified at all? Would info.sizep ever\n> be set to non-NULL if skip_object_info is set (hence we are not\n> calling object-info)?\n>\n\nIndeed, we don’t need to consider sizep.\n\n> > + }\n>\n> In other words, shouldn't this patch just this one liner?\n>\n> - if (type != data->type)\n> + if (data->skip_object_info && type != data->type)\n> die(\"object %s changed type!?\", oid_to_hex(oid));\n>\n\nWith the original patch, this is the best code.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"426005","messageId":"YLT2UfCZyQIXWIOv@coredump.intra.peff.net","threadId":"55803","inReplyTo":"CAOLTT8Q0zbxh8X03ZmgAzHadTbE4-Af+AB3POOUF2n22u8RExw@mail.gmail.com","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-31T14:44:33Z","receivedAt":"2021-05-31T14:47:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 31, 2021 at 09:20:34PM +0800, ZheNing Hu wrote:\n\n> > But I think we'd want to solve it by swapping the two conditionals I\n> > showed above, which restores the assumption made in print_object_or_die().\n> \n> This method is correct. This will ensure that skip_object_info will not\n> be set when print_contents is set.\n\nOK, good, it sounds like we are on the same page. :)\n\n> By the way, maybe we can merge this two \"if (opt->all_objects)\" block to one.\n> [...]\n\nYes, I think that would make sense to do. It might be worth doing as a\nseparate patch on top of the bug-fix, though, to make it clear that it's\njust cosmetic and separate from the bug-fix.\n\n> > > +test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n> > > + git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" | wc -l >expect &&\n> > > + git -C all-two cat-file --batch-all-objects --batch=\"batman\" | wc -l >actual &&\n> > > + test_cmp expect actual\n> > > +'\n> >\n> > Is it worth testing both of these? The %(objectname) one will fail in\n> > the same way (because we do not need to run oid_object_info() to get the\n> > oid, which we already have). I'm OK doing both for better coverage, but\n> > it may be worth mentioning either in a comment or in the commit message\n> > that we expect both to fail, and why.\n> \n> Yes, these damages need to be pointed out in the commit message.\n\nI think what confused me here is that you are using \"%(objectname)\" as\nthe \"expect\" output, but it also exhibits the bug. So I'd expect this\ntest to pass even before your patch (though I didn't try it).\n\nReally, the symptom of the bug is that _neither_ of those cat-file\ninvocations will exit with a success code. But because they're on the\nleft-hand side of a pipe, we wouldn't even notice.\n\nThe simplest test is just:\n\n git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >/dev/null\n\nwhich will currently fail. It would be nice to verify that its output is\nsensible, but I'm not sure how to easily do that (it will spew a bunch\nof binary tree data, and it cannot even be parsed reliably since we\nhaven't output the sizes).\n\n-Peff\n"},{"id":"426021","messageId":"CAOLTT8T8aJ8X4TFV51JVB-ZAH-5E_E+bAmSMs2UtJ0k3OCON_w@mail.gmail.com","threadId":"55803","inReplyTo":"YLT2UfCZyQIXWIOv@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-31T15:32:24Z","receivedAt":"2021-05-31T17:18:49Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年5月31日周一 下午10:44写道：\n>\n> > > > +test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n> > > > + git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" | wc -l >expect &&\n> > > > + git -C all-two cat-file --batch-all-objects --batch=\"batman\" | wc -l >actual &&\n> > > > + test_cmp expect actual\n> > > > +'\n> > >\n> > > Is it worth testing both of these? The %(objectname) one will fail in\n> > > the same way (because we do not need to run oid_object_info() to get the\n> > > oid, which we already have). I'm OK doing both for better coverage, but\n> > > it may be worth mentioning either in a comment or in the commit message\n> > > that we expect both to fail, and why.\n> >\n> > Yes, these damages need to be pointed out in the commit message.\n>\n> I think what confused me here is that you are using \"%(objectname)\" as\n> the \"expect\" output, but it also exhibits the bug. So I'd expect this\n> test to pass even before your patch (though I didn't try it).\n>\n\nYes, %(objectname) should not be used as the output of \"expect\",\nit is also a broken part.\n\n> Really, the symptom of the bug is that _neither_ of those cat-file\n> invocations will exit with a success code. But because they're on the\n> left-hand side of a pipe, we wouldn't even notice.\n>\n> The simplest test is just:\n>\n> git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >/dev/null\n>\n\nYes, this is enough. I even think about use something like\n\"!test_must_fail xxx\" before... that is not necessary.\n\n> which will currently fail. It would be nice to verify that its output is\n> sensible, but I'm not sure how to easily do that (it will spew a bunch\n> of binary tree data, and it cannot even be parsed reliably since we\n> haven't output the sizes).\n>\n> -Peff\n\nThanks!\n--\nZheNing Hu\n"},{"id":"426025","messageId":"60b509be97423_24d2820856@natae.notmuch","threadId":"55803","inReplyTo":"YLT2UfCZyQIXWIOv@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-31T16:07:26Z","receivedAt":"2021-05-31T17:27:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> The simplest test is just:\n> \n> git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >/dev/null\n> \n> which will currently fail. It would be nice to verify that its output is\n> sensible, but I'm not sure how to easily do that (it will spew a bunch\n> of binary tree data, and it cannot even be parsed reliably since we\n> haven't output the sizes).\n\nI use ruby to parse binary data from git all the time:\n\n git log --format='%b%x00' |\n ruby -e 'ARGF.each(\"\\0\", chomp: true) { |chunk| p chunk }'\n\n-- \nFelipe Contreras\n"},{"id":"426091","messageId":"xmqq35u2uzu1.fsf@gitster.g","threadId":"55803","inReplyTo":"YLQFRU2avYFBfdPu@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-01T01:40:38Z","receivedAt":"2021-06-01T01:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, May 31, 2021 at 06:15:00AM +0900, Junio C Hamano wrote:\n>\n>> There however is one curious thing about this. The log message of\n>> the original commit that introduced this optimization does use the\n>> batch-check and batch-all-objects at the same time. Was this\n>> breakage not there when the original was written and we broke it in\n>> a later update? If so, with what commit? Can that commit have\n>> broken other places in cat-file in a similar manner?\n>\n> The subtlety here is that it is broken with --batch, not --batch-check.\n> Only the former calls print_object_or_die(), which is after all the\n> difference between the two. :)\n>\n>> [...]\n>\n> Everything else you noted was quite reasonable, but I think we can get\n> further at the root of the issue; see my other email.\n\nYup, I missed the place we depend on type being set without\nchecking.\n"},{"id":"426092","messageId":"YLWSRBJHiph+Bejo@coredump.intra.peff.net","threadId":"55803","inReplyTo":"60b509be97423_24d2820856@natae.notmuch","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-01T01:49:56Z","receivedAt":"2021-06-01T01:49:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 31, 2021 at 11:07:26AM -0500, Felipe Contreras wrote:\n\n> Jeff King wrote:\n> > The simplest test is just:\n> > \n> > git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >/dev/null\n> > \n> > which will currently fail. It would be nice to verify that its output is\n> > sensible, but I'm not sure how to easily do that (it will spew a bunch\n> > of binary tree data, and it cannot even be parsed reliably since we\n> > haven't output the sizes).\n> \n> I use ruby to parse binary data from git all the time:\n> \n> git log --format='%b%x00' |\n> ruby -e 'ARGF.each(\"\\0\", chomp: true) { |chunk| p chunk }'\n\nI doubt we'd want to add a ruby dependency to our test suite, but sure,\nwe could do the same thing with perl.\n\nThe trickier part is that without the sizes, the output is ambiguous\n(the command above will dump trees that contain arbitrary bytes\nincluding NULs, so there's no solid delimiter). We could probably devise\na hacky perl snippet that checks for an expected sequence of output that\nis unlikely to appear otherwise.\n\nOr we could just generate the entire expected output and check it with\n\"cmp\". The most robust way is probably to loop over the objects, running\n\"git cat-file\" for each, but that's slow. Maybe:\n\n git cat-file --batch-all-objects --batch-check='%(objectname)' >objects &&\n git cat-file --batch='%(objectname)' expect &&\n git cat-file --batch-all-objects --batch='%(objectname)' >actual &&\n # not test_cmp, as it is not binary clean!\n cmp expect actual\n\nThat feels a bit circular in that it's mostly just exercising most of\nthe same code in the \"expect\" and \"actual\" paths. The interesting part\nis the combination of the two options, which is why I think that just\nmaking sure we don't hit an error might be enough.\n\n-Peff\n"},{"id":"426097","messageId":"60b5d4f87f30c_e359f2085e@natae.notmuch","threadId":"55803","inReplyTo":"YLWSRBJHiph+Bejo@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-01T06:34:32Z","receivedAt":"2021-06-01T06:34:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Mon, May 31, 2021 at 11:07:26AM -0500, Felipe Contreras wrote:\n> \n> > Jeff King wrote:\n> > > The simplest test is just:\n> > > \n> > > git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >/dev/null\n> > > \n> > > which will currently fail. It would be nice to verify that its output is\n> > > sensible, but I'm not sure how to easily do that (it will spew a bunch\n> > > of binary tree data, and it cannot even be parsed reliably since we\n> > > haven't output the sizes).\n> > \n> > I use ruby to parse binary data from git all the time:\n> > \n> > git log --format='%b%x00' |\n> > ruby -e 'ARGF.each(\"\\0\", chomp: true) { |chunk| p chunk }'\n> \n> I doubt we'd want to add a ruby dependency to our test suite, but sure,\n> we could do the same thing with perl.\n\nI don't mean in the final patches, I mean while the patches are\nbeing developed.\n\nOnce it's clear what the code should do, and how to verify it's doing\nwhat it's supposed to be doing, we can decide how the test suite should\nverify it.\n\nRuby is great for prototyping.\n\n-- \nFelipe Contreras\n"},{"id":"426120","messageId":"CAOLTT8SZxiBtxj7=B-75gVghZAp-qf_oqFGwyi7iKD61uH-vsw@mail.gmail.com","threadId":"55803","inReplyTo":"YLWSRBJHiph+Bejo@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-06-01T12:46:51Z","receivedAt":"2021-06-01T12:47:06Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年6月1日周二 上午9:49写道：\n>\n> Or we could just generate the entire expected output and check it with\n> \"cmp\". The most robust way is probably to loop over the objects, running\n> \"git cat-file\" for each, but that's slow. Maybe:\n>\n> git cat-file --batch-all-objects --batch-check='%(objectname)' >objects &&\n> git cat-file --batch='%(objectname)' expect &&\n> git cat-file --batch-all-objects --batch='%(objectname)' >actual &&\n> # not test_cmp, as it is not binary clean!\n> cmp expect actual\n>\n> That feels a bit circular in that it's mostly just exercising most of\n> the same code in the \"expect\" and \"actual\" paths. The interesting part\n> is the combination of the two options, which is why I think that just\n> making sure we don't hit an error might be enough.\n>\n\nSuch a test is also acceptable. I will add a test like this.\n\n> -Peff\n\n--\nZheNing Hu\n"},{"id":"426143","messageId":"YLZTmzvuRpgY935A@coredump.intra.peff.net","threadId":"55803","inReplyTo":"60b5d4f87f30c_e359f2085e@natae.notmuch","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-01T15:34:51Z","receivedAt":"2021-06-01T15:34:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 01, 2021 at 01:34:32AM -0500, Felipe Contreras wrote:\n\n> > > I use ruby to parse binary data from git all the time:\n> > > \n> > > git log --format='%b%x00' |\n> > > ruby -e 'ARGF.each(\"\\0\", chomp: true) { |chunk| p chunk }'\n> > \n> > I doubt we'd want to add a ruby dependency to our test suite, but sure,\n> > we could do the same thing with perl.\n> \n> I don't mean in the final patches, I mean while the patches are\n> being developed.\n> \n> Once it's clear what the code should do, and how to verify it's doing\n> what it's supposed to be doing, we can decide how the test suite should\n> verify it.\n> \n> Ruby is great for prototyping.\n\nIf we are not worried about the test suite, then I would think viewing\nthe output in \"less\" would be the simplest way to see that it's doing\nthe right thing.\n\n-Peff\n"},{"id":"426149","messageId":"60b66363cc293_3f42088c@natae.notmuch","threadId":"55803","inReplyTo":"YLZTmzvuRpgY935A@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-01T16:42:11Z","receivedAt":"2021-06-01T16:42:17Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Tue, Jun 01, 2021 at 01:34:32AM -0500, Felipe Contreras wrote:\n> \n> > > > I use ruby to parse binary data from git all the time:\n> > > > \n> > > > git log --format='%b%x00' |\n> > > > ruby -e 'ARGF.each(\"\\0\", chomp: true) { |chunk| p chunk }'\n> > > \n> > > I doubt we'd want to add a ruby dependency to our test suite, but sure,\n> > > we could do the same thing with perl.\n> > \n> > I don't mean in the final patches, I mean while the patches are\n> > being developed.\n> > \n> > Once it's clear what the code should do, and how to verify it's doing\n> > what it's supposed to be doing, we can decide how the test suite should\n> > verify it.\n> > \n> > Ruby is great for prototyping.\n> \n> If we are not worried about the test suite, then I would think viewing\n> the output in \"less\" would be the simplest way to see that it's doing\n> the right thing.\n\nNo, I meant you write the test case with ruby and once everything works\nyou decide how to replicate the same with something more widely\navailable.\n\nSometimes I decide to delete the test cases I used for prototyping\nbecause they are too convoluted, test something very esoteric, or it's\nnot easy to do the same in CI infraestructure.\n\nBut they helped me write the code.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}