From: Jeff King <firstname.lastname@example.org> To: ZheNing Hu <email@example.com> Cc: ZheNing Hu via GitGitGadget <firstname.lastname@example.org>, Git List <email@example.com>, Junio C Hamano <firstname.lastname@example.org>, Christian Couder <email@example.com>, Hariom Verma <firstname.lastname@example.org>, Felipe Contreras <email@example.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
next prev parent 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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.