On Mon, May 20, 2019 at 8:17 PM Jeff King <peff@xxxxxxxx> wrote: > The patch looks good, though I wonder if we could simplify even further > by just embedding an index into the repository object. The purpose of > having it as a pointer, I think, is so that the_repository can point to > the_index. But we could possibly hide the latter behind some macro > trickery like: > > #define the_index (the_repository->index) > > I spent a few minutes on a proof of concept patch, but it gets a bit > hairy: > > 1. There are some circular dependencies in the header files. We'd need > repository.h to depend on cache.h to get the definition of > index_state, but the latter includes repository.h. We'd need to > break the index bits out of cache.h into index.h, which in turn > requires breaking out some other parts. I did a sloppy job of it in > the patch below. > > 2. There are hundreds of spots that need to swap out "repo->index" for > "&repo->index". In the patch below I just did enough to compile > archive-zip.o, to illustrate. :) You are more thorough than me. I saw #2 first and immediately backed off (partly for a selfish reason: I have plenty of the_repo conversion patches in queue and anything touching "repo" may delay those patches even more). There's also #3 but this one is minor. So far 'struct repo' is more of a glue of things. Embedding index_state in it while leaving object_store, ref_store... pointers feels inconsistent and a bit weird. It's not a strong reason for making index_state a pointer too, but if we have to deal with pointers anyway... > So it's definitely non-trivial to go that way. I'm not sure if it's > worth the effort to switch at this point, but even if it is, your patch > seems like a good thing to do in the meantime. > > Either way, I think we could probably revert the non-test portion of my > 581d2fd9f2 (get_oid: handle NULL repo->index, 2019-05-14) after this. Yeah. I'm thinking of doing that after, scanning for similar lines too. But it looks like it's the only one. Will fix in v2. -- Duy
