Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 30, 2021, 21:15 UTC
- Message-ID
- <xmqqy2bv3ovf.fsf@gitster.g>
- In-Reply-To
- <pull.965.git.1622363366722.gitgitgadget@gmail.com>
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 13 quoted lines
> From: ZheNing Hu <adlternative@gmail.com> > > When we use `--batch` with no atoms formatting and use > `--batch-all-objects` at the same time (e.g. > `git cat-file --batch="batman" --batch-all-objects`), > Git will exit and report "object xxx changed type!?". > > This is because we have a format string which does not > contain any atoms, so skip_object_info option will be > set if we also use --batch-all-objects, and then > `oid_object_info_extended()` will be skipped in > `batch_object_write()`, it cause object type to not be > collected. Therefore, it reported object type has changed.
The above analysis on how these die()s get hit makes sense, but ...
> 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.
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.
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?
Show 16 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?
> + 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)?
> + }
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));Show 17 quoted lines
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh > index 5d2dc99b74ad..9b0f1ae5ef8b 100755 > --- a/t/t1006-cat-file.sh > +++ b/t/t1006-cat-file.sh > @@ -586,4 +586,10 @@ test_expect_success 'cat-file --unordered works' ' > test_cmp expect actual > ' > > +test_expect_success 'cat-file --batch="batman" with --batch-all-objects will work' ' > + git -C all-two cat-file --batch-all-objects --batch="%(objectname)" | wc -l >expect && > + git -C all-two cat-file --batch-all-objects --batch="batman" | wc -l >actual && > + test_cmp expect actual > +' > + > test_done > > base-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5