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

From: Dana How <danahow@gmail.com>
Date: Apr 5, 2007, 23:41 UTC
Message-ID: <56b7f5510704051641s69c9bdf1t8ce75385456bb4da@mail.gmail.com>
In-Reply-To: <7v3b3emmv6.fsf@assigned-by-dhcp.cox.net>

On 4/5/07, Junio C Hamano <junkio@cox.net> wrote:

Show 20 quoted lines

> "Dana How" <danahow@gmail.com> writes:
> >       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.

Yes, I need retraining ;-) I'm accustomed to more expensive commits in which the log message describes the objective, not the details. Or, I could have followed the example of the comments on the initialization for "to_reuse".

Show 19 quoted lines

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

You are correct. In response to someone's previous question I think I said I hadn't handled the --thin-pack cases yet. Do you think these will matter? There is strong sentiment on this list that a pack should be split at the receiver, not the transmitter, so --thin-pack and --pack-limit wouldn't be used together. If you think this combination does matter, I'd prefer to add the extra cases in a separate follow-on patch if that's OK.

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

Yes, write_one() does that. I was being too cautious, since I had only known (some of) the code for a few hours!

-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell