Re: [PATCH 1/7] builtin/init: stop modifying global `git_work_tree_cfg` variable

[Justin Tobler <jltobler@gmail.com>]

On 26/06/10 08:56AM, Patrick Steinhardt wrote:
> When executing git-init(1) we need to figure out the final location of
> the worktree. This location can be configured in a couple of ways: via
> an environment variable, via the preexisting "core.worktree" config in
> case we're reinitializing, or implicitly when reinitializing a non-bare
> repository.
> 
> When checking for the worktree location in "builtin/init-db.c" we
> populate any potentially-discovered value both by setting the global
> `git_work_tree_cfg` variable and via `set_git_work_tree()`, which
> ultimately ends up modifying `struct repository::worktree`.

Does `git_work_tree_cfg` have any bearing on the worktree value that
ends up in `struct repository`? Or is it just `set_git_work_tree()`?

> Modifying `git_work_tree_cfg` is unnecessary though: we configure the
> worktree in `create_default_files()`, and that function derives the
> worktree location via `repo_get_work_tree()`. Consequently, propagating
> the worktree via `set_git_work_tree()` is sufficient.

Ok, so IIUC there is really no reason to globally store the
`git_work_tree_cfg` value derived during `cmd_init_db()`. We only care
about the worktree written to `struct repository`.

> Stop munging `git_work_tree_cfg` and make it file-local to "setup.c" and
> function-local to `cmd_init_db()`.

Makes sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/init-db.c | 4 ++++
>  environment.c     | 3 ---
>  environment.h     | 1 -
>  setup.c           | 3 +++
>  4 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index c55517ad94..01bc27904e 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -229,6 +229,8 @@ int cmd_init_db(int argc,
>  
>  	if (!is_bare_repository_cfg) {
>  		const char *git_dir_parent = strrchr(git_dir, '/');
> +		char *git_work_tree_cfg = NULL;

We figure out the worktree locally and continue to set it via
`set_git_work_tree()` without updating a global worktree config value.

> +
>  		if (git_dir_parent) {
>  			char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
>  			git_work_tree_cfg = real_pathdup(rel, 1);
> @@ -243,6 +245,8 @@ int cmd_init_db(int argc,
>  		if (access(repo_get_work_tree(the_repository), X_OK))
>  			die_errno (_("Cannot access work tree '%s'"),
>  				   repo_get_work_tree(the_repository));
> +
> +		free(git_work_tree_cfg);
>  	}
>  	else {
>  		if (real_git_dir)
> diff --git a/environment.c b/environment.c
> index fc3ed8bb1c..4e86335f25 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -100,9 +100,6 @@ int auto_comment_line_char;
>  bool warn_on_auto_comment_char;
>  #endif /* !WITH_BREAKING_CHANGES */
>  
> -/* This is set by setup_git_directory_gently() and/or git_default_config() */
> -char *git_work_tree_cfg;
> -
>  /*
>   * Repository-local GIT_* environment variables; see environment.h for details.
>   */
> diff --git a/environment.h b/environment.h
> index ccfcf37bfb..5d6e4e6c1b 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -149,7 +149,6 @@ int have_git_dir(void);
>  
>  extern int is_bare_repository_cfg;
>  int is_bare_repository(void);
> -extern char *git_work_tree_cfg;
>  
>  /* Environment bits from configuration mechanism */
>  extern int trust_executable_bit;
> diff --git a/setup.c b/setup.c
> index b4652651df..52228b42a1 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -31,6 +31,9 @@ enum allowed_bare_repo {
>  	ALLOWED_BARE_REPO_ALL,
>  };
>  
> +/* This is set by setup_git_directory_gently() and/or git_default_config() */
> +static char *git_work_tree_cfg;

Ok, now `git_work_tree_cfg` is only referenced from "setup.c".

-Justin