Re: [PATCH v7 06/13] pack-objects: move in_pack out of struct object_entry

On Fri, Mar 30, 2018 at 10:48 PM, Jeff King [off-list ref] wrote:

On Sat, Mar 24, 2018 at 07:33:46AM +0100, Nguyễn Thái Ngọc Duy wrote:

quoted

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.

quoted

+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.

quoted

+     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[].

quoted

 /*
+ * 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