On Thu, Jul 24, 2025 at 3:44 AM Junio C Hamano [off-list ref] wrote:

Ayush Chandekar [off-list ref] writes:

quoted

* 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.

quoted

* 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