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

[Kousik Sanagavarapu <five231003@gmail.com>]

On Wed, Oct 16, 2024 at 12:05:05AM +0800, shejialuo wrote:
> On Tue, Oct 15, 2024 at 08:01:22PM +0530, Kousik Sanagavarapu wrote:
> > Move "git_commit_encoding" and "git_log_output_encoding" to "struct
> > repository" and amend the functions associated with peeking at these
> > values so that now they take a "struct repository *" argument
> > accordingly.  While at it, rename the functions to repo_*() following
> > our usual convention.
> > 
> > Doing so removes the implicit dependency of these variables on
> > "the_repository", which is better because we now populate these
> > variables per repository.
> > 
> > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> > 
> > [...]
> > 
> > diff --git a/config.c b/config.c
> > index a11bb85da3..656748692d 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1690,13 +1690,15 @@ static int git_default_sparse_config(const char *var, const char *value)
> >  static int git_default_i18n_config(const char *var, const char *value)
> >  {
> >  	if (!strcmp(var, "i18n.commitencoding")) {
> > -		FREE_AND_NULL(git_commit_encoding);
> > -		return git_config_string(&git_commit_encoding, var, value);
> > +		FREE_AND_NULL(the_repository->git_commit_encoding);
> > +		return git_config_string(&the_repository->git_commit_encoding,
> > +					 var, value);
> >  	}
> >  
> >  	if (!strcmp(var, "i18n.logoutputencoding")) {
> > -		FREE_AND_NULL(git_log_output_encoding);
> > -		return git_config_string(&git_log_output_encoding, var, value);
> > +		FREE_AND_NULL(the_repository->git_log_output_encoding);
> > +		return git_config_string(&the_repository->git_log_output_encoding,
> > +					 var, value);
> >  	}
> 
> 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.

Makes sense.  Now that I've read this, it does indeed feel strange to
put these configs in "struct repository".

It also raises the question of how correct would it then be that these
configs and the configs similar to this are guarded by
USE_THE_REPOSITORY_VARIABLE macro - since they may not necessarily
depend on "the_repository" too - ie in cases when we are running outside
of any repo.