Re: [PATCH 4/5] archive-tar: stream large blobs to tar file
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 30, 2012, 21:36 UTC
- Message-ID
- <7vhaw0ewb8.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4F9EFF39.4010804@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 54 quoted lines
>> +static int stream_blob_to_file(const unsigned char *sha1)
>> +{
>> + struct git_istream *st;
>> + enum object_type type;
>> + unsigned long sz;
>> +
>> + st = open_istream(sha1,&type,&sz, NULL);
>> + if (!st)
>> + return error("cannot stream blob %s", sha1_to_hex(sha1));
>> + for (;;) {
>> + char buf[BLOCKSIZE];
>> + ssize_t readlen;
>> +
>> + readlen = read_istream(st, buf, sizeof(buf));
>> +
>> + if (readlen <= 0)
>> + return readlen;
>> + write_blocked(buf, readlen);
>> + }
>> + close_istream(st);
>> + return 0;
>> +}
>
> The stream is never closed. Perhaps squash this in?
>
> diff --git a/archive-tar.c b/archive-tar.c
> index 506c8cb..6109fd3 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -66,6 +66,7 @@ static void write_blocked(const void *data, unsigned long size)
> static int stream_blob_to_file(const unsigned char *sha1)
> {
> struct git_istream *st;
> + ssize_t readlen;
> enum object_type type;
> unsigned long sz;
>
> @@ -74,16 +75,15 @@ static int stream_blob_to_file(const unsigned char *sha1)
> return error("cannot stream blob %s", sha1_to_hex(sha1));
> for (;;) {
> char buf[BLOCKSIZE];
> - ssize_t readlen;
>
> readlen = read_istream(st, buf, sizeof(buf));
>
> if (readlen <= 0)
> - return readlen;
> + break;
> write_blocked(buf, readlen);
> }
> close_istream(st);
> - return 0;
> + return readlen;
> }Your patch on top obviouly is the right thing to do, but reading the code again, I am not sure if the original is correct. read_istream() itself does not promise that it will always fill the buffer before returning (it could return with a short read). It seems incorrect that the caller does not loop to avoid padding a short read with padding by calling write_blocked().