Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

[Kousik Sanagavarapu <five231003@gmail.com>]

On Wed, Oct 16, 2024 at 08:15:37AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 16, 2024 at 12:05:05AM +0800, shejialuo wrote:
> > There are many builtins will execute this config setups by calling
> > "config.c::git_default_config" and then "git_default_i18n_config". If we
> > need to use "repo" pointer, we may need to wrap this pointer. (This is
> > not the problem and it is not hard).
> > 
> > But what if the "repo" pointer is NULL? We still need to set the value
> > of these environment variables. Because when using "git-mailinfo(1)"
> > outside of the repo, we still need to set "git_commit_encoding"
> > according to the user's config.
> > 
> > So, from this perspective, I don't think it's a good idea to put these
> > two configs into "struct repository". Because we can use these two
> > configs outside of the repo, if we put them into "struct repository", it
> > is strange.
> > 
> > However, I either don't know which way we would apply. So, I cannot give
> > accurate answer here.
> > 
> > ---
> > 
> > Patrick, I wanna ask you a question here. What's your envision here in
> > above situation. As you can see, if we put some configs into "struct
> > repository" and we run the builtins outside of the repo where we need to
> > set up configs, the "repo" is NULL. And we will get into trouble.
> > 
> > My idea is that if a config could be used outside of the repo, then we
> > should not put it into "struct repository".
> 
> I guess it would be nice to have a set of convenice functions in our
> config code that know to handle the case where the passed-in repository
> is `NULL`. If so, they'd only parse the global- and system-level config.
> If it is set, it would parse all three of them.
> 
> I also kind of agree that it should likely not be put into the `struct
> repository` in that case. I think where exactly to put things will
> always depend on the current usecase. I bet that in most cases, we
> should be able to get away with not storing the value anywhere global at
> all, which would be the best solution in my opinion:
> 
>   - It can either be stored on the stack if we don't have to pass it
>     around everywhere.
> 
>   - It can be passed around in a specific structure if we pass the value
>     within in a certain subsystem, only.
> 
>   - Or we can parse it on an as-needed basis if it happens deep down in
>     the calling stack when it's used essentially everwhere.
> 
> Now there will be situations where we used to store things globally as a
> caching mechanism, and not caching it may have performance impacts. But
> I guess that most cases do not fall into this category.

I like the idea of dynamically fetching the value of the config and not
caching it somewhere - although in this particular case, ie the
*_encoding configs I'm guessing it would be better that we cache these
configs.

Now the important question is where.  In point 2, you mention about
having a separate structure - do you mean for all those configs which
would not be a part of "struct repository"?  Of course in their
respective subsystems.

Coming to point 3, won't we still have to store it somewhere when we do
need it deep down the calling stack where it is used everywhere, since
we need to pass it around?

Thanks