Re: [PATCH 5/7] environment: split up concerns of `is_bare_repository_cfg`

[Justin Tobler <jltobler@gmail.com>]

On 26/06/10 08:56AM, Patrick Steinhardt wrote:
> The `is_bare_repository_cfg` variable tracks two different pieces of
> information:
> 
>   - It tracks whether the user has invoked git with the "--bare" flag,
>     which makes us treat any discovered Git repository as if it was a
>     bare repository.
> 
>   - Otherwise it tracks whether the discovered `the_repository` is bare.
> 
> This makes the flag extremely confusing and creates a bit of a challenge
> when handling multiple repositories in the same process.

Indeed, this does seem rather confusing.

> Split up the concerns of this variable into two pieces:
> 
>   - `startup_info.force_bare_repository` tracks whether the user has
>     passed the "--bare" flag. This is used as a hint to treat newly set
>     up repositories as bare regardless of whether or not they have a
>     worktree.
> 
>   - `struct repository::bare_cfg` tracks whether or not a repository is
>     considered bare. This takes into account both whether the user has
>     passed "--bare" and the discovered state of the repository itself.
> 
> Whether or not a repository is bare is now resolved when checking the
> repository's format, and is then later applied to the repository itself
> via `apply_repository_format()`.

Nice. These seem like they should be tracked separately, so splitting
them up sounds like a good idea.

> This enables a subsequent change where we make `is_bare_repository()`
> not depend on global state anymore.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/init-db.c |  2 +-
>  environment.c     |  5 ++---
>  environment.h     |  1 -
>  git.c             |  2 +-
>  repository.c      |  1 +
>  repository.h      |  7 +++++++
>  setup.c           | 21 ++++++++++++++-------
>  setup.h           |  6 ++++++
>  worktree.c        |  2 +-
>  9 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 52aa92fb0a..566732c9f4 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -81,7 +81,7 @@ int cmd_init_db(int argc,
>  	const char *template_dir = NULL;
>  	char *template_dir_to_free = NULL;
>  	unsigned int flags = 0;
> -	int bare = is_bare_repository_cfg;
> +	int bare = startup_info->force_bare_repository ? 1 : -1;

Any particular reason to continue mapping `force_bare_repository=false`
to -1? Or was this to just minimize changes?

>  	const char *object_format = NULL;
>  	const char *ref_format = NULL;
>  	const char *initial_branch = NULL;
[snip]
> diff --git a/repository.h b/repository.h
> index 36e2db2633..7d649e32e7 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -117,6 +117,13 @@ struct repository {
>  	bool worktree_initialized;
>  	bool worktree_config_is_bogus;
>  
> +	/*
> +	 * Whether the repository is bare, as set by "core.bare" config or
> +	 * inferred during repository discovery. -1 means unset/unknown, 0
> +	 * means non-bare, 1 means bare.
> +	 */
> +	int bare_cfg;

Now we track whether a repository is bare in `struct repository` and
removes the need to a separate global to track this state.

> +
>  	/*
>  	 * Path from the root of the top-level superproject down to this
>  	 * repository.  This is only non-NULL if the repository is initialized
> diff --git a/setup.c b/setup.c
> index 71fc6b33da..2b690da8ca 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -795,10 +795,16 @@ static int check_repository_format_gently(const char *gitdir,
>  		has_common = 0;
>  	}
>  
> -	if (!has_common) {
> -		if (candidate->is_bare != -1)
> -			is_bare_repository_cfg = candidate->is_bare;
> -	} else {
> +	if (startup_info->force_bare_repository) {
> +		candidate->is_bare = 1;
> +		FREE_AND_NULL(candidate->work_tree);
> +	} else if (has_common) {
> +		/*
> +		 * When sharing a common dir with another repository (e.g. a
> +		 * linked worktree), do not let this repository's config
> +		 * dictate bareness; it is inherited from the main worktree.
> +		 */
> +		candidate->is_bare = -1;
>  		FREE_AND_NULL(candidate->work_tree);

Previously, when there was a common dir, `candidate->work_tree` was left
untouched, but now we are expclicitly setting it. I'm not sure I fully
understand this change.

-Justin