Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug

On Wed, Jun 02, 2021 at 09:15:45PM +0800, ZheNing Hu wrote:

quoted

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.

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.

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