{"thread":{"id":"13869","subject":"[PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","startedAt":"2008-06-08T23:28:37Z","lastAt":"2008-06-09T20:42:57Z","messageCount":12,"participants":["Lea Wiemann","Johannes Schindelin","Jakub Narebski","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"79187","messageId":"1212967717-5165-1-git-send-email-LeWiemann@gmail.com","threadId":"13869","inReplyTo":null,"subject":"[PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-08T23:28:37Z","receivedAt":"2008-06-08T23:28:37Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Previously, cat-file --batch / --batch-check would silently exit if it\nwas passed a non-existent SHA1 on stdin.  Now it prints \"<SHA1>\nmissing\" as in all other cases (and as advertised in the\ndocumentation).\n\nNote that cat-file --batch-check (but not --batch) will still output\n\"error: unable to find <SHA1>\" on stderr if a non-existent SHA1 is\npassed, but this does not affect parsing its stdout.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\n builtin-cat-file.c  |    7 +++++--\n t/t1006-cat-file.sh |   15 ++++++++++++---\n 2 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-cat-file.c b/builtin-cat-file.c\nindex f8b3160..112b9db 100644\n--- a/builtin-cat-file.c\n+++ b/builtin-cat-file.c\n@@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \telse\n \t\ttype = sha1_object_info(sha1, &size);\n \n-\tif (type <= 0)\n-\t\treturn 1;\n+\tif (type <= 0) {\n+\t\tprintf(\"%s missing\\n\", obj_name);\n+\t\tfflush(stdout);\n+\t\treturn 0;\n+\t}\n \n \tprintf(\"%s %s %lu\\n\", sha1_to_hex(sha1), typename(type), size);\n \tfflush(stdout);\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 973aef7..04c3be0 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -174,9 +174,18 @@ do\n     '\n done\n \n-test_expect_success \"--batch-check for a non-existent object\" '\n-    test \"deadbeef missing\" = \\\n-    \"$(echo_without_newline deadbeef | git cat-file --batch-check)\"\n+test_expect_success \"--batch-check for a non-existent named object\" '\n+    test \"foobar42 missing\n+foobar84 missing\" = \\\n+    \"$(( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)\"\n+'\n+\n+test_expect_success \"--batch-check for a non-existent hash\" '\n+    test \"0000000000000000000000000000000000000042 missing\n+0000000000000000000000000000000000000084 missing\" = \\\n+    \"$(( echo 0000000000000000000000000000000000000042;\n+         echo_without_newline 0000000000000000000000000000000000000084; ) \\\n+       | git cat-file --batch-check)\"\n '\n \n test_expect_success \"--batch-check for an emtpy line\" '\n-- \n1.5.6.rc1.20.g9e7c7e.dirty\n"},{"id":"79188","messageId":"alpine.DEB.1.00.0806090033030.1783@racer","threadId":"13869","inReplyTo":"1212967717-5165-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-08T23:34:43Z","receivedAt":"2008-06-08T23:34:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nmaybe s/do not exit/complain/?  Because it still exits if the hashes are \nmissing...\n\n> diff --git a/builtin-cat-file.c b/builtin-cat-file.c\n> index f8b3160..112b9db 100644\n> --- a/builtin-cat-file.c\n> +++ b/builtin-cat-file.c\n> @@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents)\n>  \telse\n>  \t\ttype = sha1_object_info(sha1, &size);\n>  \n> -\tif (type <= 0)\n> -\t\treturn 1;\n> +\tif (type <= 0) {\n> +\t\tprintf(\"%s missing\\n\", obj_name);\n> +\t\tfflush(stdout);\n> +\t\treturn 0;\n> +\t}\n\nIs it really intended that it returns 0 now?\n\nFurther, should it not be an error(\"%s missin...  instead?  It is an \nerror, isn't it?\n\nCiao,\nDscho\n"},{"id":"79189","messageId":"1212969741-24899-1-git-send-email-LeWiemann@gmail.com","threadId":"13869","inReplyTo":"1212967717-5165-1-git-send-email-LeWiemann@gmail.com","subject":"[PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-09T00:02:21Z","receivedAt":"2008-06-09T00:02:21Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Previously, cat-file --batch / --batch-check would silently exit if it\nwas passed a non-existent SHA1 on stdin.  Now it prints \"<SHA1>\nmissing\" as in all other cases (and as advertised in the\ndocumentation).\n\nNote that cat-file --batch-check (but not --batch) will still output\n\"error: unable to find <SHA1>\" on stderr if a non-existent SHA1 is\npassed, but this does not affect parsing its stdout.\n\nAlso, type <= 0 was previously using the potentially uninitialized\ntype variable (relying on it being 0); it is now being initialized.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\nChanged since v1: initialize type variable, and add a test case that\nhas a good chance of catching this.\n\n builtin-cat-file.c  |    9 ++++++---\n t/t1006-cat-file.sh |   24 +++++++++++++++++++++---\n 2 files changed, 27 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-cat-file.c b/builtin-cat-file.c\nindex f8b3160..bd343ef 100644\n--- a/builtin-cat-file.c\n+++ b/builtin-cat-file.c\n@@ -150,7 +150,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n static int batch_one_object(const char *obj_name, int print_contents)\n {\n \tunsigned char sha1[20];\n-\tenum object_type type;\n+\tenum object_type type = 0;\n \tunsigned long size;\n \tvoid *contents = contents;\n \n@@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \telse\n \t\ttype = sha1_object_info(sha1, &size);\n \n-\tif (type <= 0)\n-\t\treturn 1;\n+\tif (type <= 0) {\n+\t\tprintf(\"%s missing\\n\", obj_name);\n+\t\tfflush(stdout);\n+\t\treturn 0;\n+\t}\n \n \tprintf(\"%s %s %lu\\n\", sha1_to_hex(sha1), typename(type), size);\n \tfflush(stdout);\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 973aef7..542b4ae 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -174,9 +174,27 @@ do\n     '\n done\n \n-test_expect_success \"--batch-check for a non-existent object\" '\n-    test \"deadbeef missing\" = \\\n-    \"$(echo_without_newline deadbeef | git cat-file --batch-check)\"\n+test_expect_success \"--batch-check for a non-existent named object\" '\n+    test \"foobar42 missing\n+foobar84 missing\" = \\\n+    \"$(( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)\"\n+'\n+\n+test_expect_success \"--batch-check for a non-existent hash\" '\n+    test \"0000000000000000000000000000000000000042 missing\n+0000000000000000000000000000000000000084 missing\" = \\\n+    \"$(( echo 0000000000000000000000000000000000000042;\n+         echo_without_newline 0000000000000000000000000000000000000084; ) \\\n+       | git cat-file --batch-check)\"\n+'\n+\n+test_expect_success \"--batch for an existent and a non-existent hash\" '\n+    test \"$tag_sha1 tag $tag_size\n+$tag_content\n+0000000000000000000000000000000000000000 missing\" = \\\n+    \"$(( echo $tag_sha1;\n+         echo_without_newline 0000000000000000000000000000000000000000; ) \\\n+       | git cat-file --batch)\"\n '\n \n test_expect_success \"--batch-check for an emtpy line\" '\n-- \n1.5.6.rc1.21.gc5a36.dirty\n"},{"id":"79190","messageId":"484C73DA.10804@gmail.com","threadId":"13869","inReplyTo":"alpine.DEB.1.00.0806090033030.1783@racer","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-09T00:05:46Z","receivedAt":"2008-06-09T00:05:46Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Johannes Schindelin wrote:\n> Lea Wiemann wrote:\n>> -\tif (type <= 0)\n>> -\t\treturn 1;\n>> +\tif (type <= 0) {\n>> +\t\tprintf(\"%s missing\\n\", obj_name);\n>> +\t\tfflush(stdout);\n>> +\t\treturn 0;\n>> +\t}\n> \n> Is it really intended that it returns 0 now?\n\nYes, return 0 causes it to not exit (which is the desired effect, as in \ndocumented).\n\n> Further, should it not be an error(\"%s missin...  instead?  It is an \n> error, isn't it?\n\nNo, it's not an error.  I suggest you read git-cat-file.1 -- getting a \nresponse of \"<object> missing\" (rather than cat-file terminating) if you \npass in a non-existent object is the desired behavior, and it happened \nbefore for named objects, like \"HEAD\", but not for full hashes.  Now it \nalso happens for full hashes.\n\n-- Lea\n"},{"id":"79196","messageId":"alpine.DEB.1.00.0806090201090.1783@racer","threadId":"13869","inReplyTo":"484C73DA.10804@gmail.com","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-09T01:02:14Z","receivedAt":"2008-06-09T01:02:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 9 Jun 2008, Lea Wiemann wrote:\n\n> Johannes Schindelin wrote:\n> > Lea Wiemann wrote:\n> > > -\tif (type <= 0)\n> > > -\t\treturn 1;\n> > > +\tif (type <= 0) {\n> > > +\t\tprintf(\"%s missing\\n\", obj_name);\n> > > +\t\tfflush(stdout);\n> > > +\t\treturn 0;\n> > > +\t}\n> > \n> > Is it really intended that it returns 0 now?\n> \n> Yes, return 0 causes it to not exit (which is the desired effect, as in\n> documented).\n\nThat is unexpected.  IMO this is a bug.  rev-parse I can understand, but \ncat-file not failing when it clearly did not find the object?\n\nCiao,\nDscho\n"},{"id":"79216","messageId":"484D033A.3020006@gmail.com","threadId":"13869","inReplyTo":"alpine.DEB.1.00.0806090201090.1783@racer","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-09T10:17:30Z","receivedAt":"2008-06-09T10:17:30Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Johannes Schindelin wrote:\n> That is unexpected.  IMO this is a bug.  rev-parse I can understand, but \n> cat-file not failing when it clearly did not find the object?\n\n From a recent IRC conversation (with permission):\n\nDscho thinks that git-cat-file --batch should print an error and exit if \npassed an invalid revision (as opposed to the current behavior of \nprinting \"<object> missing\" and continuing), since anything else would \nbe unexpected. [1]  He says that an --ignore-missing option should be \nintroduced instead, and cat-file --batch should exit on non-existent \nobjects unless the --ignore-missing option is given.\n\nI maintain that my patch isn't introducing a new feature but fixing a \nbug -- cat-file --batch already doesn't exit but (as documented) prints \n\"<object> missing\" if passed a non-existent object.  The only exception \nis a full (= 40-character) SHA1 that isn't augmented (e.g. by \"^\" etc.). \n  This exception is neither useful, expected, nor documented.  Hence, my \npatch fixes this inconsistency.\n\nIf Dscho (or anybody else) wants to introduce an --ignore-missing \noption, feel free to submit it separately, but please don't object to my \npatch because of it -- my patch is merely fixing the existing code. \n(Note that such an option is unlikely to make it into the code though, \nsince it would have to change the existing behavior of cat-file --batch, \nwhich other programs rely on.)\n\n-- Lea\n\n[1] He says he hasn't read the manual so far though because he doesn't \nhave time for it -- if he had read it (as I repeatedly recommended) he \nprobably wouldn't find the behavior surprising.\n"},{"id":"79217","messageId":"m3lk1ex3s6.fsf@localhost.localdomain","threadId":"13869","inReplyTo":"484D033A.3020006@gmail.com","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-09T11:07:14Z","receivedAt":"2008-06-09T11:07:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n>  From a recent IRC conversation (with permission):\n> \n> Dscho thinks that git-cat-file --batch should print an error and exit\n> if passed an invalid revision (as opposed to the current behavior of\n> printing \"<object> missing\" and continuing), since anything else would\n> be unexpected. [1]  He says that an --ignore-missing option should be\n> introduced instead, and cat-file --batch should exit on non-existent\n> objects unless the --ignore-missing option is given.\n[...]\n\n> If Dscho (or anybody else) wants to introduce an --ignore-missing\n> option, feel free to submit it separately, but please don't object to\n> my patch because of it -- my patch is merely fixing the existing\n> code. (Note that such an option is unlikely to make it into the code\n> though, since it would have to change the existing behavior of\n> cat-file --batch, which other programs rely on.)\n\nI think the (usual) solution is to add --ignore-missing and\n--no-ignore-missing (or --noignore-missing), add configuration\noption 'catfile.ignoreMissing', make ignore-missing default and\ndeprecate it with some transition time...\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"79222","messageId":"484D2FBA.1010606@gmail.com","threadId":"13869","inReplyTo":"m3lk1ex3s6.fsf@localhost.localdomain","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-09T13:27:22Z","receivedAt":"2008-06-09T13:27:22Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> I think the (usual) solution is to add --ignore-missing and\n> --no-ignore-missing (or --noignore-missing), add configuration\n> option 'catfile.ignoreMissing', make ignore-missing default and\n> deprecate it with some transition time...\n\n*shrugs*  I'm not convinced that anyone would ever *not* want to use \n--ignore-missing, since programs that use --batch(-check) presumably \nquery for a large batch of objects and therefore define their own error \nhandling anyway.  Also, it's not like you could accidentally ignore the \n\"<object> missing\" output in your code since you're parsing the output \nanyway.  So I'm not sure what the point of changing the current behavior \nis.  (Perhaps add a --exit-on-missing option?  Not that I or anyone else \nhas ever asked for that feature.)\n\nIn any case, can please someone review my patch?  I think my patch \nshould be independent of these things (since it's a bug fix), and I'd \nhate to see it not make it into the code just because some people don't \nseem to like cat-file's behavior. ;-)  (I need it for the Perl API, \nafter all.)\n\n-- Lea\n"},{"id":"79228","messageId":"484D3CCB.1000508@viscovery.net","threadId":"13869","inReplyTo":"1212967717-5165-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-06-09T14:23:07Z","receivedAt":"2008-06-09T14:23:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Lea Wiemann schrieb:\n> Previously, cat-file --batch / --batch-check would silently exit if it\n> was passed a non-existent SHA1 on stdin.  Now it prints \"<SHA1>\n> missing\" as in all other cases (and as advertised in the\n> documentation).\n\nMakes sense!\n\n> diff --git a/builtin-cat-file.c b/builtin-cat-file.c\n> index f8b3160..112b9db 100644\n> --- a/builtin-cat-file.c\n> +++ b/builtin-cat-file.c\n> @@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents)\n>  \telse\n>  \t\ttype = sha1_object_info(sha1, &size);\n>  \n> -\tif (type <= 0)\n> -\t\treturn 1;\n> +\tif (type <= 0) {\n> +\t\tprintf(\"%s missing\\n\", obj_name);\n> +\t\tfflush(stdout);\n> +\t\treturn 0;\n> +\t}\n\nWith this change an invalid name and a non-existing SHA1 are treated as\n\"missing\". But an empty name is still an error, which we don't see here\nbecause it's outside the hunk's context: It is the first thing that the\nfunction checks, and there it exits with return 1. Why is this case still\nspecial?\n\n-- Hannes\n"},{"id":"79249","messageId":"484D7495.3020407@gmail.com","threadId":"13869","inReplyTo":"484D3CCB.1000508@viscovery.net","subject":"Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-09T18:21:09Z","receivedAt":"2008-06-09T18:21:09Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Johannes Sixt wrote:\n> With this change an invalid name and a non-existing SHA1 are treated as\n> \"missing\". But an empty name is still an error  [...]: It is the first thing\n> that the function checks, and there it exits with return 1.\n\nAn empty object name is actually not an error but just yields \"SP \nmissing\".  The \"if (!obj_name) return 1;\" statement doesn't dereference \nobj_name, so it checks for null pointers.  However, batch_one_object \ngets called with buf.buf as its obj_name parameter, which according to \napi-strbuf.txt is never NULL.  So probably the check is pointless and \ncan be removed (the tests still pass); someone more knowledgeable than \nme should confirm this though.\n\n(FWIW, perhaps the original author intended to catch empty input this \nway, but I'm not convinced that this is even sensible.)\n\nUnrelatedly, does anyone know how to stop sha1_object_info from barfing \non non-existent hashes?  It causes the annoying \"error: unable to find \n000...\" on stderr if you run \"echo \n0000000000000000000000000000000000000000 | git-cat-file --batch-check\".\n\n-- Lea\n"},{"id":"79263","messageId":"7v8wxegxae.fsf@gitster.siamese.dyndns.org","threadId":"13869","inReplyTo":"1212969741-24899-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-09T20:37:13Z","receivedAt":"2008-06-09T20:37:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> Previously, cat-file --batch / --batch-check would silently exit if it\n> was passed a non-existent SHA1 on stdin.  Now it prints \"<SHA1>\n> missing\" as in all other cases (and as advertised in the\n> documentation).\n\nI think this change makes sense, given that --batch is about having an\nobject server a read-only process can drive over bi-di pipe.\n\nThanks.\n"},{"id":"79264","messageId":"7vwskyfige.fsf@gitster.siamese.dyndns.org","threadId":"13869","inReplyTo":"1212969741-24899-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-09T20:42:57Z","receivedAt":"2008-06-09T20:42:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 973aef7..542b4ae 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -174,9 +174,27 @@ do\n> ...\n> +foobar84 missing\" = \\\n> +    \"$(( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)\"\n\n\nI have a slight suspicion that some of the Bourne shell implementations we\nsupport may misinterpret $(( as beginning of arithmetic expansion, so\nperhaps we would want an extra SP between the open parens.\n\nOther than that, looked ok.\n"}]}