Re: [PATCH 03/11] cat-file: use streaming interface to print blobs

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2012, 17:44 UTC
Message-ID: <7vzkc49nnu.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1330329315-11407-4-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 19 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/cat-file.c |   22 ++++++++++++++++++++++
>  t/t1050-large.sh   |    2 +-
>  2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 8ed501f..3f3b558 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -82,6 +82,24 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long
>  		write_or_die(1, cp, endp - cp);
>  }
>  
> +static int write_blob(const unsigned char *sha1)
> +{
> +	unsigned char new_sha1[20];
> +
> +	if (sha1_object_info(sha1, NULL) == OBJ_TAG) {

This smells bad. Why in the world could an API be sane if lets a caller call "write_blob()" with something that can be a tag?

Both of your callsites call this function when (type == OBJ_BLOB), but the "case 0:" arm in the large switch in cat_one_file() only checks "expected type" which may not match the real type at all, so it is wrong to switch on that in the first place. In addition, that call site alone needs to deref tag to the requested/expected type.

This block does not belong to this function, but to only one of its callers among two.

Show 11 quoted lines

> +		enum object_type type;
> +		unsigned long size;
> +		char *buffer = read_sha1_file(sha1, &type, &size);
> +		if (memcmp(buffer, "object ", 7) ||
> +		    get_sha1_hex(buffer + 7, new_sha1))
> +			die("%s not a valid tag", sha1_to_hex(sha1));
> +		sha1 = new_sha1;
> +		free(buffer);
> +	}
> +
> +	return streaming_write_sha1(1, 0, sha1, OBJ_BLOB, NULL);

I do not think your previous refactoring added a fall-back codepath to the function you are calling here. In the original context, the caller of streaming_write_entry() made sure that the blob is suitable for streaming write by getting an istream, and called the function only when that is the case. Blobs unsuitable for streaming (e.g. an deltified object in a pack) were handled by the caller that decided not to call streaming_write_entry() with the conventional "read to core and then write it out" codepath.

And I do not think your updated caller in cat_one_file() is equipped to do so at all.

So it looks to me that this patch totally breaks the cat-file. What am I missing?

Show 35 quoted lines

> +}
> +
>  static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
>  {
>  	unsigned char sha1[20];
> @@ -127,6 +145,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
>  			return cmd_ls_tree(2, ls_args, NULL);
>  		}
>  
> +		if (type == OBJ_BLOB)
> +			return write_blob(sha1);
>  		buf = read_sha1_file(sha1, &type, &size);
>  		if (!buf)
>  			die("Cannot read object %s", obj_name);
> @@ -149,6 +169,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
>  		break;
>  
>  	case 0:
> +		if (type_from_string(exp_type) == OBJ_BLOB)
> +			return write_blob(sha1);
>  		buf = read_object_with_reference(sha1, exp_type, &size, NULL);
>  		break;
>  
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index f245e59..39a3e77 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -114,7 +114,7 @@ test_expect_success 'hash-object' '
>  	git hash-object large1
>  '
>  
> -test_expect_failure 'cat-file a large file' '
> +test_expect_success 'cat-file a large file' '
>  	git cat-file blob :large1 >/dev/null
>  '