Re: [PATCH v4] fast-import: do not write bad delta for replaced subtrees

From: Dmitry Ivankov <divanorama@gmail.com>
Date: Aug 20, 2011, 18:28 UTC
Message-ID: <CA+gfSn_G0Q8=NsLr_Qku+oHwgkzBXajHpebLVT2SG4YDUUZD-g@mail.gmail.com>
In-Reply-To: <20110820174812.GD15864@elie.gateway.2wire.net>

On Sat, Aug 20, 2011 at 11:48 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 18 quoted lines

> Dmitry Ivankov wrote:
>
>> How about adding a new bit field "no_delta" instead?
>
> Currently the layout of "struct tree_entry_ms" is:
>
>        uint16_t mode;                  two bytes
>        unsigned char sha1[20]          20 bytes
>
> which adds up to 22 bytes.  Here is "struct tree_entry":
>
>        struct tree_entry *tree;                one machine word
>        struct atom_str *name;                  one machine word
>        struct tree_entry_ms versions[2];       44 bytes
>
> Although it only looks like it adds one byte per tree entry, in
> practice I suspect your patch adds four.  Is that worth it?  (The
> answer might be yes.  I'm not sure.)

Hm, we can make mode 1 byte in fast-import (only 8 values are used). But not in this patch of course.

Show 21 quoted lines

>
>> The patch is
>> smaller this way. Also could 04000 theoretically be S_IFDIR on some
>> platform?
>
> No, these modes are part of the format of objects as written on disk
> and over the wire, so when we meet a platform with S_IFDIR != 040000,
> there will have to be bigger changes (to distinguish between the
> platform's idea of file status and git's idea of modes).
>
>> - switch to a separate no_delta bit in tree_entry
>
> If it doesn't cost too much, this is a good idea.
>
>> - when setting no_delta = 1 don't check for S_ISDIR(versions[0].mode),
>>   this is a redundant check and logic duplication. Who knows, maybe some
>>   day we'll want to delta a tree against blob. :)
>
> Why?  When versions[0] is not a tree, the hack is not needed, since
> versions[0].mode and versions[0].sha1 accurately describe the delta
> base and are not inconsistent with anything.

Oh, right you are. The new check and one in store_tree look the same but the purpose differs.

>
> Thanks, that was helpful.
>