Re: [PATCH 10/13] update delta handling in write_object() for --pack-limit
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
