[BUG] "echo HEAD | git cat-file --batch=''" fails catastrophically

[BUG] "echo HEAD | git cat-file --batch=''" fails catastrophically

[Samuel Bronson]

Observe:

% echo HEAD | git cat-file --batch=

fatal: object fde075cb72fc0773d8e8ca93d55a35d77bb6688b changed type!?

Without the =, it works fine; with a string that has both
"%(objecttype)" and "%(objectsize)", it's fine; but when you don't
include both, it complains about one of the values that you did not
mention having changed.

jrnieder fingered v1.8.4-rc0~7^2~15 as the (likely?) culprit here.

Re: [BUG] "echo HEAD | git cat-file --batch=''" fails catastrophically

[Jeff King]

On Tue, Dec 10, 2013 at 11:37:14PM -0500, Samuel Bronson wrote:

> % echo HEAD | git cat-file --batch=
> 
> fatal: object fde075cb72fc0773d8e8ca93d55a35d77bb6688b changed type!?
> 
> Without the =, it works fine; with a string that has both
> "%(objecttype)" and "%(objectsize)", it's fine; but when you don't
> include both, it complains about one of the values that you did not
> mention having changed.
> 
> jrnieder fingered v1.8.4-rc0~7^2~15 as the (likely?) culprit here.

It's not actually that commit itself, but rather that commit in
conjunction with further optimizations in that patch series.

The rest of the series tries hard to avoid looking up items that we
aren't going to print, for --batch-check. But I didn't think about the
fact that "--batch" got the same custom-header feature, but was relying
on the values from the default header to do its consistency checks.

The following patches should fix it.

  [1/2]: cat-file: pass expand_data to print_object_or_die
  [2/2]: cat-file: handle --batch format with missing type/size

Doing "--batch=" is somewhat pointless. If you do not get the size, you
cannot know when the object content ends, so it only makes sense with a
single object. At which point using --batch is pointless. Doing
"--batch=%(objectsize)" is reasonable, though, and that is broken, too.

v1.8.4 has the breakage, though it's not a regression (doing
"--batch=anything" did not exist before that). This can probably just go
to the regular "maint" track for v1.8.5).

-Peff

[PATCH 1/2] cat-file: pass expand_data to print_object_or_die

[Jeff King]

We currently individually pass the sha1, type, and size
fields calculated by sha1_object_info. However, if we pass
the whole struct, the called function can make more
intelligent decisions about which fields were actualled
filled by sha1_object_info.

As a side effect, we can rename the local variables in the
function to "type" and "size", since the names are no longer
taken.

There should be no functional change to this patch.

Signed-off-by: Jeff King <peff@peff.net>
---
I split this out mostly to keep the noise out of the follow-on diff.

 builtin/cat-file.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b2ca775..1434afb 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,25 +193,26 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 	return end - start + 1;
 }
 
