Jeff King wrote:
> We could do the same for the type. However, besides our
> consistency check, we also care about the type in deciding
> whether to stream or not. We therefore make sure to always
> trigger a type lookup when we are printing, so that
This "We make sure" is the behavior after this patch, not before,
right?
[...]
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)
> die("object %s disappeared", sha1_to_hex(sha1));
> if (type != data->type)
> die("object %s changed type!?", sha1_to_hex(sha1));
Maybe an assert(data.info.typep) or similar would make this more
locally readable.
[...]
> @@ -276,6 +276,13 @@ static int batch_objects(struct batch_options *opt)
> data.mark_query = 0;
>
> + /*
> + * If we are printing out the object, then always fill in the type,
> + * since we will want to decide whether or not to stream.
> + */
> + if (opt->print_contents)
> + data.info.typep = &data.type;
Oof. I guess this means that the optimization from 98e2092b wasn't being
applied by 'git cat-file --batch' with format specifiers that don't
include %(objecttype), but no one would have noticed because of the
"changed type" thing. :)
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -85,6 +85,28 @@ $content"
> git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
> test_cmp expect actual
> '
> +
> + test -z "$content" ||
> + test_expect_success "--batch without type ($type)" '
> + {
> + echo "$size" &&
> + maybe_remove_timestamp "$content" $no_ts
> + } >expect &&
> + echo $sha1 | git cat-file --batch="%(objectsize)" >actual.full &&
> + maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
> + test_cmp expect actual
> + '
> +
> + test -z "$content" ||
> + test_expect_success "--batch without size ($type)" '
> + {
> + echo "$type" &&
> + maybe_remove_timestamp "$content" $no_ts
> + } >expect &&
> + echo $sha1 | git cat-file --batch="%(objecttype)" >actual.full &&
> + maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
> + test_cmp expect actual
> + '
> }
Looks good.
(not about this patch) I suspect a test_cmp_ignore_timestamp helper
could simplify these tests somewhat. :)
For what it's worth, with or without commit message changes or the
check that data->type is initialized,
Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html