From: Dana How Date: Thu, 05 Apr 2007 23:41:03 GMT Subject: Re: [PATCH 10/13] update delta handling in write_object() for --pack-limit Message-ID: <56b7f5510704051641s69c9bdf1t8ce75385456bb4da@mail.gmail.com> In-Reply-To: <7v3b3emmv6.fsf@assigned-by-dhcp.cox.net> On 4/5/07, Junio C Hamano wrote: > "Dana How" 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