On Fri, Dec 4, 2020 at 12:25 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Han-Wen Nienhuys <hanwen@xxxxxxxxxx> writes: > > > If the_repository is only half-initialized at this point in init_db(), > > then why are we passing it in refs_init_db() just a couple of lines > > further? At what point the_repository considered initialized? > > I would have to say it probably depends on what callees expect. The > current implementation of refs_init_db() for files backend may not > need anything other than the hash algorithm enum, but many other > fields are missing, and they should ideally be populated, no? I think so too, but it looks like a major refactoring by itself, which is only very tangentially related to the reftable effort, so I'd like to punt on it. > For example, I see files_ref_store_create() cheats by calling > get_common_dir_noenv() to find out where the commondir is, instead > of ever asking the repository the ref store belongs to. At least, > get_main_ref_store() is told to get the ref store that belongs to > the_repository, and it would be the right place to learn relevant > pieces of information (for that matter, I am not sure why struct > ref_store does not have a pointer to a repository structure; perhaps > we are seeing the result of piecemeal evolution, not a designed > structure?). > > > I'm a bit at a loss here; I never learned how to cleanly work with so > > many global variables, so I'm happy to take your suggestion. > > I am only interested in giving a clear direction to future > developers where to populate the_repository's members (and nothing > else) if their enhancement needs members other than the hash > algorithm to be populated, as if it were the_repository initialized > in an already working repository (I am not talking about many global > variables, whichever you are referring to). You said earlier that maybe ref_store should hold a link to 'struct repository'. However, "struct repository" doesn't have a documented purpose (there is literally no comment documenting its purpose), so it's hard to tell upfront how to correctly configure the relation between "struct ref_store" and "struct repository". Currently, "struct repository" has a pointer to 'struct ref_store", which makes it suspect for there to be a pointer in the other direction as well. The reason I bring up global variables is that without them, the code would enforce a much more clear relation between different entities. Looking at the definition of repository, there seems to be some overlap in functionality between struct repository, struct repository_format, struct repo_settings, and config_set, and none of these structs have a clearly documented role. Before we change code, it's probably better to spell out what these structs are thought to be. > One way to do so would probably be to do something like the > attached. Thanks, I've amended my patch. -- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
