cat-file: fix --batch report changed-type bug

Change from last version:

1. Use Peff's processing method.
2. Add tests for atoms %(objectname), %(rest), and modified the test method.
3. Merge two check block into one.

ZheNing Hu (2):
[GSOC] cat-file: fix --batch report changed-type bug [GSOC] cat-file: merge two block into one

 builtin/cat-file.c  | 10 ++++------
 t/t1006-cat-file.sh | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

base-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-967%2Fadlternative%2Fcat-file-batch-bug-fix-v2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-967/adlternative/cat-file-batch-bug-fix-v2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/967

gitgitgadget

From: ZheNing Hu <[email protected]>

When --batch used with --batch-all-objects, with some format atoms like %(objectname), %(rest) or even no atoms may cause Git exit and report "object xxx changed type!?".

E.g. git cat-file --batch="batman" --batch-all-objects

This is because we did not get the object type through oid_object_info_extended(), it's composed of two situations:

1. Since object_info is empty, skip_object_info is set to true, We skipped collecting the object type.

2. The formatting atom like %(objectname) does not require oid_object_info_extended() to collect object types.

The correct way to deal with it is to swap the order of setting skip_object_info and setting typep. This will ensure that we must get the type of the object when using --batch.

---
 builtin/cat-file.c  | 13 +++++++------
 t/t1006-cat-file.sh | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

index 5ebf13359e83..02461bb5ea6f 100644
@@ -512,12 +512,6 @@ static int batch_objects(struct batch_options *opt)
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
 
