{"thread":{"id":"55817","subject":"[PATCH 0/2] [GSOC] cat-file: fix --batch report changed-type bug","startedAt":"2021-06-01T14:36:01Z","lastAt":"2021-06-03T22:51:16Z","messageCount":16,"participants":["ZheNing Hu via GitGitGadget","Jeff King","ZheNing Hu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"426127","messageId":"pull.967.git.1622558157.gitgitgadget@gmail.com","threadId":"55817","inReplyTo":null,"subject":"[PATCH 0/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-01T14:35:55Z","receivedAt":"2021-06-01T14:36:01Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Change from last version:\n\n 1. Use Peff's processing method.\n 2. Add tests for atoms %(objectname), %(rest), and modified the test\n method.\n 3. Merge two check block into one.\n\nZheNing Hu (2):\n [GSOC] cat-file: fix --batch report changed-type bug\n [GSOC] cat-file: merge two block into one\n\n builtin/cat-file.c | 10 ++++------\n t/t1006-cat-file.sh | 19 +++++++++++++++++++\n 2 files changed, 23 insertions(+), 6 deletions(-)\n\n\nbase-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-967%2Fadlternative%2Fcat-file-batch-bug-fix-v2-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-967/adlternative/cat-file-batch-bug-fix-v2-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/967\n-- \ngitgitgadget\n"},{"id":"426128","messageId":"495cd90dbaf43e957d03edd2fdc7449b39eee53a.1622558157.git.gitgitgadget@gmail.com","threadId":"55817","inReplyTo":"pull.967.git.1622558157.gitgitgadget@gmail.com","subject":"[PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-01T14:35:56Z","receivedAt":"2021-06-01T14:36:02Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen `--batch` used with `--batch-all-objects`,\nwith some format atoms like %(objectname), %(rest)\nor even no atoms may cause Git exit and report\n\"object xxx changed type!?\".\n\nE.g. `git cat-file --batch=\"batman\" --batch-all-objects`\n\nThis is because we did not get the object type through\noid_object_info_extended(), it's composed of two\nsituations:\n\n1. Since object_info is empty, skip_object_info is\nset to true, We skipped collecting the object type.\n\n2. The formatting atom like %(objectname) does not require\noid_object_info_extended() to collect object types.\n\nThe correct way to deal with it is to swap the order\nof setting skip_object_info and setting typep. This\nwill ensure that we must get the type of the object\nwhen using --batch.\n\nHelped-by: Jeff King \nSigned-off-by: ZheNing Hu \n---\n builtin/cat-file.c | 13 +++++++------\n t/t1006-cat-file.sh | 19 +++++++++++++++++++\n 2 files changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 5ebf13359e83..02461bb5ea6f 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -512,12 +512,6 @@ static int batch_objects(struct batch_options *opt)\n \tif (opt->cmdmode)\n \t\tdata.split_on_whitespace = 1;\n \n-\tif (opt->all_objects) {\n-\t\tstruct object_info empty = OBJECT_INFO_INIT;\n-\t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n-\t\t\tdata.skip_object_info = 1;\n-\t}\n-\n \t/*\n \t * If we are printing out the object, then always fill in the type,\n \t * since we will want to decide whether or not to stream.\n@@ -525,6 +519,13 @@ static int batch_objects(struct batch_options *opt)\n \tif (opt->print_contents)\n \t\tdata.info.typep = &data.type;\n \n+\tif (opt->all_objects) {\n+\t\tstruct object_info empty = OBJECT_INFO_INIT;\n+\n+\t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n+\t\t\tdata.skip_object_info = 1;\n+\t}\n+\n \tif (opt->all_objects) {\n \t\tstruct object_cb_data cb;\n \ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 5d2dc99b74ad..1502a27142ba 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -586,4 +586,23 @@ test_expect_success 'cat-file --unordered works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cat-file --batch=\"%(objectname)\" with --batch-all-objects will work' '\n+\tgit -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects &&\n+\tgit -C all-two cat-file --batch=\"%(objectname)\" expect &&\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'cat-file --batch=\"%(rest)\" with --batch-all-objects will work' '\n+\tgit -C all-two cat-file --batch=\"%(rest)\" expect &&\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"%(rest)\" >actual &&\n+\tcmp 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=\"batman\" expect &&\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"batman\" >actual &&\n+\tcmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"426129","messageId":"f02c1144d916bd36bd32069e439cfc08b2a7898a.1622558157.git.gitgitgadget@gmail.com","threadId":"55817","inReplyTo":"pull.967.git.1622558157.gitgitgadget@gmail.com","subject":"[PATCH 2/2] [GSOC] cat-file: merge two block into one","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-01T14:35:57Z","receivedAt":"2021-06-01T14:36:06Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\n Because the two \"if (opt->all_objects)\" block\n are redundant, merge them into one, to provide\n better readability.\n\nHelped-by: Jeff King \nSigned-off-by: ZheNing Hu \n---\n builtin/cat-file.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 02461bb5ea6f..243fe6844bc6 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -520,14 +520,11 @@ static int batch_objects(struct batch_options *opt)\n \t\tdata.info.typep = &data.type;\n \n \tif (opt->all_objects) {\n+\t\tstruct object_cb_data cb;\n \t\tstruct object_info empty = OBJECT_INFO_INIT;\n \n \t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n \t\t\tdata.skip_object_info = 1;\n-\t}\n-\n-\tif (opt->all_objects) {\n-\t\tstruct object_cb_data cb;\n \n \t\tif (has_promisor_remote())\n \t\t\twarning(\"This repository uses promisor remotes. Some objects may not be loaded.\");\n-- \ngitgitgadget\n"},{"id":"426144","messageId":"YLZXyBJ5YgGfmkKv@coredump.intra.peff.net","threadId":"55817","inReplyTo":"495cd90dbaf43e957d03edd2fdc7449b39eee53a.1622558157.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-01T15:52:40Z","receivedAt":"2021-06-01T15:52:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 01, 2021 at 02:35:56PM +0000, ZheNing Hu via GitGitGadget wrote:\n\n> From: ZheNing Hu \n> \n> When `--batch` used with `--batch-all-objects`,\n> with some format atoms like %(objectname), %(rest)\n> or even no atoms may cause Git exit and report\n> \"object xxx changed type!?\".\n> \n> E.g. `git cat-file --batch=\"batman\" --batch-all-objects`\n> \n> This is because we did not get the object type through\n> oid_object_info_extended(), it's composed of two\n> situations:\n> \n> 1. Since object_info is empty, skip_object_info is\n> set to true, We skipped collecting the object type.\n> \n> 2. The formatting atom like %(objectname) does not require\n> oid_object_info_extended() to collect object types.\n> \n> The correct way to deal with it is to swap the order\n> of setting skip_object_info and setting typep. This\n> will ensure that we must get the type of the object\n> when using --batch.\n\nThanks, this explanation and the patch make sense, and I'd be happy if\nwe take it as-is. But in the name of GSoC, let me offer a few polishing\ntips.\n\nThe commit message hints at the root of the problem, but doesn't say it\nexplicitly. Which is: because setting skip_object_info depends on seeing\nthat the object_info is empty, we can't add items to it after setting\nthat flag. And the code path for --batch does that, hence re-ordering\nthem is the solution.\n\nIt might also be worth noting that the bug was present from when the\nskip_object_info code was initially added in 845de33a5b (cat-file: avoid\nnoop calls to sha1_object_info_extended, 2016-05-18).\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 5d2dc99b74ad..1502a27142ba 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -586,4 +586,23 @@ test_expect_success 'cat-file --unordered works' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'cat-file --batch=\"%(objectname)\" with --batch-all-objects will work' '\n> +\tgit -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects &&\n> +\tgit -C all-two cat-file --batch=\"%(objectname)\" expect &&\n> +\tgit -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >actual &&\n> +\tcmp expect actual\n> +'\n\nOK, we're checking the output of --batch-all-objects versus taking the\nobject list from the input. We can depend on the ordering being\nidentical between the two because --batch-all-objects sorts. Good.\n\n> +test_expect_success 'cat-file --batch=\"%(rest)\" with --batch-all-objects will work' '\n> +\tgit -C all-two cat-file --batch=\"%(rest)\" expect &&\n> +\tgit -C all-two cat-file --batch-all-objects --batch=\"%(rest)\" >actual &&\n> +\tcmp expect actual\n> +'\n\nThis one is rather curious. It definitely shows the bug, but I can't\nimagine why %(rest) would be useful with --batch-all-objects, since its\npurpose is to show the rest of the input line (and there are no input\nlines!).\n\nThat might be a problem later if we change the behavior (e.g., to say\n\"%(rest) does not make sense with --batch-all-objects\"). But I'm also OK\nleaving it for now; somebody later can dig up this commit via git-blame.\n\n> +test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n> +\tgit -C all-two cat-file --batch=\"batman\" expect &&\n> +\tgit -C all-two cat-file --batch-all-objects --batch=\"batman\" >actual &&\n> +\tcmp expect actual\n> +'\n\nAnd this one is a more extreme version of the \"%(objectname)\" one. It's\nuseful as a regression test because we might later change the\noptimization so that %(objectname) ends up filling in the other object\ninfo.\n\nThere's a subtle dependency here on the \"objects\" file from the earlier\ntest. In such a case, we'll often split that out as a separate setup\nstep like:\n\n test_expect_success 'set up object list for --batch-all-objects tests' '\n\tgit -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects\n '\n\nThat makes it more clear that all three of the other tests are doing the\nsame thing (just with different formats), and can be reordered, removed,\netc, later if we wanted to. So not a correctness thing, but rather just\ncommunicating the structure of the tests to later readers.\n\n-Peff\n"},{"id":"426145","messageId":"YLZYZS8jBiOJynbz@coredump.intra.peff.net","threadId":"55817","inReplyTo":"f02c1144d916bd36bd32069e439cfc08b2a7898a.1622558157.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] [GSOC] cat-file: merge two block into one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-01T15:55:17Z","receivedAt":"2021-06-01T15:55:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 01, 2021 at 02:35:57PM +0000, ZheNing Hu via GitGitGadget wrote:\n\n> From: ZheNing Hu \n> \n> Because the two \"if (opt->all_objects)\" block\n> are redundant, merge them into one, to provide\n> better readability.\n\nFunny indentation of the commit message. :)\n\nI think this is worth doing, and I agree the end-result is easier to\nread. Really minor nit, but I probably wouldn't have said \"redundant\"\nhere. The conditionals themselves are redundant, but not the blocks.\nMaybe: There are two \"if (opt->all_objects)\" blocks next to each other;\nmerge them into one to provide better readability.\n\n(not a huge deal, as I think seeing the patch helps explain what is\ngoing on. But again, just trying to offer polishing advice for future\npatches)\n"},{"id":"426234","messageId":"CAOLTT8SCeKy74cVO3K5zJ5n=0s=o9zk2ipV5wM6CHQPzRoMi5Q@mail.gmail.com","threadId":"55817","inReplyTo":"YLZXyBJ5YgGfmkKv@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-06-02T13:15:45Z","receivedAt":"2021-06-02T13:16:10Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年6月1日周二 下午11:52写道：\n>\n> On Tue, Jun 01, 2021 at 02:35:56PM +0000, ZheNing Hu via GitGitGadget wrote:\n>\n> > From: ZheNing Hu \n> >\n> > When `--batch` used with `--batch-all-objects`,\n> > with some format atoms like %(objectname), %(rest)\n> > or even no atoms may cause Git exit and report\n> > \"object xxx changed type!?\".\n> >\n> > E.g. `git cat-file --batch=\"batman\" --batch-all-objects`\n> >\n> > This is because we did not get the object type through\n> > oid_object_info_extended(), it's composed of two\n> > situations:\n> >\n> > 1. Since object_info is empty, skip_object_info is\n> > set to true, We skipped collecting the object type.\n> >\n> > 2. The formatting atom like %(objectname) does not require\n> > oid_object_info_extended() to collect object types.\n> >\n> > The correct way to deal with it is to swap the order\n> > of setting skip_object_info and setting typep. This\n> > will ensure that we must get the type of the object\n> > when using --batch.\n>\n> Thanks, this explanation and the patch make sense, and I'd be happy if\n> we take it as-is. But in the name of GSoC, let me offer a few polishing\n> tips.\n>\n\nThanks, as a GSOC student, your reminders will be very helpful to me.\n\n> The commit message hints at the root of the problem, but doesn't say it\n> explicitly. Which is: because setting skip_object_info depends on seeing\n> that the object_info is empty, we can't add items to it after setting\n> that flag. And the code path for --batch does that, hence re-ordering\n> them is the solution.\n>\n\nUm, let's rewrite the commit message, I don't know if this is accurate:\n\n[GSOC] cat-file: fix --batch report changed-type bug\n\nWhen `--batch` used with `--batch-all-objects`,\nwith some format atoms like %(objectname), %(rest)\nor even no atoms may cause Git exit and report\n\"object xxx changed type!?\".\n\nE.g. `git cat-file --batch=\"batman\" --batch-all-objects`\n\nThe bug was present from when the skip_object_info code\nwas initially added in 845de33a5b (cat-file: avoid\nnoop calls to sha1_object_info_extended, 2016-05-18).\n\nThis is because we did not get the object type through\noid_object_info_extended(), it's composed of two\nsituations:\n\n1. all_objects will be set to true when we use\n`--batch-all-objects`, seeing that object_info\nis empty, skip_object_info will be to true,\n`oid_object_info_extended()` will not get the\ntype of the object.\n\n2. The formatting atom like %(objectname) does\nnot require oid_object_info_extended() to collect\nobject types.\n\nprint_contents will be set to true when we use\n`--batch`, which can make object_info non-empty,\nso the solution is to swap the code order of it\nand checking if object_info is empty, which will\nensure that we must get the type of the object\nwhen using --batch.\n\n> It might also be worth noting that the bug was present from when the\n> skip_object_info code was initially added in 845de33a5b (cat-file: avoid\n> noop calls to sha1_object_info_extended, 2016-05-18).\n>\n\nOK.\n\n> > +test_expect_success 'cat-file --batch=\"%(objectname)\" with --batch-all-objects will work' '\n> > + git -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects &&\n> > + git -C all-two cat-file --batch=\"%(objectname)\" expect &&\n> > + git -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >actual &&\n> > + cmp expect actual\n> > +'\n>\n> OK, we're checking the output of --batch-all-objects versus taking the\n> object list from the input. We can depend on the ordering being\n> identical between the two because --batch-all-objects sorts. Good.\n>\n> > +test_expect_success 'cat-file --batch=\"%(rest)\" with --batch-all-objects will work' '\n> > + git -C all-two cat-file --batch=\"%(rest)\" expect &&\n> > + git -C all-two cat-file --batch-all-objects --batch=\"%(rest)\" >actual &&\n> > + cmp expect actual\n> > +'\n>\n> This one is rather curious. It definitely shows the bug, but I can't\n> imagine why %(rest) would be useful with --batch-all-objects, since its\n> purpose is to show the rest of the input line (and there are no input\n> lines!).\n>\n\nI wanted to argue that print_object_or_die() will use data->rest originally.\nBut --batch-all-objects and --textconv and --filter are incompatible.\nSo your idea is reasonable: %(rest) with --batch-all-objects is useless.\n\n> That might be a problem later if we change the behavior (e.g., to say\n> \"%(rest) does not make sense with --batch-all-objects\"). But I'm also OK\n> leaving it for now; somebody later can dig up this commit via git-blame.\n>\n\nYes, I think this feature can be completed later.(not in this patch)\n\n> > +test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will work' '\n> > + git -C all-two cat-file --batch=\"batman\" expect &&\n> > + git -C all-two cat-file --batch-all-objects --batch=\"batman\" >actual &&\n> > + cmp expect actual\n> > +'\n>\n> And this one is a more extreme version of the \"%(objectname)\" one. It's\n> useful as a regression test because we might later change the\n> optimization so that %(objectname) ends up filling in the other object\n> info.\n>\n\nYes, it does not use atoms, so it is the most special.\n\n> There's a subtle dependency here on the \"objects\" file from the earlier\n> test. In such a case, we'll often split that out as a separate setup\n> step like:\n>\n> test_expect_success 'set up object list for --batch-all-objects tests' '\n> git -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects\n> '\n>\n> That makes it more clear that all three of the other tests are doing the\n> same thing (just with different formats), and can be reordered, removed,\n> etc, later if we wanted to. So not a correctness thing, but rather just\n> communicating the structure of the tests to later readers.\n>\n\nMakes sense.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"426235","messageId":"CAOLTT8QfvRQdLOhHUsK63Z_8K2VsRq1umwFCNVhHKeodtu7BNw@mail.gmail.com","threadId":"55817","inReplyTo":"YLZYZS8jBiOJynbz@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] [GSOC] cat-file: merge two block into one","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-06-02T13:27:51Z","receivedAt":"2021-06-02T13:28:04Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年6月1日周二 下午11:55写道：\n>\n> On Tue, Jun 01, 2021 at 02:35:57PM +0000, ZheNing Hu via GitGitGadget wrote:\n>\n> > From: ZheNing Hu \n> >\n> > Because the two \"if (opt->all_objects)\" block\n> > are redundant, merge them into one, to provide\n> > better readability.\n>\n> Funny indentation of the commit message. :)\n>\n\nYes, there is a small space that I didn’t notice. :)\n\n> I think this is worth doing, and I agree the end-result is easier to\n> read. Really minor nit, but I probably wouldn't have said \"redundant\"\n> here. The conditionals themselves are redundant, but not the blocks.\n> Maybe: There are two \"if (opt->all_objects)\" blocks next to each other;\n> merge them into one to provide better readability.\n>\n\nYeah, your expression is correct.\n\n> (not a huge deal, as I think seeing the patch helps explain what is\n> going on. But again, just trying to offer polishing advice for future\n> patches)\n\nThanks.\n--\nZheNing Hu\n"},{"id":"426254","messageId":"YLfjexczp1/HILWj@coredump.intra.peff.net","threadId":"55817","inReplyTo":"CAOLTT8SCeKy74cVO3K5zJ5n=0s=o9zk2ipV5wM6CHQPzRoMi5Q@mail.gmail.com","subject":"Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-02T20:00:59Z","receivedAt":"2021-06-02T20:01:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 02, 2021 at 09:15:45PM +0800, ZheNing Hu wrote:\n\n> > The commit message hints at the root of the problem, but doesn't say it\n> > explicitly. Which is: because setting skip_object_info depends on seeing\n> > that the object_info is empty, we can't add items to it after setting\n> > that flag. And the code path for --batch does that, hence re-ordering\n> > them is the solution.\n> \n> Um, let's rewrite the commit message, I don't know if this is accurate:\n> \n> [GSOC] cat-file: fix --batch report changed-type bug\n> \n> When `--batch` used with `--batch-all-objects`,\n> with some format atoms like %(objectname), %(rest)\n> or even no atoms may cause Git exit and report\n> \"object xxx changed type!?\".\n> \n> E.g. `git cat-file --batch=\"batman\" --batch-all-objects`\n> \n> The bug was present from when the skip_object_info code\n> was initially added in 845de33a5b (cat-file: avoid\n> noop calls to sha1_object_info_extended, 2016-05-18).\n> \n> This is because we did not get the object type through\n> oid_object_info_extended(), it's composed of two\n> situations:\n> \n> 1. all_objects will be set to true when we use\n> `--batch-all-objects`, seeing that object_info\n> is empty, skip_object_info will be to true,\n> `oid_object_info_extended()` will not get the\n> type of the object.\n> \n> 2. The formatting atom like %(objectname) does\n> not require oid_object_info_extended() to collect\n> object types.\n> \n> print_contents will be set to true when we use\n> `--batch`, which can make object_info non-empty,\n> so the solution is to swap the code order of it\n> and checking if object_info is empty, which will\n> ensure that we must get the type of the object\n> when using --batch.\n\nI don't see any inaccuracies there. I do think we could explain it a bit\nmore succinctly. I'll give my attempt, and then you can pick and choose\nwhich parts to include between ours. :)\n\n Subject: cat-file: handle trivial --batch format with --batch-all-objects\n\n The --batch code to print an object assumes we found out the type of\n the object from calling oid_object_info_extended(). This is true for\n the default format, but even in a custom format, we manually modify\n the object_info struct to ask for the type.\n\n This assumption was broken by 845de33a5b (cat-file: avoid noop calls\n to sha1_object_info_extended, 2016-05-18). That commit skips the call\n to oid_object_info_extended() entirely when --batch-all-objects is in\n use, and the custom format does not include any placeholders that\n require calling it.\n\n This results in an error when we try to confirm that the type didn't\n change:\n\n $ git cat-file --batch=batman --batch-all-objects\n batman\n fatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?\n\n and also has other subtle effects (e.g., we'd fail to stream a blob,\n since we don't realize it's a blog in the first place).\n\n We can fix this by flipping the order of the setup. The check for \"do\n we need to get the object info\" must come _after_ we've decided\n whether we need to look up the type.\n\n-Peff\n"},{"id":"426302","messageId":"CAOLTT8QHm5xXPJ2fb70f9gH2Hk2gjbAHCHyvm94KB5n1beCE8A@mail.gmail.com","threadId":"55817","inReplyTo":"YLfjexczp1/HILWj@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-06-03T06:28:41Z","receivedAt":"2021-06-03T06:30:04Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年6月3日周四 上午4:01写道：\n>\n> I don't see any inaccuracies there. I do think we could explain it a bit\n> more succinctly. I'll give my attempt, and then you can pick and choose\n> which parts to include between ours. :)\n>\n\nYes, you wrote it more concisely. I don't need to write out the details of\nthose variables. I just need to tell the reader the introduction of\nskip_object_info will break the assumption of --batch that we have found\nthe object type.\n\n> Subject: cat-file: handle trivial --batch format with --batch-all-objects\n>\n> The --batch code to print an object assumes we found out the type of\n> the object from calling oid_object_info_extended(). This is true for\n> the default format, but even in a custom format, we manually modify\n> the object_info struct to ask for the type.\n>\n> This assumption was broken by 845de33a5b (cat-file: avoid noop calls\n> to sha1_object_info_extended, 2016-05-18). That commit skips the call\n> to oid_object_info_extended() entirely when --batch-all-objects is in\n> use, and the custom format does not include any placeholders that\n> require calling it.\n>\n> This results in an error when we try to confirm that the type didn't\n> change:\n>\n> $ git cat-file --batch=batman --batch-all-objects\n> batman\n> fatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?\n>\n> and also has other subtle effects (e.g., we'd fail to stream a blob,\n> since we don't realize it's a blog in the first place).\n>\n\ns/blog/blob/\n\n> We can fix this by flipping the order of the setup. The check for \"do\n> we need to get the object info\" must come _after_ we've decided\n> whether we need to look up the type.\n>\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"426305","messageId":"CAOLTT8QMpMV=7A4ZP2k5YUrqjznBwh_uDv+qvC7Meq3pwvZt1g@mail.gmail.com","threadId":"55817","inReplyTo":"YLfjexczp1/HILWj@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-06-03T07:18:43Z","receivedAt":"2021-06-03T07:19:03Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年6月3日周四 上午4:01写道：\n>\n> This assumption was broken by 845de33a5b (cat-file: avoid noop calls\n> to sha1_object_info_extended, 2016-05-18). That commit skips the call\n> to oid_object_info_extended() entirely when --batch-all-objects is in\n> use, and the custom format does not include any placeholders that\n> require calling it.\n>\n\nOr when the custom format only include placeholders like %(objectname) or\n%(rest), oid_object_info_extended() will not get the type of the object.\n\n> This results in an error when we try to confirm that the type didn't\n> change:\n>\n> $ git cat-file --batch=batman --batch-all-objects\n> batman\n> fatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?\n>\n> and also has other subtle effects (e.g., we'd fail to stream a blob,\n> since we don't realize it's a blog in the first place).\n>\n> We can fix this by flipping the order of the setup. The check for \"do\n> we need to get the object info\" must come _after_ we've decided\n> whether we need to look up the type.\n>\n> -Peff\n\n--\nZheNing Hu\n"},{"id":"426330","messageId":"pull.967.v2.git.1622737766.gitgitgadget@gmail.com","threadId":"55817","inReplyTo":"pull.967.git.1622558157.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-03T16:29:24Z","receivedAt":"2021-06-03T16:30:30Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Change from last version:\n\n 1. Modified the test structure under the recommendation of Peff.\n 2. Use clearer and more concise commit message help by Peff.\n\nZheNing Hu (2):\n [GSOC] cat-file: handle trivial --batch format with\n --batch-all-objects\n [GSOC] cat-file: merge two block into one\n\n builtin/cat-file.c | 10 ++++------\n t/t1006-cat-file.sh | 22 ++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 6 deletions(-)\n\n\nbase-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-967%2Fadlternative%2Fcat-file-batch-bug-fix-v2-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-967/adlternative/cat-file-batch-bug-fix-v2-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/967\n\nRange-diff vs v1:\n\n 1: 495cd90dbaf4 ! 1: 4af3b958dd05 [GSOC] cat-file: fix --batch report changed-type bug\n @@ Metadata\n Author: ZheNing Hu \n \n ## Commit message ##\n - [GSOC] cat-file: fix --batch report changed-type bug\n + [GSOC] cat-file: handle trivial --batch format with --batch-all-objects\n \n - When `--batch` used with `--batch-all-objects`,\n - with some format atoms like %(objectname), %(rest)\n - or even no atoms may cause Git exit and report\n - \"object xxx changed type!?\".\n + The --batch code to print an object assumes we found out the type of\n + the object from calling oid_object_info_extended(). This is true for\n + the default format, but even in a custom format, we manually modify\n + the object_info struct to ask for the type.\n \n - E.g. `git cat-file --batch=\"batman\" --batch-all-objects`\n + This assumption was broken by 845de33a5b (cat-file: avoid noop calls\n + to sha1_object_info_extended, 2016-05-18). That commit skips the call\n + to oid_object_info_extended() entirely when --batch-all-objects is in\n + use, and the custom format does not include any placeholders that\n + require calling it.\n \n - This is because we did not get the object type through\n - oid_object_info_extended(), it's composed of two\n - situations:\n + Or when the custom format only include placeholders like %(objectname) or\n + %(rest), oid_object_info_extended() will not get the type of the object.\n \n - 1. Since object_info is empty, skip_object_info is\n - set to true, We skipped collecting the object type.\n + This results in an error when we try to confirm that the type didn't\n + change:\n \n - 2. The formatting atom like %(objectname) does not require\n - oid_object_info_extended() to collect object types.\n + $ git cat-file --batch=batman --batch-all-objects\n + batman\n + fatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?\n \n - The correct way to deal with it is to swap the order\n - of setting skip_object_info and setting typep. This\n - will ensure that we must get the type of the object\n - when using --batch.\n + and also has other subtle effects (e.g., we'd fail to stream a blob,\n + since we don't realize it's a blob in the first place).\n +\n + We can fix this by flipping the order of the setup. The check for \"do\n + we need to get the object info\" must come _after_ we've decided\n + whether we need to look up the type.\n \n Helped-by: Jeff King \n Signed-off-by: ZheNing Hu \n @@ t/t1006-cat-file.sh: test_expect_success 'cat-file --unordered works' '\n \ttest_cmp expect actual\n '\n \n ++test_expect_success 'set up object list for --batch-all-objects tests' '\n ++\tgit -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects\n ++'\n ++\n +test_expect_success 'cat-file --batch=\"%(objectname)\" with --batch-all-objects will work' '\n -+\tgit -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects &&\n +\tgit -C all-two cat-file --batch=\"%(objectname)\" expect &&\n +\tgit -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >actual &&\n +\tcmp expect actual\n 2: f02c1144d916 ! 2: 759451e784de [GSOC] cat-file: merge two block into one\n @@ Metadata\n Author: ZheNing Hu \n \n ## Commit message ##\n - [GSOC] cat-file: merge two block into one\n + [GSOC] cat-file: merge two block into one\n \n - Because the two \"if (opt->all_objects)\" block\n - are redundant, merge them into one, to provide\n - better readability.\n + There are two \"if (opt->all_objects)\" blocks next\n + to each other, merge them into one to provide better\n + readability.\n \n Helped-by: Jeff King \n Signed-off-by: ZheNing Hu \n\n-- \ngitgitgadget\n"},{"id":"426331","messageId":"4af3b958dd056e2162fdc5d7f6600bcedde210b8.1622737766.git.gitgitgadget@gmail.com","threadId":"55817","inReplyTo":"pull.967.v2.git.1622737766.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] [GSOC] cat-file: handle trivial --batch format with --batch-all-objects","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-03T16:29:25Z","receivedAt":"2021-06-03T16:30:31Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nThe --batch code to print an object assumes we found out the type of\nthe object from calling oid_object_info_extended(). This is true for\nthe default format, but even in a custom format, we manually modify\nthe object_info struct to ask for the type.\n\nThis assumption was broken by 845de33a5b (cat-file: avoid noop calls\nto sha1_object_info_extended, 2016-05-18). That commit skips the call\nto oid_object_info_extended() entirely when --batch-all-objects is in\nuse, and the custom format does not include any placeholders that\nrequire calling it.\n\nOr when the custom format only include placeholders like %(objectname) or\n%(rest), oid_object_info_extended() will not get the type of the object.\n\nThis results in an error when we try to confirm that the type didn't\nchange:\n\n$ git cat-file --batch=batman --batch-all-objects\nbatman\nfatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?\n\nand also has other subtle effects (e.g., we'd fail to stream a blob,\nsince we don't realize it's a blob in the first place).\n\nWe can fix this by flipping the order of the setup. The check for \"do\nwe need to get the object info\" must come _after_ we've decided\nwhether we need to look up the type.\n\nHelped-by: Jeff King \nSigned-off-by: ZheNing Hu \n---\n builtin/cat-file.c | 13 +++++++------\n t/t1006-cat-file.sh | 22 ++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 5ebf13359e83..02461bb5ea6f 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -512,12 +512,6 @@ static int batch_objects(struct batch_options *opt)\n \tif (opt->cmdmode)\n \t\tdata.split_on_whitespace = 1;\n \n-\tif (opt->all_objects) {\n-\t\tstruct object_info empty = OBJECT_INFO_INIT;\n-\t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n-\t\t\tdata.skip_object_info = 1;\n-\t}\n-\n \t/*\n \t * If we are printing out the object, then always fill in the type,\n \t * since we will want to decide whether or not to stream.\n@@ -525,6 +519,13 @@ static int batch_objects(struct batch_options *opt)\n \tif (opt->print_contents)\n \t\tdata.info.typep = &data.type;\n \n+\tif (opt->all_objects) {\n+\t\tstruct object_info empty = OBJECT_INFO_INIT;\n+\n+\t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n+\t\t\tdata.skip_object_info = 1;\n+\t}\n+\n \tif (opt->all_objects) {\n \t\tstruct object_cb_data cb;\n \ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 5d2dc99b74ad..18b3779ccb60 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -586,4 +586,26 @@ test_expect_success 'cat-file --unordered works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'set up object list for --batch-all-objects tests' '\n+\tgit -C all-two cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects\n+'\n+\n+test_expect_success 'cat-file --batch=\"%(objectname)\" with --batch-all-objects will work' '\n+\tgit -C all-two cat-file --batch=\"%(objectname)\" expect &&\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"%(objectname)\" >actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'cat-file --batch=\"%(rest)\" with --batch-all-objects will work' '\n+\tgit -C all-two cat-file --batch=\"%(rest)\" expect &&\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"%(rest)\" >actual &&\n+\tcmp 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=\"batman\" expect &&\n+\tgit -C all-two cat-file --batch-all-objects --batch=\"batman\" >actual &&\n+\tcmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"426332","messageId":"759451e784deb7b0cc84665ff7c0d9a98ec0fc0c.1622737766.git.gitgitgadget@gmail.com","threadId":"55817","inReplyTo":"pull.967.v2.git.1622737766.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] [GSOC] cat-file: merge two block into one","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-03T16:29:26Z","receivedAt":"2021-06-03T16:30:44Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nThere are two \"if (opt->all_objects)\" blocks next\nto each other, merge them into one to provide better\nreadability.\n\nHelped-by: Jeff King \nSigned-off-by: ZheNing Hu \n---\n builtin/cat-file.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 02461bb5ea6f..243fe6844bc6 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -520,14 +520,11 @@ static int batch_objects(struct batch_options *opt)\n \t\tdata.info.typep = &data.type;\n \n \tif (opt->all_objects) {\n+\t\tstruct object_cb_data cb;\n \t\tstruct object_info empty = OBJECT_INFO_INIT;\n \n \t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n \t\t\tdata.skip_object_info = 1;\n-\t}\n-\n-\tif (opt->all_objects) {\n-\t\tstruct object_cb_data cb;\n \n \t\tif (has_promisor_remote())\n \t\t\twarning(\"This repository uses promisor remotes. Some objects may not be loaded.\");\n-- \ngitgitgadget\n"},{"id":"426349","messageId":"YLktazqb0yY5LK/V@coredump.intra.peff.net","threadId":"55817","inReplyTo":"CAOLTT8QMpMV=7A4ZP2k5YUrqjznBwh_uDv+qvC7Meq3pwvZt1g@mail.gmail.com","subject":"Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-03T19:28:43Z","receivedAt":"2021-06-03T19:28:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 03, 2021 at 03:18:43PM +0800, ZheNing Hu wrote:\n\n> Jeff King 于2021年6月3日周四 上午4:01写道：\n> >\n> > This assumption was broken by 845de33a5b (cat-file: avoid noop calls\n> > to sha1_object_info_extended, 2016-05-18). That commit skips the call\n> > to oid_object_info_extended() entirely when --batch-all-objects is in\n> > use, and the custom format does not include any placeholders that\n> > require calling it.\n> >\n> \n> Or when the custom format only include placeholders like %(objectname) or\n> %(rest), oid_object_info_extended() will not get the type of the object.\n\nYeah, that's what I was trying to get at with \"placeholders that require\ncalling it\", but I couldn't think of a less awkward way to say that. :)\nSpelling it out is probably a good idea.\n\n-Peff\n"},{"id":"426350","messageId":"YLktoTqu/YZgOe4k@coredump.intra.peff.net","threadId":"55817","inReplyTo":"pull.967.v2.git.1622737766.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-03T19:29:37Z","receivedAt":"2021-06-03T19:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 03, 2021 at 04:29:24PM +0000, ZheNing Hu via GitGitGadget wrote:\n\n> Change from last version:\n> \n> 1. Modified the test structure under the recommendation of Peff.\n> 2. Use clearer and more concise commit message help by Peff.\n\nThanks, this version looks good to me.\n\n-Peff\n"},{"id":"426369","messageId":"xmqq7dja4l5q.fsf@gitster.g","threadId":"55817","inReplyTo":"pull.967.v2.git.1622737766.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] [GSOC] cat-file: fix --batch report changed-type bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-03T22:51:13Z","receivedAt":"2021-06-03T22:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> Change from last version:\n>\n> 1. Modified the test structure under the recommendation of Peff.\n> 2. Use clearer and more concise commit message help by Peff.\n>\n> ZheNing Hu (2):\n> [GSOC] cat-file: handle trivial --batch format with\n> --batch-all-objects\n> [GSOC] cat-file: merge two block into one\n>\n> builtin/cat-file.c | 10 ++++------\n> t/t1006-cat-file.sh | 22 ++++++++++++++++++++++\n> 2 files changed, 26 insertions(+), 6 deletions(-)\n>\n\nNicely done. Thanks, both.\n"}]}