Re: [PATCH] Handle deltified object correctly in git-*-pull family.

On Thu, 2 Jun 2005, Junio C Hamano wrote:

> The initial parts of each retrieved SHA1 file is inflated and
> inspected to see if it is deltified, and its base object is
> asked from the remote side when it is.  Since this partial
> inflation and inspection has a small performance hit, it can
> optionally be skipped by giving -d flag to git-*-pull commands.

It is still way more expensive than it could.

> diff --git a/sha1_file.c b/sha1_file.c
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -325,7 +325,13 @@ void *unpack_sha1_rest(z_stream *stream,
>  	int bytes = strlen(buffer) + 1;
>  	char *buf = xmalloc(1+size);
>  
> -	memcpy(buf, buffer + bytes, stream->total_out - bytes);
> +	/* (stream->total_out - bytes) is what we already have.  The
> +	 * caller could be asking for something smaller than that.
> +	 */
> +	if (size < stream->total_out - bytes)
> +		memcpy(buf, buffer + bytes, size);
> +	else
> +		memcpy(buf, buffer + bytes, stream->total_out - bytes);
>  	bytes = stream->total_out - bytes;
>  	if (bytes < size) {
>  		stream->next_out = buf + bytes;

This hunk is completely unneeded.

> @@ -401,6 +407,41 @@ void * unpack_sha1_file(void *map, unsig
>  	return unpack_sha1_rest(&stream, hdr, *size);
>  }
>  
> +int sha1_delta_base(const unsigned char *sha1, unsigned char *base_sha1)
> +{
> +	int ret;
> +	unsigned long mapsize, size;
> +	void *map;
> +	z_stream stream;
> +	char hdr[1024], type[20];

Don't make hdr 1024 bytes long.  If you do so unpack_sha1_header() will 
uncompress up to 1024 bytes which is way above required.  Instead, 
consider a value of say 64 which is plenty sufficient (10 for the type 
string, another 10 for the size, 20 for the reference sha1 and another 
10 for the beginning of the delta data that must include the size, and 
the rest for good measure).

> +	void *delta_data_head;
> +
> +	map = map_sha1_file(sha1, &mapsize);
> +	if (!map)
> +		return -1;
> +	ret = unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr));
> +	if (ret < Z_OK || parse_sha1_header(hdr, type, &size) < 0) {
> +		ret = -1;
> +		goto out;
> +	}
> +	if (strcmp(type, "delta")) {
> +		ret = 0;
> +		goto out;
> +	}
> +	delta_data_head = unpack_sha1_rest(&stream, hdr, 20);

Here you don't need to call unpack_sha1_rest() at all which would call 
xmalloc and another memcpy needlessly.  Instead, just use:

	memcpy(base_sha1, hdr + strlen(hdr) + 1, 20);

and you're done.  No need to call an extra free() either.

And maybe this function should live in delta.c instead?


Nicolas
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html