-	if (opt->all_objects) {
-		struct object_info empty = OBJECT_INFO_INIT;
-		if (!memcmp(&data.info, &empty, sizeof(empty)))
-			data.skip_object_info = 1;
-	}
-
 	/*
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.

@@ -525,6 +519,13 @@ static int batch_objects(struct batch_options *opt)
 	if (opt->print_contents)
 		data.info.typep = &data.type;
 
+	if (opt->all_objects) {
+		struct object_info empty = OBJECT_INFO_INIT;
+
+		if (!memcmp(&data.info, &empty, sizeof(empty)))
+			data.skip_object_info = 1;
+	}
+
 	if (opt->all_objects) {
 		struct object_cb_data cb;
 

index 5d2dc99b74ad..1502a27142ba 100755
@@ -586,4 +586,23 @@ test_expect_success 'cat-file --unordered works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'cat-file --batch="%(objectname)" with --batch-all-objects will work' '
+	git -C all-two cat-file --batch-all-objects --batch-check="%(objectname)" >objects &&
+	git -C all-two cat-file --batch="%(objectname)" <objects >expect &&
+	git -C all-two cat-file --batch-all-objects --batch="%(objectname)" >actual &&
+	cmp expect actual
+'
+
+test_expect_success 'cat-file --batch="%(rest)" with --batch-all-objects will work' '
+	git -C all-two cat-file --batch="%(rest)" <objects >expect &&
+	git -C all-two cat-file --batch-all-objects --batch="%(rest)" >actual &&
+	cmp expect actual
+'
+
+test_expect_success 'cat-file --batch="batman" with --batch-all-objects will work' '
+	git -C all-two cat-file --batch="batman" <objects >expect &&
+	git -C all-two cat-file --batch-all-objects --batch="batman" >actual &&
+	cmp expect actual
+'
+
 test_done

gitgitgadget

From: ZheNing Hu <[email protected]>

Because the two "if (opt->all_objects)" block
are redundant, merge them into one, to provide
better readability.

---
 builtin/cat-file.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index 02461bb5ea6f..243fe6844bc6 100644
@@ -520,14 +520,11 @@ static int batch_objects(struct batch_options *opt)
 		data.info.typep = &data.type;
 
 	if (opt->all_objects) {
+		struct object_cb_data cb;
 		struct object_info empty = OBJECT_INFO_INIT;
 
 		if (!memcmp(&data.info, &empty, sizeof(empty)))
 			data.skip_object_info = 1;
-	}
-
-	if (opt->all_objects) {
-		struct object_cb_data cb;
 
 		if (has_promisor_remote())
 			warning("This repository uses promisor remotes. Some objects may not be loaded.");

gitgitgadget

Thanks, this explanation and the patch make sense, and I'd be happy if we take it as-is. But in the name of GSoC, let me offer a few polishing tips.

The commit message hints at the root of the problem, but doesn't say it explicitly. Which is: because setting skip_object_info depends on seeing that the object_info is empty, we can't add items to it after setting that flag. And the code path for --batch does that, hence re-ordering them is the solution.

It might also be worth noting that the bug was present from when the skip_object_info code was initially added in 845de33a5b (cat-file: avoid noop calls to sha1_object_info_extended, 2016-05-18).

	git -C all-two cat-file --batch-all-objects --batch-check="%(objectname)" >objects

Funny indentation of the commit message. :)

I think this is worth doing, and I agree the end-result is easier to read. Really minor nit, but I probably wouldn't have said "redundant" here. The conditionals themselves are redundant, but not the blocks.
Maybe: There are two "if (opt->all_objects)" blocks next to each other; merge them into one to provide better readability.

(not a huge deal, as I think seeing the patch helps explain what is going on. But again, just trying to offer polishing advice for future patches)

Thanks, as a GSOC student, your reminders will be very helpful to me.

Um, let's rewrite the commit message, I don't know if this is accurate:

[GSOC] cat-file: fix --batch report changed-type bug

When --batch used with --batch-all-objects, with some format atoms like %(objectname), %(rest) or even no atoms may cause Git exit and report "object xxx changed type!?".

E.g. git cat-file --batch="batman" --batch-all-objects

The bug was present from when the skip_object_info code was initially added in 845de33a5b (cat-file: avoid noop calls to sha1_object_info_extended, 2016-05-18).

This is because we did not get the object type through oid_object_info_extended(), it's composed of two situations:

1. all_objects will be set to true when we use
--batch-all-objects, seeing that object_info
is empty, skip_object_info will be to true,
oid_object_info_extended() will not get the
type of the object.

2. The formatting atom like %(objectname) does
not require oid_object_info_extended() to collect
object types.

print_contents will be set to true when we use
--batch, which can make object_info non-empty,
so the solution is to swap the code order of it
and checking if object_info is empty, which will
ensure that we must get the type of the object
when using --batch.

OK.

I wanted to argue that print_object_or_die() will use data->rest originally. But --batch-all-objects and --textconv and --filter are incompatible. So your idea is reasonable: %(rest) with --batch-all-objects is useless.

Yes, I think this feature can be completed later.(not in this patch)

Yes, it does not use atoms, so it is the most special.

Makes sense.

Thanks.
--
ZheNing Hu

Yes, there is a small space that I didn’t notice. :)

Yeah, your expression is correct.

Thanks.
--
ZheNing Hu

I don't see any inaccuracies there. I do think we could explain it a bit more succinctly. I'll give my attempt, and then you can pick and choose which parts to include between ours. :)

Subject: cat-file: handle trivial --batch format with --batch-all-objects

The --batch code to print an object assumes we found out the type of the object from calling oid_object_info_extended(). This is true for the default format, but even in a custom format, we manually modify the object_info struct to ask for the type.

This assumption was broken by 845de33a5b (cat-file: avoid noop calls to sha1_object_info_extended, 2016-05-18). That commit skips the call to oid_object_info_extended() entirely when --batch-all-objects is in use, and the custom format does not include any placeholders that require calling it.

This results in an error when we try to confirm that the type didn't change:

    $ git cat-file --batch=batman --batch-all-objects
    batman
    fatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?

and also has other subtle effects (e.g., we'd fail to stream a blob, since we don't realize it's a blog in the first place).

We can fix this by flipping the order of the setup. The check for "do we need to get the object info" must come _after_ we've decided whether we need to look up the type.

-Peff

Yes, you wrote it more concisely. I don't need to write out the details of those variables. I just need to tell the reader the introduction of skip_object_info will break the assumption of --batch that we have found the object type.

s/blog/blob/

Thanks.
--
ZheNing Hu

Or when the custom format only include placeholders like %(objectname) or %(rest), oid_object_info_extended() will not get the type of the object.

--
ZheNing Hu

Change from last version:

1. Modified the test structure under the recommendation of Peff.
2. Use clearer and more concise commit message help by Peff.

ZheNing Hu (2):
[GSOC] cat-file: handle trivial --batch format with
--batch-all-objects
[GSOC] cat-file: merge two block into one

 builtin/cat-file.c  | 10 ++++------
 t/t1006-cat-file.sh | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

base-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-967%2Fadlternative%2Fcat-file-batch-bug-fix-v2-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-967/adlternative/cat-file-batch-bug-fix-v2-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/967

Range-diff vs v1:

1: 495cd90dbaf4 ! 1: 4af3b958dd05 [GSOC] cat-file: fix --batch report changed-type bug
@@ Metadata
Author: ZheNing Hu <[email protected]>

## Commit message ##
- [GSOC] cat-file: fix --batch report changed-type bug
+ [GSOC] cat-file: handle trivial --batch format with --batch-all-objects

- When --batch used with --batch-all-objects,
- with some format atoms like %(objectname), %(rest)
- or even no atoms may cause Git exit and report
- "object xxx changed type!?".
+ The --batch code to print an object assumes we found out the type of
+ the object from calling oid_object_info_extended(). This is true for
+ the default format, but even in a custom format, we manually modify
+ the object_info struct to ask for the type.

- E.g. git cat-file --batch="batman" --batch-all-objects
+ This assumption was broken by 845de33a5b (cat-file: avoid noop calls
+ to sha1_object_info_extended, 2016-05-18). That commit skips the call
+ to oid_object_info_extended() entirely when --batch-all-objects is in
+ use, and the custom format does not include any placeholders that
+ require calling it.

- This is because we did not get the object type through
- oid_object_info_extended(), it's composed of two
- situations:
+ Or when the custom format only include placeholders like %(objectname) or
+ %(rest), oid_object_info_extended() will not get the type of the object.

- 1. Since object_info is empty, skip_object_info is
- set to true, We skipped collecting the object type.
+ This results in an error when we try to confirm that the type didn't
+ change:

- 2. The formatting atom like %(objectname) does not require
- oid_object_info_extended() to collect object types.
+ $ git cat-file --batch=batman --batch-all-objects
+ batman
+ fatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?

- The correct way to deal with it is to swap the order
- of setting skip_object_info and setting typep. This
- will ensure that we must get the type of the object
- when using --batch.
+ and also has other subtle effects (e.g., we'd fail to stream a blob,
+ since we don't realize it's a blob in the first place).
+
+ We can fix this by flipping the order of the setup. The check for "do
+ we need to get the object info" must come _after_ we've decided
+ whether we need to look up the type.

@@ t/t1006-cat-file.sh: test_expect_success 'cat-file --unordered works' '
test_cmp expect actual
'

++test_expect_success 'set up object list for --batch-all-objects tests' '
++ git -C all-two cat-file --batch-all-objects --batch-check="%(objectname)" >objects
++'
++
+test_expect_success 'cat-file --batch="%(objectname)" with --batch-all-objects will work' '
-+ git -C all-two cat-file --batch-all-objects --batch-check="%(objectname)" >objects &&
+ git -C all-two cat-file --batch="%(objectname)" <objects >expect &&
+ git -C all-two cat-file --batch-all-objects --batch="%(objectname)" >actual &&
+ cmp expect actual
2: f02c1144d916 ! 2: 759451e784de [GSOC] cat-file: merge two block into one
@@ Metadata
Author: ZheNing Hu <[email protected]>

## Commit message ##
- [GSOC] cat-file: merge two block into one
+ [GSOC] cat-file: merge two block into one

- Because the two "if (opt->all_objects)" block
- are redundant, merge them into one, to provide
- better readability.
+ There are two "if (opt->all_objects)" blocks next
+ to each other, merge them into one to provide better
+ readability.

gitgitgadget

From: ZheNing Hu <[email protected]>

The --batch code to print an object assumes we found out the type of the object from calling oid_object_info_extended(). This is true for the default format, but even in a custom format, we manually modify the object_info struct to ask for the type.

This assumption was broken by 845de33a5b (cat-file: avoid noop calls to sha1_object_info_extended, 2016-05-18). That commit skips the call to oid_object_info_extended() entirely when --batch-all-objects is in use, and the custom format does not include any placeholders that require calling it.

Or when the custom format only include placeholders like %(objectname) or %(rest), oid_object_info_extended() will not get the type of the object.

This results in an error when we try to confirm that the type didn't change:

$ git cat-file --batch=batman --batch-all-objects
batman
fatal: object 000023961a0c02d6e21dc51ea3484ff71abf1c74 changed type!?

and also has other subtle effects (e.g., we'd fail to stream a blob, since we don't realize it's a blob in the first place).

We can fix this by flipping the order of the setup. The check for "do we need to get the object info" must come _after_ we've decided whether we need to look up the type.

---
 builtin/cat-file.c  | 13 +++++++------
 t/t1006-cat-file.sh | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)

index 5ebf13359e83..02461bb5ea6f 100644
@@ -512,12 +512,6 @@ static int batch_objects(struct batch_options *opt)
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
 
-	if (opt->all_objects) {
-		struct object_info empty = OBJECT_INFO_INIT;
-		if (!memcmp(&data.info, &empty, sizeof(empty)))
-			data.skip_object_info = 1;
-	}
-
 	/*
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.

@@ -525,6 +519,13 @@ static int batch_objects(struct batch_options *opt)
 	if (opt->print_contents)
 		data.info.typep = &data.type;
 
+	if (opt->all_objects) {
+		struct object_info empty = OBJECT_INFO_INIT;
+
+		if (!memcmp(&data.info, &empty, sizeof(empty)))
+			data.skip_object_info = 1;
+	}
+
 	if (opt->all_objects) {
 		struct object_cb_data cb;
 

index 5d2dc99b74ad..18b3779ccb60 100755
@@ -586,4 +586,26 @@ test_expect_success 'cat-file --unordered works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'set up object list for --batch-all-objects tests' '
+	git -C all-two cat-file --batch-all-objects --batch-check="%(objectname)" >objects
+'
+
+test_expect_success 'cat-file --batch="%(objectname)" with --batch-all-objects will work' '
+	git -C all-two cat-file --batch="%(objectname)" <objects >expect &&
+	git -C all-two cat-file --batch-all-objects --batch="%(objectname)" >actual &&
+	cmp expect actual
+'
+
+test_expect_success 'cat-file --batch="%(rest)" with --batch-all-objects will work' '
+	git -C all-two cat-file --batch="%(rest)" <objects >expect &&
+	git -C all-two cat-file --batch-all-objects --batch="%(rest)" >actual &&
+	cmp expect actual
+'
+
+test_expect_success 'cat-file --batch="batman" with --batch-all-objects will work' '
+	git -C all-two cat-file --batch="batman" <objects >expect &&
+	git -C all-two cat-file --batch-all-objects --batch="batman" >actual &&
+	cmp expect actual
+'
+
 test_done

gitgitgadget

From: ZheNing Hu <[email protected]>

There are two "if (opt->all_objects)" blocks next to each other, merge them into one to provide better readability.

---
 builtin/cat-file.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index 02461bb5ea6f..243fe6844bc6 100644
@@ -520,14 +520,11 @@ static int batch_objects(struct batch_options *opt)
 		data.info.typep = &data.type;
 
 	if (opt->all_objects) {
+		struct object_cb_data cb;
 		struct object_info empty = OBJECT_INFO_INIT;
 
 		if (!memcmp(&data.info, &empty, sizeof(empty)))
 			data.skip_object_info = 1;
-	}
-
-	if (opt->all_objects) {
-		struct object_cb_data cb;
 
 		if (has_promisor_remote())
 			warning("This repository uses promisor remotes. Some objects may not be loaded.");

gitgitgadget

Yeah, that's what I was trying to get at with "placeholders that require calling it", but I couldn't think of a less awkward way to say that. :) Spelling it out is probably a good idea.

-Peff

Thanks, this version looks good to me.

-Peff

Nicely done. Thanks, both.