Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope
- From
shejialuo <shejialuo@gmail.com>
- Date
- Oct 15, 2024, 16:05 UTC
- Message-ID
- <Zw6SsUyZ0oA0XqMK@ArchLinux>
- In-Reply-To
- <20241015144935.4059-2-five231003@gmail.com>
On Tue, Oct 15, 2024 at 08:01:22PM +0530, Kousik Sanagavarapu wrote:
Show 34 quoted lines
> 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>
Show 22 quoted lines
> 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