-static void print_object_or_die(int fd, const unsigned char *sha1,
-				enum object_type type, unsigned long size)
+static void print_object_or_die(int fd, struct expand_data *data)
 {
-	if (type == OBJ_BLOB) {
+	const unsigned char *sha1 = data->sha1;
+
+	if (data->type == OBJ_BLOB) {
 		if (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)
 			die("unable to stream %s to stdout", sha1_to_hex(sha1));
 	}
 	else {
-		enum object_type rtype;
-		unsigned long rsize;
+		enum object_type type;
+		unsigned long size;
 		void *contents;
 
-		contents = read_sha1_file(sha1, &rtype, &rsize);
+		contents = read_sha1_file(sha1, &type, &size);
 		if (!contents)
 			die("object %s disappeared", sha1_to_hex(sha1));
-		if (rtype != type)
+		if (type != data->type)
 			die("object %s changed type!?", sha1_to_hex(sha1));
-		if (rsize != size)
-			die("object %s change size!?", sha1_to_hex(sha1));
+		if (size != data->size)
+			die("object %s changed size!?", sha1_to_hex(sha1));
 
 		write_or_die(fd, contents, size);
 		free(contents);
@@ -250,7 +251,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,
 	strbuf_release(&buf);
 
 	if (opt->print_contents) {
-		print_object_or_die(1, data->sha1, data->type, data->size);
+		print_object_or_die(1, data);
 		write_or_die(1, "\n", 1);
 	}
 	return 0;
-- 
1.8.5.524.g6743da6

[PATCH 2/2] cat-file: handle --batch format with missing type/size

[Jeff King]

Commit 98e2092 taught cat-file to stream blobs with --batch,
which requires that we look up the object type before
loading it into memory.  As a result, we now print the
object header from information in sha1_object_info, and the
actual contents from the read_sha1_file. We double-check
that the information we printed in the header matches the
content we are about to show.

Later, commit 93d2a60 allowed custom header lines for
--batch, and commit 5b08640 made type lookups optional. As a
result, specifying a header line without the type or size
means that we will not look up those items at all.

This causes our double-checking to erroneously die with an
error; we think the type or size has changed, when in fact
it was simply left at "0".

For the size, we can fix this by only doing the consistency
double-check when we have retrieved the size via
sha1_object_info. In the case that we have not retrieved the
value, that means we also did not print it, so there is
nothing for us to check that we are consistent with.

We could do the same for the type. However, besides our
consistency check, we also care about the type in deciding
whether to stream or not. We therefore make sure to always
trigger a type lookup when we are printing, so that even a
format without the type will stream as we would in the
normal case.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c  |  9 ++++++++-
 t/t1006-cat-file.sh | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1434afb..4af67fd 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)
 			die("object %s disappeared", sha1_to_hex(sha1));
 		if (type != data->type)
 			die("object %s changed type!?", sha1_to_hex(sha1));
-		if (size != data->size)
+		if (data->info.sizep && size != data->size)
 			die("object %s changed size!?", sha1_to_hex(sha1));
 
 		write_or_die(fd, contents, size);
@@ -276,6 +276,13 @@ static int batch_objects(struct batch_options *opt)
 	data.mark_query = 0;
 
 	/*
+	 * If we are printing out the object, then always fill in the type,
+	 * since we will want to decide whether or not to stream.
+	 */
+	if (opt->print_contents)
+		data.info.typep = &data.type;
+
+	/*
 	 * We are going to call get_sha1 on a potentially very large number of
 	 * objects. In most large cases, these will be actual object sha1s. The
 	 * cost to double-check that each one is not also a ref (just so we can
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 8a1bc5c..1687098 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -85,6 +85,28 @@ $content"
 		git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
 	test_cmp expect actual
     '
+
+    test -z "$content" ||
+    test_expect_success "--batch without type ($type)" '
+	{
+		echo "$size" &&
+		maybe_remove_timestamp "$content" $no_ts
+	} >expect &&
+	echo $sha1 | git cat-file --batch="%(objectsize)" >actual.full &&
+	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
+	test_cmp expect actual
+    '
+
+    test -z "$content" ||
+    test_expect_success "--batch without size ($type)" '
+	{
+		echo "$type" &&
+		maybe_remove_timestamp "$content" $no_ts
+	} >expect &&
+	echo $sha1 | git cat-file --batch="%(objecttype)" >actual.full &&
+	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
+	test_cmp expect actual
+    '
 }
 
 hello_content="Hello World"
-- 
1.8.5.524.g6743da6

Re: [PATCH 1/2] cat-file: pass expand_data to print_object_or_die

[Jonathan Nieder]

Hi,

Jeff King wrote:

>                                        However, if we pass
> the whole struct, the called function can make more
> intelligent decisions about which fields were actualled
> filled by sha1_object_info.

Thanks.

s/actualled/actually/, I think.

At first I thought this patch was going to be about making those
intelligent decisions.  Maybe s/the called function can/a future patch
can teach the called function/ or something?

[...]
> There should be no functional change to this patch.

The patch itself looks straightforward, yep. :)

With the typofix mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size

[Jonathan Nieder]

Jeff King wrote:

> We could do the same for the type. However, besides our
> consistency check, we also care about the type in deciding
> whether to stream or not. We therefore make sure to always
> trigger a type lookup when we are printing, so that

This "We make sure" is the behavior after this patch, not before,
right?

[...]
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)
>  			die("object %s disappeared", sha1_to_hex(sha1));
>  		if (type != data->type)
>  			die("object %s changed type!?", sha1_to_hex(sha1));

Maybe an assert(data.info.typep) or similar would make this more
locally readable.

[...]
> @@ -276,6 +276,13 @@ static int batch_objects(struct batch_options *opt)
>  	data.mark_query = 0;
>  
> +	/*
> +	 * If we are printing out the object, then always fill in the type,
> +	 * since we will want to decide whether or not to stream.
> +	 */
> +	if (opt->print_contents)
> +		data.info.typep = &data.type;

Oof.  I guess this means that the optimization from 98e2092b wasn't being
applied by 'git cat-file --batch' with format specifiers that don't
include %(objecttype), but no one would have noticed because of the
"changed type" thing. :)

> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -85,6 +85,28 @@ $content"
>  		git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
>  	test_cmp expect actual
>      '
> +
> +    test -z "$content" ||
> +    test_expect_success "--batch without type ($type)" '
> +	{
> +		echo "$size" &&
> +		maybe_remove_timestamp "$content" $no_ts
> +	} >expect &&
> +	echo $sha1 | git cat-file --batch="%(objectsize)" >actual.full &&
> +	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
> +	test_cmp expect actual
> +    '
> +
> +    test -z "$content" ||
> +    test_expect_success "--batch without size ($type)" '
> +	{
> +		echo "$type" &&
> +		maybe_remove_timestamp "$content" $no_ts
> +	} >expect &&
> +	echo $sha1 | git cat-file --batch="%(objecttype)" >actual.full &&
> +	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
> +	test_cmp expect actual
> +    '
>  }

