On Mon, Oct 28, 2024 at 02:43:39PM +0100, Karthik Nayak wrote:

The struct `packed_git` holds information regarding a packed object
file. Let's add the repository variable to this object, to represent the
repository that this packfile belongs to. This helps remove dependency
on the global `the_repository` object in `packfile.c` by simply using
repository information now readily available in the struct.

Makes sense, good. I think it would be useful here to capture some of
the discussion from just before you sent this series to indicate why
it's OK to use the_repository even when we have alternates.

I think it is now quite obvious in retrospect, but let's do our future
selves a service by capturing it here, too ;-).

---
 10 files changed, 30 insertions(+), 16 deletions(-)

Oh, good. I am glad to see that this new approach is already yielding
far less disruptive of a change.

Makes sense. Here we are crafting the packfile by hand, so initializing
->repo directly makes sense here.

It would be nice if we could rewrite this in terms of
packfile.c:alloc_packed_git(), but that is a static function. Exposing
it as non-static is probably showing too much of the internals, so I
think leaving this as-is makes sense.

I wondered if ctx->r was the right choice here or not, but it is, and it
is (currently) always equal to the value of the_repository, so it's a
moot point. Let's keep going...

Ugh. I thought we had already added a repository field to our auxiliary
write_midx_context struct, but we have not, so this change looks right
to me.  Doing so (adding that new field) seems like it would be a good
piece of #leftoverbits.

OK, so here we're trusting the value of 'r' from the caller. That comes
from 64404a24cf (midx: pass a repository pointer, 2019-04-29), which is
doing the right thing. (As an aside, I thought that that change was from
when we added the --object-dir flag to 'git multi-pack-index', but the
change is in fact unrelated and has to do with adding installed packs to
the repository's MRU list).

Hmm. Do we need to include all of repository.h here? I don't think we
do, because we never peek into any of the fields of that structure from
within this header. So I think you could do something like:

--- 8< ---
diff --git a/object-store-ll.h b/object-store-ll.h
index 6f9f4276e6..bcfae2e1bf 100644
--- a/object-store-ll.h
+++ b/object-store-ll.h

@@ -4,13 +4,13 @@
 #include "hashmap.h"
 #include "object.h"
 #include "list.h"
-#include "repository.h"
 #include "thread-utils.h"
 #include "oidset.h"

 struct oidmap;
 struct oidtree;
 struct strbuf;
+struct repository;

 struct object_directory {
 	struct object_directory *next;
--- >8 ---

instead of #include-ing the whole thing, which would be preferable.

Calling this 'repo' makes sense, but...

Here and elsewhere, I think our usual convention is to call a 'struct
repository *' (when it is a formal parameter of some function) just "r"
instead of "repo".

At least that's what my intuition told me, and a very rough grep says
that '*r' appears as a parameter 815 times, while '*repo' appears only
577 times. It's close, but I think that '*r' is preferred here since
it's fewer characters.

 {
 	struct packed_git *p = xmalloc(st_add(sizeof(*p), extra));
 	memset(p, 0, sizeof(*p));

Not at all the fault of this patch, but it feels like a bit of a
foot-gun to allocate a bounds-checked version of 'sizeof(*p)+extra',
while only zero'ing the first 'sizeof(*p)' bytes. I think in all cases
where it actually matters via add_packed_git() we fill out that extra
space anyway, but it might be nice cleanup to do something like:

    struct packed_git *p;
    size_t sz = sizeof(*p) + extra;

    p = xcalloc(1, sz);

, or something. But that can be dealt with later and/or as #leftoverbits.

The rest is looking good, nicely done. Let's keep reading...

Thanks,
Taylor