Ayush Chandekar <ayu.chandekar@xxxxxxxxx> writes: > The setting "core.sparsecheckout" is stored in the global > `core_apply_sparse_checkout` and is populated in config.c. Refactor the > code to store it in the variable `sparse_checkout` in the struct > `repo_settings`. Also, create functions to set and get the value of the > setting and update all the occurrences. > > This also allows us to remove the definition `#define > USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'. > > This change is part of an ongoing effort to eliminate global variables, > improve modularity and help libify the codebase. > > Mentored-by: Christian Couder <christian.couder@xxxxxxxxx> > Mentored-by: Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx> > Signed-off-by: Ayush Chandekar <ayu.chandekar@xxxxxxxxx> > --- It may make sense to move the sparse-checkout bit from the process-wide global to per-repository settings. One advantage of having it at the global level is that the code that does not require to be in a repository can refer to it, but the nature of this variable requires a repository to make sense anyway, so that advantage does not apply to it. But looking at members of repo.settings, among 25+ of them, there are only a handful of variables with getter-setter pair, and this patch seems to add yet another for a simple boolean. Among existing setters and getters, prevailing pattern seem to be /* getter */ if (!repo->settings.foo) repo_cfg_ulong(repo, "core.foo", &repo->settings.foo, FOO_DEFAULT); return repo->settings.foo; /* setter */ repo->settings.foo = value; but there are more involved ones. This new one adopts the most expensive pattern from get_warn_ambiguous_refs() where prepare_repo_settings() is used, as if it fears to be fed a repo instance that hasn't been prepared yet, and it even does so for its setter, which I think is probably a mistake. And there are tons of members in repo-settings that are initialized in prepare_repo_settings() (perhaps reading from the configuration file or assigned their default values) and without having an explicit setter/getter pair. If the code paths that depends on the value of these members are functioning correctly (and they should, given how widely Git is used these days and not having heard about bugs coming from these variables), would not it mean that in the program start up sequence, prepare_repo_settings() is called early enough so that all the code paths that care about these repository settings values do not have to worry about having a getter setter pair at all? What is it so special about sparse-checkout bit that cannot be using the same "initialize in prepare_repo_settings() and access it directly thereafter" model, or "ah, do you need to know about this bit? Let me see if I already have figured it out, and otherwise let me read from the configuration, and give it back to you" model? My gut feeling, if I have to choose between "lazy loading" and "popluate in prepare_repo_settings() and then access the member directly thereafter" for this variable, I may pick the latter for this particular variable.