Looks good.

(not about this patch) I suspect a test_cmp_ignore_timestamp helper
could simplify these tests somewhat. :)

For what it's worth, with or without commit message changes or the
check that data->type is initialized,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 1/2] cat-file: pass expand_data to print_object_or_die

[Jeff King]

On Wed, Dec 11, 2013 at 12:11:12PM -0800, Jonathan Nieder wrote:

> >                                        However, if we pass
> > the whole struct, the called function can make more
> > intelligent decisions about which fields were actualled
> > filled by sha1_object_info.
> 
> Thanks.
> 
> s/actualled/actually/, I think.

Yes. Not sure how I managed that typo.

> At first I thought this patch was going to be about making those
> intelligent decisions.  Maybe s/the called function can/a future patch
> can teach the called function/ or something?

I clarified it in the commit message below.

> > There should be no functional change to this patch.
> 
> The patch itself looks straightforward, yep. :)

It technically does typo-fix the error message, which I guess is a
functional change. But I didn't count that. :)

Here it is with the commit message fixes and your reviewed-by.

-- >8 --
Subject: cat-file: pass expand_data to print_object_or_die

We currently individually pass the sha1, type, and size
fields calculated by sha1_object_info. However, if we pass
the whole struct, the called function can make more
intelligent decisions about which fields were actually
filled by sha1_object_info.

This patch takes that first refactoring step, passing the
whole struct, so further patches can make those decisions
with less noise in their diffs. There should be no
functional change to this patch (aside from a minor typo fix
in the error message).

As a side effect, we can rename the local variables in the
function to "type" and "size", since the names are no longer
taken.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b2ca775..1434afb 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,25 +193,26 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 	return end - start + 1;
 }
 
-static void print_object_or_die(int fd, const unsigned char *sha1,
-				enum object_type type, unsigned long size)
+static void print_object_or_die(int fd, struct expand_data *data)
 {
-	if (type == OBJ_BLOB) {
+	const unsigned char *sha1 = data->sha1;
+
+	if (data->type == OBJ_BLOB) {
 		if (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)
 			die("unable to stream %s to stdout", sha1_to_hex(sha1));
 	}
 	else {
-		enum object_type rtype;
-		unsigned long rsize;
+		enum object_type type;
+		unsigned long size;
 		void *contents;
 
-		contents = read_sha1_file(sha1, &rtype, &rsize);
+		contents = read_sha1_file(sha1, &type, &size);
 		if (!contents)
 			die("object %s disappeared", sha1_to_hex(sha1));
-		if (rtype != type)
+		if (type != data->type)
 			die("object %s changed type!?", sha1_to_hex(sha1));
-		if (rsize != size)
-			die("object %s change size!?", sha1_to_hex(sha1));
+		if (size != data->size)
+			die("object %s changed size!?", sha1_to_hex(sha1));
 
 		write_or_die(fd, contents, size);
 		free(contents);
@@ -250,7 +251,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,
 	strbuf_release(&buf);
 
 	if (opt->print_contents) {
-		print_object_or_die(1, data->sha1, data->type, data->size);
+		print_object_or_die(1, data);
 		write_or_die(1, "\n", 1);
 	}
 	return 0;
-- 
1.8.5.524.g6743da6

Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size

[Jeff King]

On Wed, Dec 11, 2013 at 12:42:00PM -0800, Jonathan Nieder wrote:

> > We could do the same for the type. However, besides our
> > consistency check, we also care about the type in deciding
> > whether to stream or not. We therefore make sure to always
> > trigger a type lookup when we are printing, so that
> 
> This "We make sure" is the behavior after this patch, not before,
> right?

Correct. I'll clarify that.

> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)
> >  			die("object %s disappeared", sha1_to_hex(sha1));
> >  		if (type != data->type)
> >  			die("object %s changed type!?", sha1_to_hex(sha1));
> 
> Maybe an assert(data.info.typep) or similar would make this more
> locally readable.

I'm not sure it makes it more readable, but it would protect against
violating our assumptions later (and ease people's minds who wonder why
we can touch data->type without a similar check).

> > +	/*
> > +	 * If we are printing out the object, then always fill in the type,
> > +	 * since we will want to decide whether or not to stream.
> > +	 */
> > +	if (opt->print_contents)
> > +		data.info.typep = &data.type;
> 
> Oof.  I guess this means that the optimization from 98e2092b wasn't being
> applied by 'git cat-file --batch' with format specifiers that don't
> include %(objecttype), but no one would have noticed because of the
> "changed type" thing. :)

Yes. The loss of the optimization was a small thing compared to being
totally broken. :)

