Re: [PATCH v2 07/10] object_info: content_limit only applies to blobs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2024, 22:02 UTC
- Message-ID
- <xmqqwmk3eydh.fsf@gitster.g>
- In-Reply-To
- <20240823224630.1180772-8-e@80x24.org>
Eric Wong <e@80x24.org> writes:
Show 28 quoted lines
> Streaming is only supported for blobs, so we'd end up having to
> slurp all the other object types into memory regardless. So
> slurp all the non-blob types up front when requesting content
> since we always handle them in-core, anyways.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
> builtin/cat-file.c | 21 +++++++++++++++++++--
> object-file.c | 3 ++-
> packfile.c | 8 +++++---
> t/t1006-cat-file.sh | 19 ++++++++++++++++---
> 4 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 8debcdca3e..2aedd62324 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -385,7 +385,24 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
> assert(data->info.typep);
>
> if (data->content) {
> - batch_write(opt, data->content, data->size);
> + void *content = data->content;
> + unsigned long size = data->size;
> +
> + data->content = NULL;
> + if (use_mailmap && (data->type == OBJ_COMMIT ||
> + data->type == OBJ_TAG)) {Line-wrap at higher level in the parse tree, i.e.
if (use_mailmap &&
(data->type == OBJ_COMMIT || data->type == OBJ_TAG)) {Show 9 quoted lines
> + size_t s = size;
> +
> + if (data->info.whence == OI_DBCACHED) {
> + content = xmemdupz(content, s);
> + data->info.whence = OI_PACKED;
> + }
> +
> + content = replace_idents_using_mailmap(content, &s);
> + size = cast_size_t_to_ulong(s);This piece of code does look good, but it makes me wonder where the need to duplicate the code comes from. Before this patch, if somebody asked use_mailmap, we must have been making the replace_idents_using_mailmap() call and showing the result elsewhere in the existing code, and it is curious why that code path is not removed, or if we can share more common code with that code paths here.
Show 12 quoted lines
> + }
> +
> + batch_write(opt, content, size);
> switch (data->info.whence) {
> case OI_CACHED:
> /*
> @@ -395,7 +412,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
> BUG("TODO OI_CACHED support not done");
> case OI_LOOSE:
> case OI_PACKED:
> - FREE_AND_NULL(data->content);
> + free(content);data->content has been nuked earlier, and content that holds the new copy with replaced one is freed (and the original data->content was freed by replace_idents_using_mailmap(), so there is no leak or double-free here. Good.
Show 14 quoted lines
> diff --git a/object-file.c b/object-file.c
> index 19100e823d..59842cfe1b 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1492,7 +1492,8 @@ static int loose_object_info(struct repository *r,
>
> if (!oi->contentp)
> break;
> - if (oi->content_limit && *oi->sizep > oi->content_limit) {
> + if (oi->content_limit && *oi->typep == OBJ_BLOB &&
> + *oi->sizep > oi->content_limit) {
> git_inflate_end(&stream);
> oi->contentp = NULL;
> goto cleanup;OK, so non-BLOB objects we would fall through this if/else cascade and inflate them fully.