Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable

From: shejialuo <shejialuo@gmail.com>
Date: Nov 16, 2024, 12:09 UTC
Message-ID: <ZziLlXln3NLPaHc-@ArchLinux>
In-Reply-To: <xmqqjzdeqzzk.fsf@gitster.g>

On Fri, Nov 08, 2024 at 10:24:31AM +0900, Junio C Hamano wrote:

Show 27 quoted lines

> shejialuo <shejialuo@gmail.com> writes:
> 
> > I also want to ask this question. Actually, I feel quite strange about
> > why we need to add a new parameter `is_bare` to `repo_init` function.
> >
> > For this call:
> >
> >     repo_init(the_repository, git_dir, work_tree, -1);
> >
> > We add a new field "is_bare_cfg" to the "struct repository". So, at now,
> > `the_repository` variable should contain the information about whether
> > the repo is bare(1), is not bare(0) or unknown(-1). However, in this
> > call, we pass "-1" to the parameter `is_bare` for "repo_init" function.
> 
> Isn't this merely trying to be faithful to the original to avoid
> unintended behaviour change?  We initialize the global variable
> is_bare_repository_cfg to unspecified(-1) in the original, and
> for a rewrite to move the global to a member in the singleton
> instance of the_repo, it would need to be able to do the same.
> 
> And for callers of repo_init() that prepares _another_ in-core
> repository instance, which is different from the_repository, because
> the original has a process-wide singleton global variable, copying
> the value from the_repository->is_bare to a newly initialized one
> would hopefully give us the most faithful rewrite to avoid
> unintended behaviour change.
>

Yes, I agree that this is the most faithful way to make sure the consistency when we want to create a new `repo` instead of letting the caller do this itself.

So, I think what I feel strange is that we need to do this assignment. Because we make a global variable not global by incorporating this into "struct repository *", we have to maintain this state whenever we create a new "repo".

It lets me think whether we should place "is_bare_cfg" into "struct repository" in the first place. I will explain why in the later comments.

Show 14 quoted lines

> At least, that is how I understood why the patch does it this way.
> As you noticed, too, there are ...
> 
> > When I first look at this code, I have thought that we will set
> > "repo->is_bare_cfg = -1" to indicate that we cannot tell whether the
> > repo is bare or not. But it just sets the "repo->is_bare_cfg = is_bare"
> > if `bare > 0`. Junio has already commented on this.
> 
> ... places in the updated code that makes it unclear what the
> is_bare member really means.  The corresponding global variable used
> to be "this is what we were told by config or env or command line",
> but it is unclear, with conditional assignments like the above, what
> it means in the updated code.
>

Yes, John has changed the corresponding code paths by setting the global variable "the_repository->is_bare_cfg". So, we will refactor this later.

In the previous days, Kousik wanted to make "builtin/mailinfo" not to reply on "the_repository". I have commented in

    https://lore.kernel.org/git/Zw6SsUyZ0oA0XqMK@ArchLinux/

In this thread, I do not agree that we should not incorporate the global variables in "git_commit_encoding" and "git_log_output_encoding" in "environment.c" into "struct repository *" because we could use these two configs outside of the repo.

So, I don't think it's a good idea to put into "is_bare_cfg" into "struct repository". Put it further more, we should not put the global variables in "struct repository" structure for the following reasons:

  1. These variables are used across the whole lifecycle. Not just only
     related to the repository. Some variables could be used outside of
     the repo.
  2. Currently, the config functions which set up these variables don't
     have parameters to access the "struct repository *". Of course, we
     could add the parameter, but as 1 shows, some variables could be
     used outside of the repo. We may need many efforts for such
     situation.
  3. We need to maintain the consistency if we create a new "struct
     repository", because we will make global variables not global.

So, in my perspective, we may just create a new structure called "struct env" to incorporate all these variables in "environment.c" just like what we have done for "struct repository *". But we also introduced another overhead, we may pass this structure to every function when setting up.

> Thanks.

Thanks, Jialuo