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

[shejialuo <shejialuo@gmail.com>]

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>
> ---
>  builtin/am.c       |  6 ++++--
>  builtin/blame.c    |  2 +-
>  builtin/commit.c   |  8 +++++---
>  builtin/log.c      |  3 ++-
>  builtin/mailinfo.c |  3 ++-
>  builtin/replay.c   |  3 ++-
>  builtin/rev-list.c |  2 +-
>  builtin/shortlog.c |  2 +-
>  bundle.c           |  2 +-
>  commit.c           |  9 ++++++---
>  config.c           | 10 ++++++----
>  environment.c      | 13 -------------
>  environment.h      |  6 ------
>  log-tree.c         |  4 ++--
>  pretty.c           |  2 +-
>  remote-curl.c      |  4 +++-
>  repository.c       | 13 +++++++++++++
>  repository.h       |  6 ++++++
>  revision.c         | 10 +++++-----
>  sequencer.c        | 17 +++++++++--------
>  submodule.c        |  2 +-
>  21 files changed, 71 insertions(+), 56 deletions(-)

For "git-mailinfo(1)" and "git-shortlog(1)", these two commands could
run outside of the repository. If we incorporate these two configs into
the "struct repository", we will have trouble when we remove the
"the_repository" global variable.

The patch in [1] will pass a NULL pointer for builtins with
"RUN_SETUP_GENTLY" flag.

[1] <d59b85b529865793c652d983d71a9fbb7e16b3e3.1728594828.git.gitgitgadget@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.

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

Thanks,
Jialuo