# [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing 12 messages from 2008-06-08 to 2008-06-09. Participants: Lea Wiemann, Johannes Schindelin, Jakub Narebski, Johannes Sixt, Junio C Hamano. Thread: https://gitlist.dev/t/13869 ## Lea Wiemann, 2008-06-08 23:28 Subject: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <1212967717-5165-1-git-send-email-LeWiemann@gmail.com> URL: https://gitlist.dev/e/1212967717-5165-1-git-send-email-LeWiemann%40gmail.com ``` Previously, cat-file --batch / --batch-check would silently exit if it was passed a non-existent SHA1 on stdin. Now it prints " missing" as in all other cases (and as advertised in the documentation). Note that cat-file --batch-check (but not --batch) will still output "error: unable to find " on stderr if a non-existent SHA1 is passed, but this does not affect parsing its stdout. Signed-off-by: Lea Wiemann --- builtin-cat-file.c | 7 +++++-- t/t1006-cat-file.sh | 15 ++++++++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/builtin-cat-file.c b/builtin-cat-file.c index f8b3160..112b9db 100644 --- a/builtin-cat-file.c +++ b/builtin-cat-file.c @@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents) else type = sha1_object_info(sha1, &size); - if (type <= 0) - return 1; + if (type <= 0) { + printf("%s missing\n", obj_name); + fflush(stdout); + return 0; + } printf("%s %s %lu\n", sha1_to_hex(sha1), typename(type), size); fflush(stdout); diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 973aef7..04c3be0 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -174,9 +174,18 @@ do ' done -test_expect_success "--batch-check for a non-existent object" ' - test "deadbeef missing" = \ - "$(echo_without_newline deadbeef | git cat-file --batch-check)" +test_expect_success "--batch-check for a non-existent named object" ' + test "foobar42 missing +foobar84 missing" = \ + "$(( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)" +' + +test_expect_success "--batch-check for a non-existent hash" ' + test "0000000000000000000000000000000000000042 missing +0000000000000000000000000000000000000084 missing" = \ + "$(( echo 0000000000000000000000000000000000000042; + echo_without_newline 0000000000000000000000000000000000000084; ) \ + | git cat-file --batch-check)" ' test_expect_success "--batch-check for an emtpy line" ' -- 1.5.6.rc1.20.g9e7c7e.dirty ``` ## Johannes Schindelin, 2008-06-08 23:34 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0806090033030.1783%40racer In-Reply-To: <1212967717-5165-1-git-send-email-LeWiemann@gmail.com> ``` Hi, maybe s/do not exit/complain/? Because it still exits if the hashes are missing... > diff --git a/builtin-cat-file.c b/builtin-cat-file.c > index f8b3160..112b9db 100644 > --- a/builtin-cat-file.c > +++ b/builtin-cat-file.c > @@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents) > else > type = sha1_object_info(sha1, &size); > > - if (type <= 0) > - return 1; > + if (type <= 0) { > + printf("%s missing\n", obj_name); > + fflush(stdout); > + return 0; > + } Is it really intended that it returns 0 now? Further, should it not be an error("%s missin... instead? It is an error, isn't it? Ciao, Dscho ``` ## Lea Wiemann, 2008-06-09 00:02 Subject: [PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <1212969741-24899-1-git-send-email-LeWiemann@gmail.com> URL: https://gitlist.dev/e/1212969741-24899-1-git-send-email-LeWiemann%40gmail.com In-Reply-To: <1212967717-5165-1-git-send-email-LeWiemann@gmail.com> ``` Previously, cat-file --batch / --batch-check would silently exit if it was passed a non-existent SHA1 on stdin. Now it prints " missing" as in all other cases (and as advertised in the documentation). Note that cat-file --batch-check (but not --batch) will still output "error: unable to find " on stderr if a non-existent SHA1 is passed, but this does not affect parsing its stdout. Also, type <= 0 was previously using the potentially uninitialized type variable (relying on it being 0); it is now being initialized. Signed-off-by: Lea Wiemann --- Changed since v1: initialize type variable, and add a test case that has a good chance of catching this. builtin-cat-file.c | 9 ++++++--- t/t1006-cat-file.sh | 24 +++++++++++++++++++++--- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/builtin-cat-file.c b/builtin-cat-file.c index f8b3160..bd343ef 100644 --- a/builtin-cat-file.c +++ b/builtin-cat-file.c @@ -150,7 +150,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name) static int batch_one_object(const char *obj_name, int print_contents) { unsigned char sha1[20]; - enum object_type type; + enum object_type type = 0; unsigned long size; void *contents = contents; @@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents) else type = sha1_object_info(sha1, &size); - if (type <= 0) - return 1; + if (type <= 0) { + printf("%s missing\n", obj_name); + fflush(stdout); + return 0; + } printf("%s %s %lu\n", sha1_to_hex(sha1), typename(type), size); fflush(stdout); diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 973aef7..542b4ae 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -174,9 +174,27 @@ do ' done -test_expect_success "--batch-check for a non-existent object" ' - test "deadbeef missing" = \ - "$(echo_without_newline deadbeef | git cat-file --batch-check)" +test_expect_success "--batch-check for a non-existent named object" ' + test "foobar42 missing +foobar84 missing" = \ + "$(( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)" +' + +test_expect_success "--batch-check for a non-existent hash" ' + test "0000000000000000000000000000000000000042 missing +0000000000000000000000000000000000000084 missing" = \ + "$(( echo 0000000000000000000000000000000000000042; + echo_without_newline 0000000000000000000000000000000000000084; ) \ + | git cat-file --batch-check)" +' + +test_expect_success "--batch for an existent and a non-existent hash" ' + test "$tag_sha1 tag $tag_size +$tag_content +0000000000000000000000000000000000000000 missing" = \ + "$(( echo $tag_sha1; + echo_without_newline 0000000000000000000000000000000000000000; ) \ + | git cat-file --batch)" ' test_expect_success "--batch-check for an emtpy line" ' -- 1.5.6.rc1.21.gc5a36.dirty ``` ## Lea Wiemann, 2008-06-09 00:05 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <484C73DA.10804@gmail.com> URL: https://gitlist.dev/e/484C73DA.10804%40gmail.com In-Reply-To: ``` Johannes Schindelin wrote: > Lea Wiemann wrote: >> - if (type <= 0) >> - return 1; >> + if (type <= 0) { >> + printf("%s missing\n", obj_name); >> + fflush(stdout); >> + return 0; >> + } > > Is it really intended that it returns 0 now? Yes, return 0 causes it to not exit (which is the desired effect, as in documented). > Further, should it not be an error("%s missin... instead? It is an > error, isn't it? No, it's not an error. I suggest you read git-cat-file.1 -- getting a response of " missing" (rather than cat-file terminating) if you pass in a non-existent object is the desired behavior, and it happened before for named objects, like "HEAD", but not for full hashes. Now it also happens for full hashes. -- Lea ``` ## Johannes Schindelin, 2008-06-09 01:02 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0806090201090.1783%40racer In-Reply-To: <484C73DA.10804@gmail.com> ``` Hi, On Mon, 9 Jun 2008, Lea Wiemann wrote: > Johannes Schindelin wrote: > > Lea Wiemann wrote: > > > - if (type <= 0) > > > - return 1; > > > + if (type <= 0) { > > > + printf("%s missing\n", obj_name); > > > + fflush(stdout); > > > + return 0; > > > + } > > > > Is it really intended that it returns 0 now? > > Yes, return 0 causes it to not exit (which is the desired effect, as in > documented). That is unexpected. IMO this is a bug. rev-parse I can understand, but cat-file not failing when it clearly did not find the object? Ciao, Dscho ``` ## Lea Wiemann, 2008-06-09 10:17 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <484D033A.3020006@gmail.com> URL: https://gitlist.dev/e/484D033A.3020006%40gmail.com In-Reply-To: ``` Johannes Schindelin wrote: > That is unexpected. IMO this is a bug. rev-parse I can understand, but > cat-file not failing when it clearly did not find the object? From a recent IRC conversation (with permission): Dscho thinks that git-cat-file --batch should print an error and exit if passed an invalid revision (as opposed to the current behavior of printing " missing" and continuing), since anything else would be unexpected. [1] He says that an --ignore-missing option should be introduced instead, and cat-file --batch should exit on non-existent objects unless the --ignore-missing option is given. I maintain that my patch isn't introducing a new feature but fixing a bug -- cat-file --batch already doesn't exit but (as documented) prints " missing" if passed a non-existent object. The only exception is a full (= 40-character) SHA1 that isn't augmented (e.g. by "^" etc.). This exception is neither useful, expected, nor documented. Hence, my patch fixes this inconsistency. If Dscho (or anybody else) wants to introduce an --ignore-missing option, feel free to submit it separately, but please don't object to my patch because of it -- my patch is merely fixing the existing code. (Note that such an option is unlikely to make it into the code though, since it would have to change the existing behavior of cat-file --batch, which other programs rely on.) -- Lea [1] He says he hasn't read the manual so far though because he doesn't have time for it -- if he had read it (as I repeatedly recommended) he probably wouldn't find the behavior surprising. ``` ## Jakub Narebski, 2008-06-09 11:07 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: URL: https://gitlist.dev/e/m3lk1ex3s6.fsf%40localhost.localdomain In-Reply-To: <484D033A.3020006@gmail.com> ``` Lea Wiemann writes: > From a recent IRC conversation (with permission): > > Dscho thinks that git-cat-file --batch should print an error and exit > if passed an invalid revision (as opposed to the current behavior of > printing " missing" and continuing), since anything else would > be unexpected. [1] He says that an --ignore-missing option should be > introduced instead, and cat-file --batch should exit on non-existent > objects unless the --ignore-missing option is given. [...] > If Dscho (or anybody else) wants to introduce an --ignore-missing > option, feel free to submit it separately, but please don't object to > my patch because of it -- my patch is merely fixing the existing > code. (Note that such an option is unlikely to make it into the code > though, since it would have to change the existing behavior of > cat-file --batch, which other programs rely on.) I think the (usual) solution is to add --ignore-missing and --no-ignore-missing (or --noignore-missing), add configuration option 'catfile.ignoreMissing', make ignore-missing default and deprecate it with some transition time... -- Jakub Narebski Poland ShadeHawk on #git ``` ## Lea Wiemann, 2008-06-09 13:27 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <484D2FBA.1010606@gmail.com> URL: https://gitlist.dev/e/484D2FBA.1010606%40gmail.com In-Reply-To: ``` Jakub Narebski wrote: > I think the (usual) solution is to add --ignore-missing and > --no-ignore-missing (or --noignore-missing), add configuration > option 'catfile.ignoreMissing', make ignore-missing default and > deprecate it with some transition time... *shrugs* I'm not convinced that anyone would ever *not* want to use --ignore-missing, since programs that use --batch(-check) presumably query for a large batch of objects and therefore define their own error handling anyway. Also, it's not like you could accidentally ignore the " missing" output in your code since you're parsing the output anyway. So I'm not sure what the point of changing the current behavior is. (Perhaps add a --exit-on-missing option? Not that I or anyone else has ever asked for that feature.) In any case, can please someone review my patch? I think my patch should be independent of these things (since it's a bug fix), and I'd hate to see it not make it into the code just because some people don't seem to like cat-file's behavior. ;-) (I need it for the Perl API, after all.) -- Lea ``` ## Johannes Sixt, 2008-06-09 14:23 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <484D3CCB.1000508@viscovery.net> URL: https://gitlist.dev/e/484D3CCB.1000508%40viscovery.net In-Reply-To: <1212967717-5165-1-git-send-email-LeWiemann@gmail.com> ``` Lea Wiemann schrieb: > Previously, cat-file --batch / --batch-check would silently exit if it > was passed a non-existent SHA1 on stdin. Now it prints " > missing" as in all other cases (and as advertised in the > documentation). Makes sense! > diff --git a/builtin-cat-file.c b/builtin-cat-file.c > index f8b3160..112b9db 100644 > --- a/builtin-cat-file.c > +++ b/builtin-cat-file.c > @@ -168,8 +168,11 @@ static int batch_one_object(const char *obj_name, int print_contents) > else > type = sha1_object_info(sha1, &size); > > - if (type <= 0) > - return 1; > + if (type <= 0) { > + printf("%s missing\n", obj_name); > + fflush(stdout); > + return 0; > + } With this change an invalid name and a non-existing SHA1 are treated as "missing". But an empty name is still an error, which we don't see here because it's outside the hunk's context: It is the first thing that the function checks, and there it exits with return 1. Why is this case still special? -- Hannes ``` ## Lea Wiemann, 2008-06-09 18:21 Subject: Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <484D7495.3020407@gmail.com> URL: https://gitlist.dev/e/484D7495.3020407%40gmail.com In-Reply-To: <484D3CCB.1000508@viscovery.net> ``` Johannes Sixt wrote: > With this change an invalid name and a non-existing SHA1 are treated as > "missing". But an empty name is still an error [...]: It is the first thing > that the function checks, and there it exits with return 1. An empty object name is actually not an error but just yields "SP missing". The "if (!obj_name) return 1;" statement doesn't dereference obj_name, so it checks for null pointers. However, batch_one_object gets called with buf.buf as its obj_name parameter, which according to api-strbuf.txt is never NULL. So probably the check is pointless and can be removed (the tests still pass); someone more knowledgeable than me should confirm this though. (FWIW, perhaps the original author intended to catch empty input this way, but I'm not convinced that this is even sensible.) Unrelatedly, does anyone know how to stop sha1_object_info from barfing on non-existent hashes? It causes the annoying "error: unable to find 000..." on stderr if you run "echo 0000000000000000000000000000000000000000 | git-cat-file --batch-check". -- Lea ``` ## Junio C Hamano, 2008-06-09 20:37 Subject: Re: [PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <7v8wxegxae.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8wxegxae.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1212969741-24899-1-git-send-email-LeWiemann@gmail.com> ``` Lea Wiemann writes: > Previously, cat-file --batch / --batch-check would silently exit if it > was passed a non-existent SHA1 on stdin. Now it prints " > missing" as in all other cases (and as advertised in the > documentation). I think this change makes sense, given that --batch is about having an object server a read-only process can drive over bi-di pipe. Thanks. ``` ## Junio C Hamano, 2008-06-09 20:42 Subject: Re: [PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing Message-ID: <7vwskyfige.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwskyfige.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1212969741-24899-1-git-send-email-LeWiemann@gmail.com> ``` Lea Wiemann writes: > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh > index 973aef7..542b4ae 100755 > --- a/t/t1006-cat-file.sh > +++ b/t/t1006-cat-file.sh > @@ -174,9 +174,27 @@ do > ... > +foobar84 missing" = \ > + "$(( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)" I have a slight suspicion that some of the Bourne shell implementations we support may misinterpret $(( as beginning of arithmetic expansion, so perhaps we would want an extra SP between the open parens. Other than that, looked ok. ```