Re: [PATCH] index-pack: correctly initialize appended objects

From: Shawn O. Pearce <spearce@spearce.org>
Date: Jul 25, 2008, 17:20 UTC
Message-ID: <20080725172059.GD21117@spearce.org>
In-Reply-To: <20080725171315.GA27285@atjola.homenet>

Bjjjrn Steinbrink <B.Steinbrink@gmx.de> wrote:

Show 10 quoted lines

> When index-pack completes a thin pack it appends objects to the pack.
> Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when
> resolving deltas) such an object can be pruned in case of memory pressure.
> 
> To be able to re-read the object later, a few more fields have to be set.
> 
> Noticed by Pierre Habouzit.
> 
> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> Acked-by: Nicolas Pitre <nico@cam.org>

Acked-by: Shawn O. Pearce <spearce@spearce.org>

Show 6 quoted lines

>     On 2008.07.25 15:15:48 +0200, Johannes Schindelin wrote:
>     > So, let's add the comment as Nico suggested, and set real_type,
>     > too?
> 
>     OK, I hope the comment is what was expected. My lack of knowledge
>     made we wonder what to write... :-/

The commit message makes sense to me.  :)

Show 6 quoted lines

>     > (And it would be smashing if you could verify that the type is
>     > indeed correctly set to non-delta...)
> 
>     Hm, we get the object via read_sha1_file, can that return a delta? I
>     would not expect it to.  Sorry, never looked at those code paths
>     (and don't have the time to investigate at the moment).

read_sha1_file() _never_ returns a delta. It always reutrns the whole object, even if the object was stored as a delta in a pack somewhere. The function is widely used within git to read an object for processing, without the caller needing to worry about the types of compression used to store the object.

Show 17 quoted lines

> diff --git a/index-pack.c b/index-pack.c
> index ac20a46..d757b07 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -699,6 +699,12 @@ static struct object_entry *append_obj_to_pack(
>  	write_or_die(output_fd, header, n);
>  	obj[0].idx.crc32 = crc32(0, Z_NULL, 0);
>  	obj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);
> +	// This object comes from outside the thin pack, so we need to
> +	// initialize the size and type fields
> +	obj[0].hdr_size = n;
> +	obj[0].size = size;
> +	obj[0].type = type;
> +	obj[0].real_type = type;
>  	obj[1].idx.offset = obj[0].idx.offset + n;
>  	obj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);
>  	hashcpy(obj->idx.sha1, sha1);

-- 
Shawn.