Ayush Chandekar <ayu.chandekar@xxxxxxxxx> writes: > This patch series aims to remove global variables related to > sparse-checkout from the global scope and to remove the definition > '#define USE_THE_REPOSITORY_VARIABLE' from a few files. > > It contains three patches: > > 1 - Remove the global variable 'core_apply_sparse_checkout' and > move its setting to the 'struct repo_settings'. Also remove the > definition '#define USE_THE_REPOSITORY_VARIABLE' from > "builtin/backfill.c". > > 2 - Remove the global variable 'core_sparse_checkout_cone' and > move its setting to the 'struct repo_settings'. > > 3 - Remove the global variable > 'sparse_expect_files_outside_of_patterns` and move its setting to > 'struct repo_settings'. Also remove the definition '#define > USE_THE_REPOSITORY_VARIABLE' from "sparse-index.c" > -- > Discussions since v5: > > * For 1/3 and 2/3, Junio told me that it was concerning to put so > many calls to `prepare_repo_settings()` so I tried to minimize the > calls and made sure that there's no useless calling. I didn't mean that the number of places is the problem. What I found troubling was that this is not done in any central place, so it is hard to notice even if some random cmd_foo() failed to call the function before doing its real work. For example, shouldn't we be able to, at least for built-in commands that have RUN_SETUP bit set, centrally call prepare_repo_settings() somewhere late in git.c:run_builtin() after we figure out what should be in the_repository? Now historically, setting up a repository may never have involved opening and parsing tons of configuration files, so such a change may be incurring extra overhead we did not have to pay, so it needs a lot more thought than just trying to minimize the number of calls, but some performance measurement. > * For 3/3, Phillip told me that it broke user-facing as it will be > parsed quite late in the callchain and might throw an error mid > operation which we do not want. So has the behaviour change caused by 3/3 been resolved? A meta-level comment and a half. * Please do not use "-- " (that is a line that has dash dash and a single space and nothing else on it) lightly. It is called signature line and often MUA pays attention to it when responding to a message with such a line by omitting everything after it (which is supposed to be your "who I am" advertisement) when quoting the original. Since you had one before the "discussions since v5" section and the range-diff, I had to manually resurrect the part after the signature line while composing this message. * This throws everything in repo_settings, but these settings are inherently per repository and they are meaningful only when you are working with a repository. What makes us choose to make them new members in the repo_settings structure, not direct members in the repository structure? Not an objection and not a suggestion to move them out of the repo_settings and to the repository proper. Just wanted to hear the reasoning behind it (and have the rationale clearly documented, preferrably in the proposed log messages).
