git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
ZheNing Hu <adlternative@gmail.com>
Date
Jun 2, 2021, 13:15 UTC
Message-ID
<CAOLTT8SCeKy74cVO3K5zJ5n=0s=o9zk2ipV5wM6CHQPzRoMi5Q@mail.gmail.com>
In-Reply-To
<YLZXyBJ5YgGfmkKv@coredump.intra.peff.net>
Jeff King <peff@peff.net> 于2021年6月1日周二 下午11:52写道：
Show 31 quoted lines
>
> On Tue, Jun 01, 2021 at 02:35:56PM +0000, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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.
>
> 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.
>
Thanks, as a GSOC student, your reminders will be very helpful to me.
Show 6 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.

> 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).
>
OK.
Show 22 quoted lines
> > +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
> > +'
>
> OK, we're checking the output of --batch-all-objects versus taking the
> object list from the input. We can depend on the ordering being
> identical between the two because --batch-all-objects sorts. Good.
>
> > +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
> > +'
>
> This one is rather curious. It definitely shows the bug, but I can't
> imagine why %(rest) would be useful with --batch-all-objects, since its
> purpose is to show the rest of the input line (and there are no input
> lines!).
>

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.

> That might be a problem later if we change the behavior (e.g., to say
> "%(rest) does not make sense with --batch-all-objects"). But I'm also OK
> leaving it for now; somebody later can dig up this commit via git-blame.
>
Yes, I think this feature can be completed later.(not in this patch)
Show 11 quoted lines
> > +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
> > +'
>
> And this one is a more extreme version of the "%(objectname)" one. It's
> useful as a regression test because we might later change the
> optimization so that %(objectname) ends up filling in the other object
> info.
>
Yes, it does not use atoms, so it is the most special.
Show 13 quoted lines
> There's a subtle dependency here on the "objects" file from the earlier
> test. In such a case, we'll often split that out as a separate setup
> step like:
>
>   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
>   '
>
> That makes it more clear that all three of the other tests are doing the
> same thing (just with different formats), and can be reordered, removed,
> etc, later if we wanted to. So not a correctness thing, but rather just
> communicating the structure of the tests to later readers.
>
Makes sense.
> -Peff

Thanks. -- ZheNing Hu

Previous: Jeff KingNext: Jeff King
Message 4 of 16 in “[GSOC] cat-file: fix --batch report changed-type bug”
  1. 0/2 [GSOC] cat-file: fix --batch report changed-type bugZheNing Hu via GitGitGadget, Jun 1, 2021
  2. 1/2 [GSOC] cat-file: fix --batch report changed-type bugZheNing Hu via GitGitGadget, Jun 1, 2021
  3. Jeff KingJun 1, 2021
  4. ZheNing HuJun 2, 2021
  5. Jeff KingJun 2, 2021
  6. ZheNing HuJun 3, 2021
  7. ZheNing HuJun 3, 2021
  8. Jeff KingJun 3, 2021
  9. 2/2 [GSOC] cat-file: merge two block into oneZheNing Hu via GitGitGadget, Jun 1, 2021
  10. Jeff KingJun 1, 2021
  11. ZheNing HuJun 2, 2021
  12. 0/2 [GSOC] cat-file: fix --batch report changed-type bugZheNing Hu via GitGitGadget, Jun 3, 2021
  13. 1/2 [GSOC] cat-file: handle trivial --batch format with --batch-all-objectsZheNing Hu via GitGitGadget, Jun 3, 2021
  14. 2/2 [GSOC] cat-file: merge two block into oneZheNing Hu via GitGitGadget, Jun 3, 2021
  15. Jeff KingJun 3, 2021
  16. Junio C HamanoJun 3, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.