> > +	{
> > +		echo "$type" &&
> > +		maybe_remove_timestamp "$content" $no_ts
> > +	} >expect &&
> > +	echo $sha1 | git cat-file --batch="%(objecttype)" >actual.full &&
> > +	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
> > +	test_cmp expect actual
> [...]
> (not about this patch) I suspect a test_cmp_ignore_timestamp helper
> could simplify these tests somewhat. :)

Yeah, the maybe_remove_timestamp is ugly on so many levels. I used it
because it's deeply embedded in the existing tests, and I didn't want to
tackle refactoring the whole thing. Be my guest if you want to do it on
top. :)

> For what it's worth, with or without commit message changes or the
> check that data->type is initialized,
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks. Updated patch is below.

-- >8 --
Subject: cat-file: handle --batch format with missing type/size

Commit 98e2092 taught cat-file to stream blobs with --batch,
which requires that we look up the object type before
loading it into memory.  As a result, we now print the
object header from information in sha1_object_info, and the
actual contents from the read_sha1_file. We double-check
that the information we printed in the header matches the
content we are about to show.

Later, commit 93d2a60 allowed custom header lines for
--batch, and commit 5b08640 made type lookups optional. As a
result, specifying a header line without the type or size
means that we will not look up those items at all.

This causes our double-checking to erroneously die with an
error; we think the type or size has changed, when in fact
it was simply left at "0".

For the size, we can fix this by only doing the consistency
double-check when we have retrieved the size via
sha1_object_info. In the case that we have not retrieved the
value, that means we also did not print it, so there is
nothing for us to check that we are consistent with.

