Re: [PATCH v2 07/10] object_info: content_limit only applies to blobs

[Junio C Hamano <gitster@pobox.com>]

Eric Wong <e@80x24.org> writes:

> 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)) {

> +			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.

> +		}
> +
> +		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.

> 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.