Junio C Hamano <gitster@xxxxxxxxx> 于2021年5月31日周一 上午5:15写道：
> > 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.
> 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.
> 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).
> > 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".
>
> > + 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.
> > + }
>
> 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
