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

From: Nicolas Pitre <nico@cam.org>
Date: Jun 2, 2005, 21:31 UTC
Message-ID: <Pine.LNX.4.63.0506021713330.17354@localhost.localdomain>
In-Reply-To: <7v3bs07fmu.fsf@assigned-by-dhcp.cox.net>

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

Show 5 quoted lines

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

Show 18 quoted lines

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

Show 11 quoted lines

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

Show 15 quoted lines

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