We could do the same for the type. However, besides our
consistency check, we also care about the type in deciding
whether to stream or not. So instead of handling the case
where we do not know the type, this patch instead makes sure
that we always trigger a type lookup when we are printing,
so that even a format without the type will stream as we
would in the normal case.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c  | 11 ++++++++++-
 t/t1006-cat-file.sh | 22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1434afb..f8288c8 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -197,6 +197,8 @@ static void print_object_or_die(int fd, struct expand_data *data)
 {
 	const unsigned char *sha1 = data->sha1;
 
+	assert(data->info.typep);
+
 	if (data->type == OBJ_BLOB) {
 		if (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)
 			die("unable to stream %s to stdout", sha1_to_hex(sha1));
@@ -211,7 +213,7 @@ static void print_object_or_die(int fd, struct expand_data *data)
 			die("object %s disappeared", sha1_to_hex(sha1));
 		if (type != data->type)
 			die("object %s changed type!?", sha1_to_hex(sha1));
-		if (size != data->size)
+		if (data->info.sizep && size != data->size)
 			die("object %s changed size!?", sha1_to_hex(sha1));
 
 		write_or_die(fd, contents, size);
@@ -276,6 +278,13 @@ static int batch_objects(struct batch_options *opt)
 	data.mark_query = 0;
 
 	/*
+	 * If we are printing out the object, then always fill in the type,
+	 * since we will want to decide whether or not to stream.
+	 */
+	if (opt->print_contents)
+		data.info.typep = &data.type;
+
+	/*
 	 * We are going to call get_sha1 on a potentially very large number of
 	 * objects. In most large cases, these will be actual object sha1s. The
 	 * cost to double-check that each one is not also a ref (just so we can
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 8a1bc5c..1687098 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -85,6 +85,28 @@ $content"
 		git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
 	test_cmp expect actual
     '
+
+    test -z "$content" ||
+    test_expect_success "--batch without type ($type)" '
+	{
+		echo "$size" &&
+		maybe_remove_timestamp "$content" $no_ts
+	} >expect &&
+	echo $sha1 | git cat-file --batch="%(objectsize)" >actual.full &&
+	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
+	test_cmp expect actual
+    '
+
+    test -z "$content" ||
+    test_expect_success "--batch without size ($type)" '
+	{
+		echo "$type" &&
+		maybe_remove_timestamp "$content" $no_ts
+	} >expect &&
+	echo $sha1 | git cat-file --batch="%(objecttype)" >actual.full &&
+	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
+	test_cmp expect actual
+    '
 }
 
 hello_content="Hello World"
-- 
1.8.5.524.g6743da6

Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size

[Jonathan Nieder]

Jeff King wrote:

> Updated patch is below.

Thanks!  v2 of both patches looks good.

Re: [PATCH 1/2] cat-file: pass expand_data to print_object_or_die

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It technically does typo-fix the error message, which I guess is a
> functional change. But I didn't count that. :)
>
> Here it is with the commit message fixes and your reviewed-by.

Thanks, both.

Will queue, to eventually merge to 'maint'.

>
> -- >8 --
> Subject: cat-file: pass expand_data to print_object_or_die
>
> We currently individually pass the sha1, type, and size
> fields calculated by sha1_object_info. However, if we pass
> the whole struct, the called function can make more
> intelligent decisions about which fields were actually
> filled by sha1_object_info.
>
> This patch takes that first refactoring step, passing the
> whole struct, so further patches can make those decisions
> with less noise in their diffs. There should be no
> functional change to this patch (aside from a minor typo fix
> in the error message).
>
> As a side effect, we can rename the local variables in the
> function to "type" and "size", since the names are no longer
> taken.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/cat-file.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index b2ca775..1434afb 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -193,25 +193,26 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
>  	return end - start + 1;
>  }
>  
> -static void print_object_or_die(int fd, const unsigned char *sha1,
> -				enum object_type type, unsigned long size)
> +static void print_object_or_die(int fd, struct expand_data *data)
>  {
> -	if (type == OBJ_BLOB) {
> +	const unsigned char *sha1 = data->sha1;
> +
> +	if (data->type == OBJ_BLOB) {
>  		if (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)
>  			die("unable to stream %s to stdout", sha1_to_hex(sha1));
>  	}
>  	else {
> -		enum object_type rtype;
> -		unsigned long rsize;
> +		enum object_type type;
> +		unsigned long size;
>  		void *contents;
>  
> -		contents = read_sha1_file(sha1, &rtype, &rsize);
> +		contents = read_sha1_file(sha1, &type, &size);
>  		if (!contents)
>  			die("object %s disappeared", sha1_to_hex(sha1));
> -		if (rtype != type)
> +		if (type != data->type)
>  			die("object %s changed type!?", sha1_to_hex(sha1));
> -		if (rsize != size)
> -			die("object %s change size!?", sha1_to_hex(sha1));
> +		if (size != data->size)
> +			die("object %s changed size!?", sha1_to_hex(sha1));
>  
>  		write_or_die(fd, contents, size);
>  		free(contents);
> @@ -250,7 +251,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,
>  	strbuf_release(&buf);
>  
>  	if (opt->print_contents) {
> -		print_object_or_die(1, data->sha1, data->type, data->size);
> +		print_object_or_die(1, data);
>  		write_or_die(1, "\n", 1);
>  	}
>  	return 0;

Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes. The loss of the optimization was a small thing compared to being
> totally broken. :)
> ...
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks. Updated patch is below.

;-)

I like it when I see patches are polished between the submitter and
reviewer(s) fully, before the maintainer has a chance to pick an
intermediate version (only to later replace and requeue).

Thanks, both.