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

["Dana How" <danahow@gmail.com>]

On 4/5/07, Junio C Hamano <junkio@cox.net> wrote:
> "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".

> 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