Re: [PATCH 02/13] setup: mark bogus worktree in `apply_repository_format()`

[Justin Tobler <jltobler@gmail.com>]

On 26/06/30 01:47PM, Patrick Steinhardt wrote:
> When a repository is configured to have both "core.worktree" and
> "core.bare" we emit a warning and mark the worktree configuration as
> bogus so that the next call to `setup_work_tree()` will cause us to die.
> This allows us to still use the misconfigured repository, at least as
> long as we don't try to use its worktree.

Ok.

> This condition is handled in `setup_explicit_git_dir()`. In a subsequent
> commit we'll refactor this function so that it doesn't receive a repo as
> input anymore though, and consequently we cannot set the "bogus" bit
> anymore.

Ok IIUC, `setup_explicit_git_dir()` is currently responsible for
checking if both "core.worktree" and "core.bare" are set. 

> Move the logic into `apply_repository_format()` instead to prepare for
> this. While at it, fix up formatting a bit.

So `apply_repository_format()` is expected to still have the repository
info which has access to the "bogus" field.

> Note that this change requires us to also explicitly unset the value of
> "core.worktree" in case we have the "GIT_WORK_TREE" environment variable
> set. This is because the environment variable overrides the repository's
> configuration, and we don't want to warn or die in case the work tree
> has been configured explicitly regardless of whether or not "core.bare"
> is set.

Hmmm, does this mean we now just silently ignore the misconfiguration if
done via environment variable?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  setup.c | 37 +++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 118416e350..f54eac5e5a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1147,24 +1147,24 @@ static const char *setup_explicit_git_dir(struct repository *repo,
>  	}
>  
>  	/* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */
> -	if (work_tree_env)
> +	if (work_tree_env) {
> +		/*
> +		 * The environment variable overrides "core.worktree". This
> +		 * also has the consequence that we don't want to flag cases as
> +		 * bogus where we have both "core.worktree" and "core.bare", so
> +		 * we have to exlicitly unset the configuration.
> +		 */
> +		FREE_AND_NULL(repo_fmt->work_tree);

Ok, this confused me a bit a first, but IIUC we have to unset the
environment variable because we now defer setting the bogus flag to a
later point when `apply_repository_format()` is executed.

>  		set_git_work_tree(repo, work_tree_env);
> -	else if (repo_fmt->is_bare > 0) {
> -		if (repo_fmt->work_tree) {
> -			/* #22.2, #30 */
> -			warning("core.bare and core.worktree do not make sense");
> -			repo->worktree_config_is_bogus = true;
> -		}
> -
> +	} else if (repo_fmt->is_bare > 0) {
>  		/* #18, #26 */
>  		set_git_dir(repo, gitdirenv, 0);
>  		free(gitfile);
>  		return NULL;
> -	}
> -	else if (repo_fmt->work_tree) { /* #6, #14 */
> -		if (is_absolute_path(repo_fmt->work_tree))
> +	} else if (repo_fmt->work_tree) { /* #6, #14 */
> +		if (is_absolute_path(repo_fmt->work_tree)) {
>  			set_git_work_tree(repo, repo_fmt->work_tree);
> -		else {
> +		} else {
>  			char *core_worktree;
>  			if (chdir(gitdirenv))
>  				die_errno(_("cannot chdir to '%s'"), gitdirenv);
> @@ -1176,15 +1176,14 @@ static const char *setup_explicit_git_dir(struct repository *repo,
>  			set_git_work_tree(repo, core_worktree);
>  			free(core_worktree);
>  		}
> -	}
> -	else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
> +	} else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
>  		/* #16d */
>  		set_git_dir(repo, gitdirenv, 0);
>  		free(gitfile);
>  		return NULL;
> -	}
> -	else /* #2, #10 */
> +	} else { /* #2, #10 */
>  		set_git_work_tree(repo, ".");
> +	}

Some random curly brace cleanup above.

>  
>  	/* set_git_work_tree() must have been called by now */
>  	worktree = repo_get_work_tree(repo);
> @@ -1768,6 +1767,12 @@ int apply_repository_format(struct repository *repo,
>  	if (verify_repository_format(format, err) < 0)
>  		return -1;
>  
> +	if (format->is_bare > 0 && format->work_tree) {
> +		/* #22.2, #30 */
> +		warning("core.bare and core.worktree do not make sense");
> +		repo->worktree_config_is_bogus = true;
> +	}

We now perform this validation in `apply_repository_format()`. Does
deferring this check have any meaningful impact? Or is
`apply_repository_format()` always called after
`setup_explicit_git_dir()`?

-Justin