{"thread":{"id":"35285","subject":"[PATCH] git-cat-file: fix output when format string contains no variables","startedAt":"2013-11-05T20:03:11Z","lastAt":"2013-11-06T19:40:49Z","messageCount":4,"participants":["Sven Brauch","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"230127","messageId":"1748846.6BAMC5enOx@localhost.localdomain","threadId":"35285","inReplyTo":null,"subject":"[PATCH] git-cat-file: fix output when format string contains no variables","fromName":"Sven Brauch","fromEmail":"svenbrauch@googlemail.com","sentAt":"2013-11-05T20:03:11Z","receivedAt":"2013-11-05T20:03:11Z","isPatch":true,"sender":{"key":"svenbrauch@googlemail.com","avatar":null},"body":">From 2e7b5aed771faeff654a447346bb0b57570d9569 Mon Sep 17 00:00:00 2001\nFrom: Sven Brauch \nDate: Tue, 5 Nov 2013 20:06:21 +0100\nSubject: [PATCH] git-cat-file: fix output when format string contains no\n variables\n\nWhen the format string for git-cat-object --batch-check contained no\nvariables, the function would not actually look for the object on disk,\nbut just verify that the hash is correct. Thus it would report no error\nif asking for objects which did not actually exist on disk if the SHA hash\nlooked ok.\n\nExample of buggy behaviour:\necho \"XYZ\" | git hash-object --stdin | git cat-file --batch-check=\"found\"\nwould report \"found\" although the manpage claims it would report an error.\n\nSigned-off-by: Sven Brauch \n---\n\nNotes:\n This fixes a bug where git-cat-file --batch-check would erroneously tell\n that objects exist while they did in fact not in case the argument to\n --batch-check was just a constant strig (i.e. no %(...) variables).\n The reason was that calling sha1_object_info_extended while requesting no\n properties of the object would not even verify this object existed, or more\n exactly, sha1_loose_object_info would not do that.\n \n I'm entirely unfamiliar with the git codebase; the suggested fix ensures\n that always at least one property is requested. If there's a better way\n to fix this issue, please let me know.\n\n builtin/cat-file.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b2ca775..32d0b63 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -238,6 +238,15 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\treturn 0;\n \t}\n \n+\tif (!data->info.disk_sizep && !data->info.sizep && !data->info.typep) {\n+\t\t/*\n+\t\t * None of the info fields was requested, so we request the cheapest\n+\t\t * one (disk_sizep) to force sha1_object_info_extended to actually\n+\t\t * look for the file.\n+\t\t */\n+\t\tdata->info.disk_sizep = &data->disk_size;\n+\t}\n+\n \tif (sha1_object_info_extended(data->sha1, &data->info) < 0) {\n \t\tprintf(\"%s missing\\n\", obj_name);\n \t\tfflush(stdout);\n-- \n1.8.4.2\n"},{"id":"230147","messageId":"xmqqsiv98l5y.fsf@gitster.dls.corp.google.com","threadId":"35285","inReplyTo":"1748846.6BAMC5enOx@localhost.localdomain","subject":"Re: [PATCH] git-cat-file: fix output when format string contains no variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-06T18:00:57Z","receivedAt":"2013-11-06T18:00:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sven Brauch writes:\n\n> From 2e7b5aed771faeff654a447346bb0b57570d9569 Mon Sep 17 00:00:00 2001\n> From: Sven Brauch \n> Date: Tue, 5 Nov 2013 20:06:21 +0100\n> Subject: [PATCH] git-cat-file: fix output when format string contains no\n> variables\n\nThanks; first some procedural issues:\n\n - Omit the first line \"From 2e7b...\"; that does not belong to any\n patch submission (it is a separator used between messages in the\n mailbox formatted file \"format-patch --stdout\" produces).\n\n - The second line \"From: Sven...\" records exactly the same address\n as the e-mail you are sending out, so it should be omitted as\n well.\n\n - The third line \"Date: ...\" is not the time we the general public\n sees your fix for the first time, which is what \"Date: \" of your\n e-mail header already records, so we do not need it either.\n\n - And the last one \"Subject: ...\" is redundant; we can see it in\n your e-mail header.\n\nIn general, the latter three lines are produced by format-patch to\nhelp you fill header fields in the MUA of your choice by cutting\n(not copying) and pasting. Unless there is a valid reason to have\nvalues different from what recipients would see in the e-mail header\n(and there often isn't, unless you are forwarding somebody else's\npatch, in which case you may want to use \"From: \", or you are\nresponding to an ongoing discussion with a patch, in which case you\nmay want to use \"Subject: \"), please remove them after copying them\nout to your e-mail header.\n\n> When the format string for git-cat-object --batch-check contained no\n> variables, the function would not actually look for the object on disk,\n> but just verify that the hash is correct. Thus it would report no error\n> if asking for objects which did not actually exist on disk if the SHA hash\n> looked ok.\n>\n> Example of buggy behaviour:\n> echo \"XYZ\" | git hash-object --stdin | git cat-file --batch-check=\"found\"\n> would report \"found\" although the manpage claims it would report an error.\n\nAn excellent log message. It would have been even better to add a\nnew test to t1006 based on this reproduction recipe.\n\n> Signed-off-by: Sven Brauch \n> ---\n>\n> Notes:\n> This fixes a bug where git-cat-file --batch-check would erroneously tell\n> that objects exist while they did in fact not in case the argument to\n> --batch-check was just a constant strig (i.e. no %(...) variables).\n> The reason was that calling sha1_object_info_extended while requesting no\n> properties of the object would not even verify this object existed, or more\n> exactly, sha1_loose_object_info would not do that.\n> \n> I'm entirely unfamiliar with the git codebase; the suggested fix ensures\n> that always at least one property is requested. If there's a better way\n> to fix this issue, please let me know.\n\nI think the real problem is that sha1_loose_object_info() is called\nby sha1_object_info_extended(), when it does not find a cached or a\npacked object, and the callee assumes that it is asked to fill in\nonly the requested pieces of information while the caller does not\neven bother to check if such an object actually exists.\n\nHow about doing it like the attached instead?\n\n-- >8 --\nSubject: sha1_loose_object_info(): do not return success on missing object\n\nSince 052fe5ea (sha1_loose_object_info: make type lookup optional,\n2013-07-12), sha1_loose_object_info() returns happily without\nchecking if the object in question exists, which is not what the the\ncaller sha1_object_info_extended() expects; the caller does not even\nbother checking the existence of the object itself.\n\nNoticed-by: Sven Brauch \nSigned-off-by: Junio C Hamano \n---\n\n Oh, by the way, there is this one iffy bit in batch_one_object():\n\n if (get_sha1(obj_name, data->sha1)) {\n printf(\"%s missing\\n\", obj_name);\n fflush(stdout);\n return 0;\n }\n\n At this point, the object _may_ be missing, but the obj_name may be\n malformed, so saying \"missing\" is not strictly correct. If, for\n example, you misspelled the name of the master branch, you would get\n this:\n\n $ echo mastre | git cat-file --batch-check=foo\n mastre missing\n\n I however doubt that it is a good idea to reword this message by\n adding a logic to tell misspelled object name and missing object\n name apart. The users of \"cat-file --batch-check\" are not\n expecting to be able to distinguish these two classes of errors\n anyway.\n\n sha1_file.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7dadd04..00220a4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2486,12 +2486,11 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \t * need to look inside the object at all.\n \t */\n \tif (!oi->typep && !oi->sizep) {\n-\t\tif (oi->disk_sizep) {\n-\t\t\tstruct stat st;\n-\t\t\tif (stat_sha1_file(sha1, &st) < 0)\n-\t\t\t\treturn -1;\n+\t\tstruct stat st;\n+\t\tif (stat_sha1_file(sha1, &st) < 0)\n+\t\t\treturn -1;\n+\t\tif (oi->disk_sizep)\n \t\t\t*oi->disk_sizep = st.st_size;\n-\t\t}\n \t\treturn 0;\n \t}\n \n"},{"id":"230148","messageId":"20131106183121.GA2566@sigill.intra.peff.net","threadId":"35285","inReplyTo":"xmqqsiv98l5y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-cat-file: fix output when format string contains no variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-06T18:31:21Z","receivedAt":"2013-11-06T18:31:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 06, 2013 at 10:00:57AM -0800, Junio C Hamano wrote:\n\n> I think the real problem is that sha1_loose_object_info() is called\n> by sha1_object_info_extended(), when it does not find a cached or a\n> packed object, and the callee assumes that it is asked to fill in\n> only the requested pieces of information while the caller does not\n> even bother to check if such an object actually exists.\n\nYes, exactly. This is over-optimization on the part of my 052fe5ea. The\ncaller asked for \"nothing\", so we happily optimize the request to do\nnothing. But I forgot there is always an implicit \"does it even exist\"\nquery in the call. I do not see any point in adding an \"exists\" query\nfield to \"struct object_info\". :)\n\n> -- >8 --\n> Subject: sha1_loose_object_info(): do not return success on missing object\n> \n> Since 052fe5ea (sha1_loose_object_info: make type lookup optional,\n> 2013-07-12), sha1_loose_object_info() returns happily without\n> checking if the object in question exists, which is not what the the\n> caller sha1_object_info_extended() expects; the caller does not even\n> bother checking the existence of the object itself.\n> \n> Noticed-by: Sven Brauch \n> Signed-off-by: Junio C Hamano \n\nThis is definitely the right fix. Commit message and patch look good to\nme. A few extra bits are in the patch below.\n\n> Oh, by the way, there is this one iffy bit in batch_one_object():\n> \n> if (get_sha1(obj_name, data->sha1)) {\n> printf(\"%s missing\\n\", obj_name);\n> fflush(stdout);\n> return 0;\n> }\n> \n> At this point, the object _may_ be missing, but the obj_name may be\n> malformed, so saying \"missing\" is not strictly correct. If, for\n> example, you misspelled the name of the master branch, you would get\n> this:\n> \n> $ echo mastre | git cat-file --batch-check=foo\n> mastre missing\n> \n> I however doubt that it is a good idea to reword this message by\n> adding a logic to tell misspelled object name and missing object\n> name apart. The users of \"cat-file --batch-check\" are not\n> expecting to be able to distinguish these two classes of errors\n> anyway.\n\nYes, I noticed that while doing the original series, but left it intact\nfor the exact reason you mention. Note that it is going to stdout and is\npart of the actual data stream (so there is a good chance scripts are\nparsing it).\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 7dadd04..00220a4 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2486,12 +2486,11 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n> \t * need to look inside the object at all.\n> \t */\n> \tif (!oi->typep && !oi->sizep) {\n> -\t\tif (oi->disk_sizep) {\n> -\t\t\tstruct stat st;\n> -\t\t\tif (stat_sha1_file(sha1, &st) < 0)\n> -\t\t\t\treturn -1;\n> +\t\tstruct stat st;\n> +\t\tif (stat_sha1_file(sha1, &st) < 0)\n> +\t\t\treturn -1;\n> +\t\tif (oi->disk_sizep)\n> \t\t\t*oi->disk_sizep = st.st_size;\n> -\t\t}\n> \t\treturn 0;\n\nHere's a squashable patch which expands the comment above the code\nyou're tweaking, and adds a test to t1006.\n\nI notice that t1006 could use some modernization, whitespace-fixing, and\ntypo-fixing, but I left that out of the patch for clarity.\n\nThanks both of you for working on this.\n\n-Peff\n\n---\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3474dca..10676ba 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2489,7 +2489,11 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \n \t/*\n \t * If we don't care about type or size, then we don't\n-\t * need to look inside the object at all.\n+\t * need to look inside the object at all. Note that we\n+\t * do not optimize out the stat call, even if the\n+\t * caller doesn't care about the disk-size, since our\n+\t * return value implicitly indicates whether the\n+\t * object even exists.\n \t */\n \tif (!oi->typep && !oi->sizep) {\n \t\tstruct stat st;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex a420742..8a1bc5c 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -194,6 +194,12 @@ test_expect_success \"--batch-check for an emtpy line\" '\n test \" missing\" = \"$(echo | git cat-file --batch-check)\"\n '\n \n+test_expect_success 'empty --batch-check notices missing object' '\n+\techo \"$_z40 missing\" >expect &&\n+\techo \"$_z40\" | git cat-file --batch-check=\"\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n batch_input=\"$hello_sha1\n $commit_sha1\n $tag_sha1\n"},{"id":"230151","messageId":"5085823.Oad8MOHc71@localhost.localdomain","threadId":"35285","inReplyTo":"xmqqsiv98l5y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-cat-file: fix output when format string contains no variables","fromName":"Sven Brauch","fromEmail":"svenbrauch@googlemail.com","sentAt":"2013-11-06T19:40:49Z","receivedAt":"2013-11-06T19:40:49Z","isPatch":true,"sender":{"key":"svenbrauch@googlemail.com","avatar":null},"body":"On Wednesday 06 November 2013 10:00:57 Junio C Hamano wrote:\n> Thanks; first some procedural issues:\nThanks, I will take care of the mentioned points for future submissions.\n\n> I think the real problem is that sha1_loose_object_info() is called\n> by sha1_object_info_extended(), when it does not find a cached or a\n> packed object, and the callee assumes that it is asked to fill in\n> only the requested pieces of information while the caller does not\n> even bother to check if such an object actually exists.\n> \n> How about doing it like the attached instead?\nYes; this seems more like a proper fix. I would prefer it over my suggestion.\nIt is illogical that sha1_loose_object_info sometimes returns an error if the \nobject does not exist and sometimes not, depending on which properties are \nrequested.\n\nSven\n"}]}