Re: [PATCH 2/3] setup: have the_repository use the_index
From: Junio C Hamano <hidden>
Date: 2017-07-18 21:34:38
Junio C Hamano [off-list ref] writes:
Jonathan Nieder [off-list ref] writes:quoted
All that said, I don't have a strong opinion on this. Both the 1-word approach (a pointer) and 24-word approach (embedding) are tolerable and there are reasons to prefer each.I do not care too much about 24-word wastage. If this were not "a pointer pretending to be embedded object", the fix in 1/3 wouldn't have been necessary. I am worried about this being an invitations for such unnecesasry bugs.
Another thing I noticed that you already pointed out was this bit in your review message:
quoted hunk
I wonder if this can be done sooner. For example, does the following work? This way, 'the_repository->index == &the_index' would be an invariant that always holds, even in the early setup stage before setup_git_directory_gently has run completely. Thanks, Jonathandiff --git i/repository.c w/repository.c index edca907404..bdc1f93282 100644 --- i/repository.c +++ w/repository.c@@ -4,7 +4,7 @@ #include "submodule-config.h" /* The main repository */ -static struct repository the_repo; +static struct repository the_repo = { .index = &the_index }; struct repository *the_repository = &the_repo; static char *git_path_from_env(const char *envvar, const char *git_dir,
With a pointer that can point at a random instance of index_state, the current "struct repository" allows two or more instances of it to share the same index_state. I do not think that is a designed and a desirable "feature" but an invitation for a mistake. Embedding the real instance in it would solve that, too. So, after saying "I am not (yet) telling you to fix the design" and then hearing what a potential advanage could be (and none of that was a convincing one), I am inclined to say that this eventually needs to be fixed, preferrably before too much code starts relying on it and making it more work to fix it later.