{"thread":{"id":"35517","subject":"[BUG] \"echo HEAD | git cat-file --batch=''\" fails catastrophically","startedAt":"2013-12-11T04:37:14Z","lastAt":"2013-12-12T03:05:46Z","messageCount":11,"participants":["Samuel Bronson","Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"231884","messageId":"CAJYzjmdHdLZaBijahepOQDJtDd_TdojT4ivNxGrcerRfEuHQEg@mail.gmail.com","threadId":"35517","inReplyTo":null,"subject":"[BUG] \"echo HEAD | git cat-file --batch=''\" fails catastrophically","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-11T04:37:14Z","receivedAt":"2013-12-11T04:37:14Z","isPatch":false,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"Observe:\n\n% echo HEAD | git cat-file --batch=\n\nfatal: object fde075cb72fc0773d8e8ca93d55a35d77bb6688b changed type!?\n\nWithout the =, it works fine; with a string that has both\n\"%(objecttype)\" and \"%(objectsize)\", it's fine; but when you don't\ninclude both, it complains about one of the values that you did not\nmention having changed.\n\njrnieder fingered v1.8.4-rc0~7^2~15 as the (likely?) culprit here.\n"},{"id":"231902","messageId":"20131211115458.GA10561@sigill.intra.peff.net","threadId":"35517","inReplyTo":"CAJYzjmdHdLZaBijahepOQDJtDd_TdojT4ivNxGrcerRfEuHQEg@mail.gmail.com","subject":"Re: [BUG] \"echo HEAD | git cat-file --batch=''\" fails catastrophically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-11T11:54:58Z","receivedAt":"2013-12-11T11:54:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 10, 2013 at 11:37:14PM -0500, Samuel Bronson wrote:\n\n> % echo HEAD | git cat-file --batch=\n> \n> fatal: object fde075cb72fc0773d8e8ca93d55a35d77bb6688b changed type!?\n> \n> Without the =, it works fine; with a string that has both\n> \"%(objecttype)\" and \"%(objectsize)\", it's fine; but when you don't\n> include both, it complains about one of the values that you did not\n> mention having changed.\n> \n> jrnieder fingered v1.8.4-rc0~7^2~15 as the (likely?) culprit here.\n\nIt's not actually that commit itself, but rather that commit in\nconjunction with further optimizations in that patch series.\n\nThe rest of the series tries hard to avoid looking up items that we\naren't going to print, for --batch-check. But I didn't think about the\nfact that \"--batch\" got the same custom-header feature, but was relying\non the values from the default header to do its consistency checks.\n\nThe following patches should fix it.\n\n  [1/2]: cat-file: pass expand_data to print_object_or_die\n  [2/2]: cat-file: handle --batch format with missing type/size\n\nDoing \"--batch=\" is somewhat pointless. If you do not get the size, you\ncannot know when the object content ends, so it only makes sense with a\nsingle object. At which point using --batch is pointless. Doing\n\"--batch=%(objectsize)\" is reasonable, though, and that is broken, too.\n\nv1.8.4 has the breakage, though it's not a regression (doing\n\"--batch=anything\" did not exist before that). This can probably just go\nto the regular \"maint\" track for v1.8.5).\n\n-Peff\n"},{"id":"231908","messageId":"20131211115642.GA10594@sigill.intra.peff.net","threadId":"35517","inReplyTo":"20131211115458.GA10561@sigill.intra.peff.net","subject":"[PATCH 1/2] cat-file: pass expand_data to print_object_or_die","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-11T11:56:45Z","receivedAt":"2013-12-11T11:56:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently individually pass the sha1, type, and size\nfields calculated by sha1_object_info. However, if we pass\nthe whole struct, the called function can make more\nintelligent decisions about which fields were actualled\nfilled by sha1_object_info.\n\nAs a side effect, we can rename the local variables in the\nfunction to \"type\" and \"size\", since the names are no longer\ntaken.\n\nThere should be no functional change to this patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI split this out mostly to keep the noise out of the follow-on diff.\n\n builtin/cat-file.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b2ca775..1434afb 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -193,25 +193,26 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n \treturn end - start + 1;\n }\n \n-static void print_object_or_die(int fd, const unsigned char *sha1,\n-\t\t\t\tenum object_type type, unsigned long size)\n+static void print_object_or_die(int fd, struct expand_data *data)\n {\n-\tif (type == OBJ_BLOB) {\n+\tconst unsigned char *sha1 = data->sha1;\n+\n+\tif (data->type == OBJ_BLOB) {\n \t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n \t}\n \telse {\n-\t\tenum object_type rtype;\n-\t\tunsigned long rsize;\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n \t\tvoid *contents;\n \n-\t\tcontents = read_sha1_file(sha1, &rtype, &rsize);\n+\t\tcontents = read_sha1_file(sha1, &type, &size);\n \t\tif (!contents)\n \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n-\t\tif (rtype != type)\n+\t\tif (type != data->type)\n \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n-\t\tif (rsize != size)\n-\t\t\tdie(\"object %s change size!?\", sha1_to_hex(sha1));\n+\t\tif (size != data->size)\n+\t\t\tdie(\"object %s changed size!?\", sha1_to_hex(sha1));\n \n \t\twrite_or_die(fd, contents, size);\n \t\tfree(contents);\n@@ -250,7 +251,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \tstrbuf_release(&buf);\n \n \tif (opt->print_contents) {\n-\t\tprint_object_or_die(1, data->sha1, data->type, data->size);\n+\t\tprint_object_or_die(1, data);\n \t\twrite_or_die(1, \"\\n\", 1);\n \t}\n \treturn 0;\n-- \n1.8.5.524.g6743da6\n"},{"id":"231909","messageId":"20131211115844.GB10594@sigill.intra.peff.net","threadId":"35517","inReplyTo":"20131211115458.GA10561@sigill.intra.peff.net","subject":"[PATCH 2/2] cat-file: handle --batch format with missing type/size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-11T11:58:45Z","receivedAt":"2013-12-11T11:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 98e2092 taught cat-file to stream blobs with --batch,\nwhich requires that we look up the object type before\nloading it into memory.  As a result, we now print the\nobject header from information in sha1_object_info, and the\nactual contents from the read_sha1_file. We double-check\nthat the information we printed in the header matches the\ncontent we are about to show.\n\nLater, commit 93d2a60 allowed custom header lines for\n--batch, and commit 5b08640 made type lookups optional. As a\nresult, specifying a header line without the type or size\nmeans that we will not look up those items at all.\n\nThis causes our double-checking to erroneously die with an\nerror; we think the type or size has changed, when in fact\nit was simply left at \"0\".\n\nFor the size, we can fix this by only doing the consistency\ndouble-check when we have retrieved the size via\nsha1_object_info. In the case that we have not retrieved the\nvalue, that means we also did not print it, so there is\nnothing for us to check that we are consistent with.\n\nWe could do the same for the type. However, besides our\nconsistency check, we also care about the type in deciding\nwhether to stream or not. We therefore make sure to always\ntrigger a type lookup when we are printing, so that even a\nformat without the type will stream as we would in the\nnormal case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c  |  9 ++++++++-\n t/t1006-cat-file.sh | 22 ++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 1434afb..4af67fd 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)\n \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n \t\tif (type != data->type)\n \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n-\t\tif (size != data->size)\n+\t\tif (data->info.sizep && size != data->size)\n \t\t\tdie(\"object %s changed size!?\", sha1_to_hex(sha1));\n \n \t\twrite_or_die(fd, contents, size);\n@@ -276,6 +276,13 @@ static int batch_objects(struct batch_options *opt)\n \tdata.mark_query = 0;\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+\t */\n+\tif (opt->print_contents)\n+\t\tdata.info.typep = &data.type;\n+\n+\t/*\n \t * We are going to call get_sha1 on a potentially very large number of\n \t * objects. In most large cases, these will be actual object sha1s. The\n \t * cost to double-check that each one is not also a ref (just so we can\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 8a1bc5c..1687098 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -85,6 +85,28 @@ $content\"\n \t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n \ttest_cmp expect actual\n     '\n+\n+    test -z \"$content\" ||\n+    test_expect_success \"--batch without type ($type)\" '\n+\t{\n+\t\techo \"$size\" &&\n+\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t} >expect &&\n+\techo $sha1 | git cat-file --batch=\"%(objectsize)\" >actual.full &&\n+\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\ttest_cmp expect actual\n+    '\n+\n+    test -z \"$content\" ||\n+    test_expect_success \"--batch without size ($type)\" '\n+\t{\n+\t\techo \"$type\" &&\n+\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t} >expect &&\n+\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual.full &&\n+\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\ttest_cmp expect actual\n+    '\n }\n \n hello_content=\"Hello World\"\n-- \n1.8.5.524.g6743da6\n"},{"id":"231926","messageId":"20131211201112.GM2311@google.com","threadId":"35517","inReplyTo":"20131211115642.GA10594@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] cat-file: pass expand_data to print_object_or_die","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-11T20:11:12Z","receivedAt":"2013-12-11T20:11:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n>                                        However, if we pass\n> the whole struct, the called function can make more\n> intelligent decisions about which fields were actualled\n> filled by sha1_object_info.\n\nThanks.\n\ns/actualled/actually/, I think.\n\nAt first I thought this patch was going to be about making those\nintelligent decisions.  Maybe s/the called function can/a future patch\ncan teach the called function/ or something?\n\n[...]\n> There should be no functional change to this patch.\n\nThe patch itself looks straightforward, yep. :)\n\nWith the typofix mentioned above,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"231927","messageId":"20131211204200.GN2311@google.com","threadId":"35517","inReplyTo":"20131211115844.GB10594@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-11T20:42:00Z","receivedAt":"2013-12-11T20:42:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We could do the same for the type. However, besides our\n> consistency check, we also care about the type in deciding\n> whether to stream or not. We therefore make sure to always\n> trigger a type lookup when we are printing, so that\n\nThis \"We make sure\" is the behavior after this patch, not before,\nright?\n\n[...]\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)\n>  \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n>  \t\tif (type != data->type)\n>  \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n\nMaybe an assert(data.info.typep) or similar would make this more\nlocally readable.\n\n[...]\n> @@ -276,6 +276,13 @@ static int batch_objects(struct batch_options *opt)\n>  \tdata.mark_query = 0;\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> +\t */\n> +\tif (opt->print_contents)\n> +\t\tdata.info.typep = &data.type;\n\nOof.  I guess this means that the optimization from 98e2092b wasn't being\napplied by 'git cat-file --batch' with format specifiers that don't\ninclude %(objecttype), but no one would have noticed because of the\n\"changed type\" thing. :)\n\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -85,6 +85,28 @@ $content\"\n>  \t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n>  \ttest_cmp expect actual\n>      '\n> +\n> +    test -z \"$content\" ||\n> +    test_expect_success \"--batch without type ($type)\" '\n> +\t{\n> +\t\techo \"$size\" &&\n> +\t\tmaybe_remove_timestamp \"$content\" $no_ts\n> +\t} >expect &&\n> +\techo $sha1 | git cat-file --batch=\"%(objectsize)\" >actual.full &&\n> +\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n> +\ttest_cmp expect actual\n> +    '\n> +\n> +    test -z \"$content\" ||\n> +    test_expect_success \"--batch without size ($type)\" '\n> +\t{\n> +\t\techo \"$type\" &&\n> +\t\tmaybe_remove_timestamp \"$content\" $no_ts\n> +\t} >expect &&\n> +\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual.full &&\n> +\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n> +\ttest_cmp expect actual\n> +    '\n>  }\n\nLooks good.\n\n(not about this patch) I suspect a test_cmp_ignore_timestamp helper\ncould simplify these tests somewhat. :)\n\nFor what it's worth, with or without commit message changes or the\ncheck that data->type is initialized,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"231931","messageId":"20131211230142.GA16606@sigill.intra.peff.net","threadId":"35517","inReplyTo":"20131211201112.GM2311@google.com","subject":"Re: [PATCH 1/2] cat-file: pass expand_data to print_object_or_die","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-11T23:01:42Z","receivedAt":"2013-12-11T23:01:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 11, 2013 at 12:11:12PM -0800, Jonathan Nieder wrote:\n\n> >                                        However, if we pass\n> > the whole struct, the called function can make more\n> > intelligent decisions about which fields were actualled\n> > filled by sha1_object_info.\n> \n> Thanks.\n> \n> s/actualled/actually/, I think.\n\nYes. Not sure how I managed that typo.\n\n> At first I thought this patch was going to be about making those\n> intelligent decisions.  Maybe s/the called function can/a future patch\n> can teach the called function/ or something?\n\nI clarified it in the commit message below.\n\n> > There should be no functional change to this patch.\n> \n> The patch itself looks straightforward, yep. :)\n\nIt technically does typo-fix the error message, which I guess is a\nfunctional change. But I didn't count that. :)\n\nHere it is with the commit message fixes and your reviewed-by.\n\n-- >8 --\nSubject: cat-file: pass expand_data to print_object_or_die\n\nWe currently individually pass the sha1, type, and size\nfields calculated by sha1_object_info. However, if we pass\nthe whole struct, the called function can make more\nintelligent decisions about which fields were actually\nfilled by sha1_object_info.\n\nThis patch takes that first refactoring step, passing the\nwhole struct, so further patches can make those decisions\nwith less noise in their diffs. There should be no\nfunctional change to this patch (aside from a minor typo fix\nin the error message).\n\nAs a side effect, we can rename the local variables in the\nfunction to \"type\" and \"size\", since the names are no longer\ntaken.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b2ca775..1434afb 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -193,25 +193,26 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n \treturn end - start + 1;\n }\n \n-static void print_object_or_die(int fd, const unsigned char *sha1,\n-\t\t\t\tenum object_type type, unsigned long size)\n+static void print_object_or_die(int fd, struct expand_data *data)\n {\n-\tif (type == OBJ_BLOB) {\n+\tconst unsigned char *sha1 = data->sha1;\n+\n+\tif (data->type == OBJ_BLOB) {\n \t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n \t}\n \telse {\n-\t\tenum object_type rtype;\n-\t\tunsigned long rsize;\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n \t\tvoid *contents;\n \n-\t\tcontents = read_sha1_file(sha1, &rtype, &rsize);\n+\t\tcontents = read_sha1_file(sha1, &type, &size);\n \t\tif (!contents)\n \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n-\t\tif (rtype != type)\n+\t\tif (type != data->type)\n \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n-\t\tif (rsize != size)\n-\t\t\tdie(\"object %s change size!?\", sha1_to_hex(sha1));\n+\t\tif (size != data->size)\n+\t\t\tdie(\"object %s changed size!?\", sha1_to_hex(sha1));\n \n \t\twrite_or_die(fd, contents, size);\n \t\tfree(contents);\n@@ -250,7 +251,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \tstrbuf_release(&buf);\n \n \tif (opt->print_contents) {\n-\t\tprint_object_or_die(1, data->sha1, data->type, data->size);\n+\t\tprint_object_or_die(1, data);\n \t\twrite_or_die(1, \"\\n\", 1);\n \t}\n \treturn 0;\n-- \n1.8.5.524.g6743da6\n"},{"id":"231932","messageId":"20131211231549.GB16606@sigill.intra.peff.net","threadId":"35517","inReplyTo":"20131211204200.GN2311@google.com","subject":"Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-11T23:15:50Z","receivedAt":"2013-12-11T23:15:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 11, 2013 at 12:42:00PM -0800, Jonathan Nieder wrote:\n\n> > We could do the same for the type. However, besides our\n> > consistency check, we also care about the type in deciding\n> > whether to stream or not. We therefore make sure to always\n> > trigger a type lookup when we are printing, so that\n> \n> This \"We make sure\" is the behavior after this patch, not before,\n> right?\n\nCorrect. I'll clarify that.\n\n> > --- a/builtin/cat-file.c\n> > +++ b/builtin/cat-file.c\n> > @@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)\n> >  \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n> >  \t\tif (type != data->type)\n> >  \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n> \n> Maybe an assert(data.info.typep) or similar would make this more\n> locally readable.\n\nI'm not sure it makes it more readable, but it would protect against\nviolating our assumptions later (and ease people's minds who wonder why\nwe can touch data->type without a similar check).\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> > +\t */\n> > +\tif (opt->print_contents)\n> > +\t\tdata.info.typep = &data.type;\n> \n> Oof.  I guess this means that the optimization from 98e2092b wasn't being\n> applied by 'git cat-file --batch' with format specifiers that don't\n> include %(objecttype), but no one would have noticed because of the\n> \"changed type\" thing. :)\n\nYes. The loss of the optimization was a small thing compared to being\ntotally broken. :)\n\n> > +\t{\n> > +\t\techo \"$type\" &&\n> > +\t\tmaybe_remove_timestamp \"$content\" $no_ts\n> > +\t} >expect &&\n> > +\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual.full &&\n> > +\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n> > +\ttest_cmp expect actual\n> [...]\n> (not about this patch) I suspect a test_cmp_ignore_timestamp helper\n> could simplify these tests somewhat. :)\n\nYeah, the maybe_remove_timestamp is ugly on so many levels. I used it\nbecause it's deeply embedded in the existing tests, and I didn't want to\ntackle refactoring the whole thing. Be my guest if you want to do it on\ntop. :)\n\n> For what it's worth, with or without commit message changes or the\n> check that data->type is initialized,\n> \n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks. Updated patch is below.\n\n-- >8 --\nSubject: cat-file: handle --batch format with missing type/size\n\nCommit 98e2092 taught cat-file to stream blobs with --batch,\nwhich requires that we look up the object type before\nloading it into memory.  As a result, we now print the\nobject header from information in sha1_object_info, and the\nactual contents from the read_sha1_file. We double-check\nthat the information we printed in the header matches the\ncontent we are about to show.\n\nLater, commit 93d2a60 allowed custom header lines for\n--batch, and commit 5b08640 made type lookups optional. As a\nresult, specifying a header line without the type or size\nmeans that we will not look up those items at all.\n\nThis causes our double-checking to erroneously die with an\nerror; we think the type or size has changed, when in fact\nit was simply left at \"0\".\n\nFor the size, we can fix this by only doing the consistency\ndouble-check when we have retrieved the size via\nsha1_object_info. In the case that we have not retrieved the\nvalue, that means we also did not print it, so there is\nnothing for us to check that we are consistent with.\n\nWe could do the same for the type. However, besides our\nconsistency check, we also care about the type in deciding\nwhether to stream or not. So instead of handling the case\nwhere we do not know the type, this patch instead makes sure\nthat we always trigger a type lookup when we are printing,\nso that even a format without the type will stream as we\nwould in the normal case.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c  | 11 ++++++++++-\n t/t1006-cat-file.sh | 22 ++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 1434afb..f8288c8 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -197,6 +197,8 @@ static void print_object_or_die(int fd, struct expand_data *data)\n {\n \tconst unsigned char *sha1 = data->sha1;\n \n+\tassert(data->info.typep);\n+\n \tif (data->type == OBJ_BLOB) {\n \t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n@@ -211,7 +213,7 @@ static void print_object_or_die(int fd, struct expand_data *data)\n \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n \t\tif (type != data->type)\n \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n-\t\tif (size != data->size)\n+\t\tif (data->info.sizep && size != data->size)\n \t\t\tdie(\"object %s changed size!?\", sha1_to_hex(sha1));\n \n \t\twrite_or_die(fd, contents, size);\n@@ -276,6 +278,13 @@ static int batch_objects(struct batch_options *opt)\n \tdata.mark_query = 0;\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+\t */\n+\tif (opt->print_contents)\n+\t\tdata.info.typep = &data.type;\n+\n+\t/*\n \t * We are going to call get_sha1 on a potentially very large number of\n \t * objects. In most large cases, these will be actual object sha1s. The\n \t * cost to double-check that each one is not also a ref (just so we can\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 8a1bc5c..1687098 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -85,6 +85,28 @@ $content\"\n \t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n \ttest_cmp expect actual\n     '\n+\n+    test -z \"$content\" ||\n+    test_expect_success \"--batch without type ($type)\" '\n+\t{\n+\t\techo \"$size\" &&\n+\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t} >expect &&\n+\techo $sha1 | git cat-file --batch=\"%(objectsize)\" >actual.full &&\n+\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\ttest_cmp expect actual\n+    '\n+\n+    test -z \"$content\" ||\n+    test_expect_success \"--batch without size ($type)\" '\n+\t{\n+\t\techo \"$type\" &&\n+\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t} >expect &&\n+\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual.full &&\n+\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\ttest_cmp expect actual\n+    '\n }\n \n hello_content=\"Hello World\"\n-- \n1.8.5.524.g6743da6\n"},{"id":"231934","messageId":"20131211233121.GO2311@google.com","threadId":"35517","inReplyTo":"20131211231549.GB16606@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-11T23:31:21Z","receivedAt":"2013-12-11T23:31:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Updated patch is below.\n\nThanks!  v2 of both patches looks good.\n"},{"id":"231938","messageId":"7vmwk6u5xp.fsf@alter.siamese.dyndns.org","threadId":"35517","inReplyTo":"20131211230142.GA16606@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] cat-file: pass expand_data to print_object_or_die","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-12T03:03:14Z","receivedAt":"2013-12-12T03:03:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It technically does typo-fix the error message, which I guess is a\n> functional change. But I didn't count that. :)\n>\n> Here it is with the commit message fixes and your reviewed-by.\n\nThanks, both.\n\nWill queue, to eventually merge to 'maint'.\n\n>\n> -- >8 --\n> Subject: cat-file: pass expand_data to print_object_or_die\n>\n> We currently individually pass the sha1, type, and size\n> fields calculated by sha1_object_info. However, if we pass\n> the whole struct, the called function can make more\n> intelligent decisions about which fields were actually\n> filled by sha1_object_info.\n>\n> This patch takes that first refactoring step, passing the\n> whole struct, so further patches can make those decisions\n> with less noise in their diffs. There should be no\n> functional change to this patch (aside from a minor typo fix\n> in the error message).\n>\n> As a side effect, we can rename the local variables in the\n> function to \"type\" and \"size\", since the names are no longer\n> taken.\n>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/cat-file.c | 21 +++++++++++----------\n>  1 file changed, 11 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index b2ca775..1434afb 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -193,25 +193,26 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n>  \treturn end - start + 1;\n>  }\n>  \n> -static void print_object_or_die(int fd, const unsigned char *sha1,\n> -\t\t\t\tenum object_type type, unsigned long size)\n> +static void print_object_or_die(int fd, struct expand_data *data)\n>  {\n> -\tif (type == OBJ_BLOB) {\n> +\tconst unsigned char *sha1 = data->sha1;\n> +\n> +\tif (data->type == OBJ_BLOB) {\n>  \t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n>  \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n>  \t}\n>  \telse {\n> -\t\tenum object_type rtype;\n> -\t\tunsigned long rsize;\n> +\t\tenum object_type type;\n> +\t\tunsigned long size;\n>  \t\tvoid *contents;\n>  \n> -\t\tcontents = read_sha1_file(sha1, &rtype, &rsize);\n> +\t\tcontents = read_sha1_file(sha1, &type, &size);\n>  \t\tif (!contents)\n>  \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n> -\t\tif (rtype != type)\n> +\t\tif (type != data->type)\n>  \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n> -\t\tif (rsize != size)\n> -\t\t\tdie(\"object %s change size!?\", sha1_to_hex(sha1));\n> +\t\tif (size != data->size)\n> +\t\t\tdie(\"object %s changed size!?\", sha1_to_hex(sha1));\n>  \n>  \t\twrite_or_die(fd, contents, size);\n>  \t\tfree(contents);\n> @@ -250,7 +251,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n>  \tstrbuf_release(&buf);\n>  \n>  \tif (opt->print_contents) {\n> -\t\tprint_object_or_die(1, data->sha1, data->type, data->size);\n> +\t\tprint_object_or_die(1, data);\n>  \t\twrite_or_die(1, \"\\n\", 1);\n>  \t}\n>  \treturn 0;\n"},{"id":"231939","messageId":"7viouuu5th.fsf@alter.siamese.dyndns.org","threadId":"35517","inReplyTo":"20131211231549.GB16606@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-12T03:05:46Z","receivedAt":"2013-12-12T03:05:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yes. The loss of the optimization was a small thing compared to being\n> totally broken. :)\n> ...\n>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> Thanks. Updated patch is below.\n\n;-)\n\nI like it when I see patches are polished between the submitter and\nreviewer(s) fully, before the maintainer has a chance to pick an\nintermediate version (only to later replace and requeue).\n\nThanks, both.\n"}]}