From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Mon, 30 Apr 2012 22:12:00 GMT
Subject: Re: [PATCH 4/5] archive-tar: stream large blobs to tar file
Message-ID: <4F9F0E30.70403@lsrfire.ath.cx>
In-Reply-To: <7vhaw0ewb8.fsf@alter.siamese.dyndns.org>

Am 30.04.2012 23:36, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
>
>>> +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().

Yes, indeed, good catch.  It could also write to block directly and 
avoid copying the buffer again.  The tail clearing part of 
write_blocked() can certainly be reused, but the rest will probably have 
to be reimplemented around read_istream().

René