On Fri, Mar 30, 2018 at 10:48 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Sat, Mar 24, 2018 at 07:33:46AM +0100, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index e1244918a5..b41610569e 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -29,6 +29,8 @@
>> #include "list.h"
>> #include "packfile.h"
>>
>> +#define IN_PACK(obj) oe_in_pack(&to_pack, obj)
>
> How come this one gets a macro, but the earlier conversions don't?
>
> I guess the problem is that oe_in_pack() is defined in the generic
> pack-objects.h, but &to_pack is only in builtin/pack-objects.c?
>
> I wonder if it would be that bad to just say oe_in_pack(&to_pack, obj)
> everywhere. It's longer, but it makes the code slightly less magical to
> read.
Longer was exactly why I added these macros (with the hope that the
macro upper case names already ring a "it's magical" bell). Should I
drop all these macros? Some code becomes a lot more verbose though.
>> +static void prepare_in_pack_by_idx(struct packing_data *pdata)
>> +{
>> + struct packed_git **mapping, *p;
>> + int cnt = 0, nr = 1 << OE_IN_PACK_BITS;
>> +
>> + if (getenv("GIT_TEST_FULL_IN_PACK_ARRAY")) {
>> + /*
>> + * leave in_pack_by_idx NULL to force in_pack[] to be
>> + * used instead
>> + */
>> + return;
>> + }
>> +
>> + ALLOC_ARRAY(mapping, nr);
>> + mapping[cnt++] = NULL; /* zero index must be mapped to NULL */
>
> Why? I guess because index==0 is a sentinel for "we're using the small
> index numbers?"
No because by default all values in object_entry is zero (or NULL). If
I remember correctly, some code will skip setting in_pack pointer to
leave it NULL. When we convert it to an index, it should also point to
NULL.
>> + prepare_packed_git();
>> + for (p = packed_git; p; p = p->next, cnt++) {
>> + if (cnt == nr) {
>> + free(mapping);
>> + return;
>> + }
>> + p->index = cnt;
>> + mapping[cnt] = p;
>> + }
>> + pdata->in_pack_by_idx = mapping;
>> +}
>
> What happens if we later have to reprepare_packed_git() and end up with
> more packs? We only call this for the first pack.
>
> It may well be handled, but I'm having trouble following the code to see
> if it is. And I doubt that case is covered by our test suite (since it
> inherently involves a race).
I don't think I covered this case. But since "index" field in
packed_git should be zero for the new packs, we could check and either
add it to in_pack_by_idx[].
>> /*
>> + * The size of struct nearly determines pack-objects's memory
>> + * consumption. This struct is packed tight for that reason. When you
>> + * add or reorder something in this struct, think a bit about this.
>> + *
>
> It's funny to see this warning come in the middle. Should it be part of
> the final struct reordering at the end?
It was at the end in some version, the I shuffled the patches and
forgot about this one :)
--
Duy
