Re: [RFC PATCH 2/5] Add delta-islands.{c,h}

From: Christian Couder <christian.couder@gmail.com>
Date: Jul 22, 2018, 13:57 UTC
Message-ID: <CAP8UFD0K+F5D2q5UY-aX9tmtkqvgMiKf=bq+wE+yZuxzyQst_g@mail.gmail.com>
In-Reply-To: <CACsJy8AzksB=rfaX_dfboMXHjqj6gj+erdF6eRFAKmWA1-3PUg@mail.gmail.com>

On Sun, Jul 22, 2018 at 10:50 AM, Duy Nguyen <pclouds@gmail.com> wrote:

> On Sun, Jul 22, 2018 at 7:51 AM Christian Couder
> <christian.couder@gmail.com> wrote:

Show 6 quoted lines

>> +pack.island::
>> +       A regular expression configuring a set of delta islands. See
>> +       "DELTA ISLANDS" in linkgit:git-pack-objects[1] for details.
>> +
>
> That section is not added until 3/5 though.

Yeah, so I guess it is better to move this hunk to 3/5 and keep pack.island undocumented until the delta islands code is actually used by pack-objects.

Show 9 quoted lines

>> diff --git a/delta-islands.c b/delta-islands.c
>> new file mode 100644
>> index 0000000000..645fe966c5
>> --- /dev/null
>> +++ b/delta-islands.c
>> @@ -0,0 +1,490 @@
>> +#include "builtin.h"
>
> A bit weird that builtin.h would be needed...

Yeah, I will get rid of that include in the next iteration.

>> +       if (progress)
>> +               progress_state = start_progress("Propagating island marks", nr);
>
> _() (same comment for other strings too)

Ok, the strings will be marked for translation in the next iteration.

Show 16 quoted lines

>> diff --git a/pack-objects.h b/pack-objects.h
>> index edf74dabdd..8eecd67991 100644
>> --- a/pack-objects.h
>> +++ b/pack-objects.h
>> @@ -100,6 +100,10 @@ struct object_entry {
>>         unsigned type_:TYPE_BITS;
>>         unsigned no_try_delta:1;
>>         unsigned in_pack_type:TYPE_BITS; /* could be delta */
>> +
>> +       unsigned int tree_depth; /* should be repositioned for packing? */
>> +       unsigned char layer;
>> +
>
> This looks very much like an optional feature. To avoid increasing
> pack-objects memory usage for common case, please move this to struct
> packing_data.

Ok, I will take a look at that.

Thanks for the review, Christian.