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

From: ZheNing Hu <adlternative@gmail.com>
Date: May 31, 2021, 13:55 UTC
Message-ID: <CAOLTT8Qz5NBXKp0iF5+mFK7evhqiTH_TUf+Ve3BuO5iV1DESaQ@mail.gmail.com>
In-Reply-To: <xmqqy2bv3ovf.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年5月31日周一 上午5:15写道：

Show 5 quoted lines

> > So avoid checking changes in type and size when all_objects
> > and skip_object_info options are set at the same time.
>
> ... it is not immediately clear how the above conclusion follows.
>

My origin idea is: Now that it is known that the first "data->type" is 0, and the second "type" obtained is the real type of the object, they are different, so we can skip the comparison here.

Show 8 quoted lines

> An obvious alternative, however, is to avoid skipping object info
> when all objects is in use---but that goes directly against why this
> "skip" mechanism was added at 845de33a (cat-file: avoid noop calls
> to sha1_object_info_extended, 2016-05-18).
>
> Which makes me suspect that the solution presented here would be
> going in the right direction.
>

Looking at it now, Peff's solution may be correct.

Show 7 quoted lines

> There however is one curious thing about this.  The log message of
> the original commit that introduced this optimization does use the
> batch-check and batch-all-objects at the same time.  Was this
> breakage not there when the original was written and we broke it in
> a later update?  If so, with what commit?  Can that commit have
> broken other places in cat-file in a similar manner?
>

Because it's bug of --batch with --batch-all-objects, not the bug of
 --batch-check with --batch-all-objects. The extra broken atoms
are %(rest), %(objectname).

Show 20 quoted lines

> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 5ebf13359e83..5f9578f9b86b 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -345,11 +345,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
> >               contents = read_object_file(oid, &type, &size);
> >               if (!contents)
> >                       die("object %s disappeared", oid_to_hex(oid));
> > -             if (type != data->type)
> > -                     die("object %s changed type!?", oid_to_hex(oid));
> > -             if (data->info.sizep && size != data->size)
> > -                     die("object %s changed size!?", oid_to_hex(oid));
> > -
> > +             if (!(opt->all_objects && data->skip_object_info)) {
> > +                     if (type != data->type)
> > +                             die("object %s changed type!?", oid_to_hex(oid));
>
> When skip_object_info bit is set, we know data->type and date->size
> are unusable and should not be checked, regardless of the reason why
> skip_object_info bit is set, don't we?

Yes... We don't need to consider "opt->all_objects".

Show 8 quoted lines

>
> > +                     if (data->info.sizep && size != data->size)
> > +                             die("object %s changed size!?", oid_to_hex(oid));
>
> Does this check need to be modified at all?  Would info.sizep ever
> be set to non-NULL if skip_object_info is set (hence we are not
> calling object-info)?
>

Indeed, we don’t need to consider sizep.

Show 8 quoted lines

> > +             }
>
> In other words, shouldn't this patch just this one liner?
>
> -               if (type != data->type)
> +               if (data->skip_object_info && type != data->type)
>                         die("object %s changed type!?", oid_to_hex(oid));
>

With the original patch, this is the best code.

Thanks. -- ZheNing Hu