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

From: Jeff King <peff@peff.net>
Date: Jun 2, 2021, 20:00 UTC
Message-ID: <YLfjexczp1/HILWj@coredump.intra.peff.net>
In-Reply-To: <CAOLTT8SCeKy74cVO3K5zJ5n=0s=o9zk2ipV5wM6CHQPzRoMi5Q@mail.gmail.com>

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

Show 41 quoted lines

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