threads / patch / 13869

patchcat-file --batch / --batch-check: do not exit if hashes are missing

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

## tl;dr

12 messages between Jun 8, 2008 and Jun 9, 2008. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Lea Wiemann· Jun 8, 2008, 23:28 UTC · lore

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(-)

Show changes to 2 files +17 −5

builtin-cat-file.c, t/t1006-cat-file.sh

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· Jun 8, 2008, 23:34 UTC · re: Lea Wiemann · lore

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

Hi,

maybe s/do not exit/complain/? Because it still exits if the hashes are missing...

Show 15 quoted lines

> 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· Jun 9, 2008, 00:05 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:

Show 10 quoted lines

> 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

Johannes Schindelin· Jun 9, 2008, 01:02 UTC · re: Lea Wiemann · lore

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

Hi,

On Mon, 9 Jun 2008, Lea Wiemann wrote:

Show 14 quoted lines

> 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· Jun 9, 2008, 10:17 UTC · re: Johannes Schindelin · lore

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

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.

Jakub Narebski· Jun 9, 2008, 11:07 UTC · re: Lea Wiemann · lore

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

Lea Wiemann <lewiemann@gmail.com> writes:

Show 8 quoted lines

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

[...]

Show 6 quoted lines

> 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· Jun 9, 2008, 13:27 UTC · re: Jakub Narebski · lore

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

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

Lea Wiemann· Jun 9, 2008, 00:02 UTC · re: Lea Wiemann · lore

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

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(-)

Show changes to 2 files +27 −6

builtin-cat-file.c, t/t1006-cat-file.sh

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

Junio C Hamano· Jun 9, 2008, 20:37 UTC · re: Lea Wiemann · lore

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

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.

Junio C Hamano· Jun 9, 2008, 20:42 UTC · re: Lea Wiemann · lore

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

Lea Wiemann <lewiemann@gmail.com> writes:

Show 8 quoted lines

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

Johannes Sixt· Jun 9, 2008, 14:23 UTC · re: Lea Wiemann · lore

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

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!

Show 15 quoted lines

> 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· Jun 9, 2008, 18:21 UTC · re: Johannes Sixt · lore

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

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