All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: ZheNing Hu <adlternative@gmail.com>
Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Christian Couder <christian.couder@gmail.com>,
	Hariom Verma <hariom18599@gmail.com>,
	Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug
Date: Wed, 2 Jun 2021 16:00:59 -0400	[thread overview]
Message-ID: <YLfjexczp1/HILWj@coredump.intra.peff.net> (raw)
In-Reply-To: <CAOLTT8SCeKy74cVO3K5zJ5n=0s=o9zk2ipV5wM6CHQPzRoMi5Q@mail.gmail.com>

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

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

  reply	other threads:[~2021-06-02 20:01 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-01 14:35 [PATCH 0/2] " ZheNing Hu via GitGitGadget
2021-06-01 14:35 ` [PATCH 1/2] " ZheNing Hu via GitGitGadget
2021-06-01 15:52   ` Jeff King
2021-06-02 13:15     ` ZheNing Hu
2021-06-02 20:00       ` Jeff King [this message]
2021-06-03  6:28         ` ZheNing Hu
2021-06-03  7:18         ` ZheNing Hu
2021-06-03 19:28           ` Jeff King
2021-06-01 14:35 ` [PATCH 2/2] [GSOC] cat-file: merge two block into one ZheNing Hu via GitGitGadget
2021-06-01 15:55   ` Jeff King
2021-06-02 13:27     ` ZheNing Hu
2021-06-03 16:29 ` [PATCH v2 0/2] [GSOC] cat-file: fix --batch report changed-type bug ZheNing Hu via GitGitGadget
2021-06-03 16:29   ` [PATCH v2 1/2] [GSOC] cat-file: handle trivial --batch format with --batch-all-objects ZheNing Hu via GitGitGadget
2021-06-03 16:29   ` [PATCH v2 2/2] [GSOC] cat-file: merge two block into one ZheNing Hu via GitGitGadget
2021-06-03 19:29   ` [PATCH v2 0/2] [GSOC] cat-file: fix --batch report changed-type bug Jeff King
2021-06-03 22:51   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YLfjexczp1/HILWj@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=adlternative@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=hariom18599@gmail.com \
    --subject='Re: [PATCH 1/2] [GSOC] cat-file: fix --batch report changed-type bug' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.