On 4/5/07, Junio C Hamano <junkio@xxxxxxx> wrote:
"Dana How" <danahow@xxxxxxxxx> 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@xxxxxxxxx +1 650 804 5991 cell
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html