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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 8, 2024, 01:24 UTC
Message-ID: <xmqqjzdeqzzk.fsf@gitster.g>
In-Reply-To: <ZyzlBZnL-K3S7Env@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

Show 11 quoted lines

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

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.

Thanks.