On Thu, Jul 24, 2025 at 3:44 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Ayush Chandekar <ayu.chandekar@xxxxxxxxx> writes: > > > > > * 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. > I was quite stumped as I don't know what the perfect solution for this would be. I get your point that we have calls to the function all over the place and would take some toll on the performance as well. As you said that we can probably call the function in git.c:run_builtin() or we can have a call to it in config.c:repo_config() so that just as the other settings, we will have our repo_settings parsed, which were once parsed through the same function(repo_config() or git_config()) and since all the cmd_*() functions have a call to this, we will also be able to call prepare_repo_settings() there itself. > > * 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? Well, I am not parsing it at that place. But, I am relying on an already existing call to prepre_repo_settings() before the function using the setting is called repository.c:repo_read_index(). I tried to narrow down to a cmd_foo() function so that I can shift a call to the prepare_repo_settings() from repo_read_index() to it, but this function is widely called and cannot be narrowed down so I had to settle with it. I'm afraid the issue still isn't completely 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. > I am sorry for that. I will keep that in mind from next time. > * 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). Yeah, so what I thought was that if it is a "core.foo" setting, I would club it with other core.* settings in the struct repo_settings. But other config settings like in the previous patch series, "extensions.preciousObjects" or also in this series, the "sparse.expectfilesoutsideofpatterns", I would put them in some local context or if they're tied to a repository, I would store them in the repository struct itself. But, as other "core.sparse_*" variables are stored in the repo_settings, I thought it was better to store the "sparse.expectfilesoutsideofpatterns" along with them rather than storing it in the repository. Thanks Ayush
