"John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: John Cai <jcai@xxxxxxxxxx>
>
> A subsequent commit will BUG() when the is_bare_cfg member is
> uninitialized. Since there are still some codepaths that initializing the
> is_bare_cfg variable, initialize them.
>
> Signed-off-by: John Cai <johncai86@xxxxxxxxx>
> ---
> setup.c | 7 +++++++
> 1 file changed, 7 insertions(+)
I am not sure about the wisdom of this step (and the next one).
Before this step, it used to be that the global variable (or
the_repository->is_bare_cfg) can be inspected to see if there is an
explicit "set to be treated as", or nobody told us if the repository
ought to be bare (or not). With this and the next step, that is no
longer possible, yet we still do the "core.bare says it is either
true or unconfigured, so we ask repo_get_work_tree() and it returns
NULL, so it is bare", which feels awfully inconsistent. Especially
the change from the next patch
> diff --git a/repository.c b/repository.c
> index 96608058b61..cd1d59ea1b9 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -464,5 +464,7 @@ int repo_hold_locked_index(struct repository *repo,
> int repo_is_bare(struct repository *repo)
> {
> /* if core.bare is not 'false', let's see if there is a work tree */
> + if (repo->is_bare_cfg < 0 )
> + BUG("is_bare_cfg unspecified");
> return repo->is_bare_cfg && !repo_get_work_tree(repo);
> }
the returned value does not make much sense anymore. One half of it
used to be "if not configured, we can ask if there is worktree and
the lack of one by definition means we are bare", which made perfect
sense, but now what remains is "the configuration says it is, but
when we ask if there is a worktree, there is, so it is not bare
after all", which is somewhat dubious.
And if the goal of steps 2 & 3 is to redefine what is_bare_cfg means
and make it "this is the only thing we need to check if the
repository is bare" (which by itself is not a bad thing), shouldn't
the checking of worktree be done where the code assigns true to the
repo->is_bare_cfg, no?
> diff --git a/setup.c b/setup.c
> index 6bc4aef3a8b..5680976c598 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -741,6 +741,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
>
> if (verify_repository_format(candidate, &err) < 0) {
> if (nongit_ok) {
> + the_repository->is_bare_cfg = 1;
It is unclear how we can be certain that we are looking at a bare
repository in this case. We do not even understand the repository
format, GIT_DIR we were given to decide which file called "config"
may not even be a repository. We are losing a bit of information
(i.e. nobody has told us if we ought to treat the repository as a
bare one, or a non-bare one") by overriding the value here.
> @@ -1017,6 +1018,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
> if (nongit_ok) {
> *nongit_ok = 1;
> free(gitfile);
> + the_repository->is_bare_cfg = 0;
> return NULL;
Ditto.
> @@ -1069,6 +1071,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
>
> /* set_git_work_tree() must have been called by now */
> worktree = repo_get_work_tree(the_repository);
> + the_repository->is_bare_cfg = 0;
What if worktree is NULL? Wouldn't it be more meaningful to say
is_bare_cfg is true in such a case?
> @@ -1125,6 +1128,9 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>
> /* #0, #1, #5, #8, #9, #12, #13 */
> set_git_work_tree(".");
> +
> + if (the_repository->is_bare_cfg < 0)
> + the_repository->is_bare_cfg = 0;
OK. We did discovery, is_bare_cfg did not say true (it would have
returned before we got here if is_bare_cfg were set to true). We
decided to treat the current directory as the top of the working tree,
so by definition, we are not treating the repository as bare.
But this makes me wonder what should happen
the_repository->is_bare_cfg is already set to true. Shouldn't that
be a BUG()?
> @@ -1767,6 +1773,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
> die(_("not a git repository (or any of the parent directories): %s"),
> DEFAULT_GIT_DIR_ENVIRONMENT);
> *nongit_ok = 1;
> + the_repository->is_bare_cfg = 1;
This is not bare nor non-bare---simply we did not find any usable
git repository, and we lose the single bit of information "nobody
told us to treat the repository as bare or non-bare".
Not that the loss of information is a huge deal. But having to make
an arbitrary choice like the above (and similar ones in previous
hunks where we didn't have any repository to begin with) is an
indication that the entire "is_bare_cfg must mean if our repository
is bare or non-bare" premise patch 3/3 wants to enforce may be
misguided, I am afraid.
> break;
> case GIT_DIR_HIT_MOUNT_POINT:
> if (!nongit_ok)
