On Fri, Mar 30, 2018 at 10:48 PM, Jeff King <p...@peff.net> 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