Re: [PATCH 10/13] update delta handling in write_object() for --pack-limit

From: Junio C Hamano <junkio@cox.net>
Date: Apr 5, 2007, 23:18 UTC
Message-ID: <7v3b3emmv6.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <56b7f5510704051538na4393d7k5e51ed2a511cc86e@mail.gmail.com>

"Dana How" <danahow@gmail.com> writes:

Show 29 quoted lines

> Subject: [PATCH 10/13] update delta handling in write_object() for --pack-limit
>
> ---
>  builtin-pack-objects.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index ccc2d15..a243eed 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -419,13 +419,17 @@ static off_t write_object(struct sha1file *f,
>  	}
>  
>  	if (!to_reuse) {
> +		int usable_delta =	!entry->delta ? 0 :
> +					!offset_limit ? 1 :
> +					entry->delta->no_write ? 0 :
> +					entry->delta->offset ? 1 : 0;
>  		buf = read_sha1_file(entry->sha1, &type, &size);
>  		if (!buf)
>  			die("unable to read %s", sha1_to_hex(entry->sha1));
>  		if (size != entry->size)
>  			die("object %s size inconsistency (%lu vs %lu)",
>  			    sha1_to_hex(entry->sha1), size, entry->size);
> -		if (entry->delta) {
> +		if (usable_delta) {
>  			buf = delta_against(buf, size, entry);
>  			size = entry->delta_size;
>  			obj_type = (allow_ofs_delta && entry->delta->offset) ?

This really needs explanation on *why*, at least in the commit log, and perhaps also in the code as comment.

Here is my attempt to understand that logic (please make necessary corrections).

 (1) When there is no delta base found by the earlier
     find_deltas()/try_delta() loop, obviously we cannot write
     deltified so we write plain.

 (2) Otherwise if we are not offset limited, then we keep the
     old way of using that delta base.

 (3) If the delta base is not in this pack (because of
     offset-limit chomping the pack into two or more), then we
     cannot use it as the base.

 (4) If the delta has already been written, we can use it as the
     base for this object, but otherwise we cannot.

(3) makes me wonder if we can still allow it in the thin-pack case by just loosening the condition here.

Also I need explanation to understand why (4) is needed -- doesn't write_one() always write base object out before writing a deltified object that depends on it?