[PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Lea Wiemann]

Previously, cat-file --batch / --batch-check would silently exit if it
was passed a non-existent SHA1 on stdin.  Now it prints "<SHA1>
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 <SHA1>" on stderr if a non-existent SHA1 is
passed, but this does not affect parsing its stdout.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
 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

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Johannes Schindelin]

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

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Lea Wiemann]

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 "<object> 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

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Johannes Schindelin]

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

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Lea Wiemann]

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 "<object> 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 
"<object> 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.

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Jakub Narebski]

Lea Wiemann <lewiemann@gmail.com> 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 "<object> 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

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Lea Wiemann]

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 
"<object> 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

[PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing

[Lea Wiemann]

Previously, cat-file --batch / --batch-check would silently exit if it
was passed a non-existent SHA1 on stdin.  Now it prints "<SHA1>
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 <SHA1>" 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 <LeWiemann@gmail.com>
---
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

Re: [PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing

[Junio C Hamano]

Lea Wiemann <lewiemann@gmail.com> writes:

> Previously, cat-file --batch / --batch-check would silently exit if it
> was passed a non-existent SHA1 on stdin.  Now it prints "<SHA1>
> 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.

Re: [PATCH v2] cat-file --batch / --batch-check: do not exit if hashes are missing

[Junio C Hamano]

Lea Wiemann <lewiemann@gmail.com> 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.

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Johannes Sixt]

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 "<SHA1>
> 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

Re: [PATCH] cat-file --batch / --batch-check: do not exit if hashes are missing

[Lea Wiemann]

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