Re: [PATCH v2 1/4] repository.c: move env-related setup code back to environment.c

[Brandon Williams <bmwill@google.com>]

On 02/27, Nguyễn Thái Ngọc Duy wrote:
> It does not make sense that generic repository code contains handling
> of environment variables, which are specific for the main repository
> only. Refactor repo_set_gitdir() function to take $GIT_DIR and
> optionally _all_ other customizable paths. These optional paths can be
> NULL and will be calculated according to the default directory layout.
> 
> Note that some dead functions are left behind to reduce diff
> noise. They will be deleted in the next patch.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  cache.h       |  2 +-
>  environment.c | 29 +++++++++++++++++++++++++---
>  repository.c  | 53 +++++++++++++++++++++++++++++++++++++++++++++------
>  repository.h  | 11 ++++++++++-
>  setup.c       |  3 +--
>  5 files changed, 85 insertions(+), 13 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 5717399183..b164a407eb 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -459,7 +459,7 @@ static inline enum object_type object_type(unsigned int mode)
>   */
>  extern const char * const local_repo_env[];
>  
> -extern void setup_git_env(void);
> +extern void setup_git_env(const char *git_dir);
>  
>  /*
>   * Returns true iff we have a configured git repository (either via
> diff --git a/environment.c b/environment.c
> index ec10b062e6..74a2900ddf 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -14,6 +14,7 @@
>  #include "fmt-merge-msg.h"
>  #include "commit.h"
>  #include "object-store.h"
> +#include "argv-array.h"
>  
>  int trust_executable_bit = 1;
>  int trust_ctime = 1;
> @@ -148,10 +149,33 @@ static char *expand_namespace(const char *raw_namespace)
>  	return strbuf_detach(&buf, NULL);
>  }
>  
> -void setup_git_env(void)
> +/* Wrapper of getenv() that returns a strdup value. This value is kept
> + * in argv to be freed later.
> + */
> +static const char *getenv_safe(struct argv_array *argv, const char *name)
> +{
> +	const char *value = getenv(name);
> +
> +	if (!value)
> +		return NULL;
> +
> +	argv_array_push(argv, value);
> +	return argv->argv[argv->argc - 1];
> +}
> +
> +void setup_git_env(const char *git_dir)
>  {
>  	const char *shallow_file;
>  	const char *replace_ref_base;
> +	struct set_gitdir_args args = { NULL };
> +	struct argv_array to_free = ARGV_ARRAY_INIT;
> +
> +	args.shared_root = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT);
> +	args.object_dir = getenv_safe(&to_free, DB_ENVIRONMENT);
> +	args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
> +	args.index_file = getenv_safe(&to_free, INDEX_ENVIRONMENT);
> +	repo_set_gitdir(the_repository, git_dir, &args);
> +	argv_array_clear(&to_free);
>  
>  	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
>  		check_replace_refs = 0;
> @@ -301,8 +325,7 @@ int set_git_dir(const char *path)
>  {
>  	if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
>  		return error("Could not set GIT_DIR to '%s'", path);
> -	repo_set_gitdir(the_repository, path);
> -	setup_git_env();
> +	setup_git_env(path);
>  	return 0;
>  }
>  
> diff --git a/repository.c b/repository.c
> index a069b1b640..343efe7282 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -61,15 +61,55 @@ static void repo_setup_env(struct repository *repo)
>  					     "index", !repo->ignore_env);
>  }
>  
> -void repo_set_gitdir(struct repository *repo, const char *path)
> +static void expand_base_dir(char **out, const char *in,
> +			    const char *base_dir, const char *def_in)
>  {
> -	const char *gitfile = read_gitfile(path);
> -	char *old_gitdir = repo->gitdir;
> +	free(*out);
> +	if (in)
> +		*out = xstrdup(in);
> +	else
> +		*out = xstrfmt("%s/%s", base_dir, def_in);
> +}
> +
> +static void repo_set_commondir(struct repository *repo,
> +			       const char *shared_root)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	free(repo->commondir);
>  
> -	repo->gitdir = xstrdup(gitfile ? gitfile : path);
> -	repo_setup_env(repo);
> +	if (shared_root) {
> +		repo->different_commondir = 1;
> +		repo->commondir = xstrdup(shared_root);
> +		return;
> +	}
>  
> +	repo->different_commondir = get_common_dir_noenv(&sb, repo->gitdir);
> +	repo->commondir = strbuf_detach(&sb, NULL);
> +}
> +
> +void repo_set_gitdir(struct repository *repo,
> +		     const char *root,
> +		     const struct set_gitdir_args *o)
> +{
> +	const char *gitfile = read_gitfile(root);
> +	/*
> +	 * repo->gitdir is saved because the caller could pass "root"
> +	 * that also points to repo->gitdir. We want to keep it alive
> +	 * until after xstrdup(root). Then we can free it.
> +	 */
> +	char *old_gitdir = repo->gitdir;
> +
> +	repo->gitdir = xstrdup(gitfile ? gitfile : root);
>  	free(old_gitdir);
> +
> +	repo_set_commondir(repo, o->shared_root);
> +	expand_base_dir(&repo->objects.objectdir, o->object_dir,
> +			repo->commondir, "objects");
> +	expand_base_dir(&repo->graft_file, o->graft_file,
> +			repo->commondir, "info/grafts");
> +	expand_base_dir(&repo->index_file, o->index_file,
> +			repo->gitdir, "index");
>  }
>  
>  void repo_set_hash_algo(struct repository *repo, int hash_algo)
> @@ -87,6 +127,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
>  	int error = 0;
>  	char *abspath = NULL;
>  	const char *resolved_gitdir;
> +	struct set_gitdir_args args = { NULL };
>  
>  	abspath = real_pathdup(gitdir, 0);
>  	if (!abspath) {
> @@ -101,7 +142,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
>  		goto out;
>  	}
>  
> -	repo_set_gitdir(repo, resolved_gitdir);
> +	repo_set_gitdir(repo, resolved_gitdir, &args);
>  
>  out:
>  	free(abspath);
> diff --git a/repository.h b/repository.h
> index fa73ab8e93..b5b5d138aa 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -89,7 +89,16 @@ struct repository {
>  
>  extern struct repository *the_repository;
>  
> -extern void repo_set_gitdir(struct repository *repo, const char *path);
> +struct set_gitdir_args {
> +	const char *shared_root;

Can you add a comment explaining what shared_root is? From reading the
code it seems to be the common gitdir but its not clear from just
reading this.

> +	const char *object_dir;
> +	const char *graft_file;
> +	const char *index_file;
> +};
> +
> +extern void repo_set_gitdir(struct repository *repo,
> +			    const char *root,
> +			    const struct set_gitdir_args *optional);
>  extern void repo_set_worktree(struct repository *repo, const char *path);
>  extern void repo_set_hash_algo(struct repository *repo, int algo);
>  extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);
> diff --git a/setup.c b/setup.c
> index c5d55dcee4..6fac1bb58a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1116,8 +1116,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  			const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
>  			if (!gitdir)
>  				gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
> -			repo_set_gitdir(the_repository, gitdir);
> -			setup_git_env();
> +			setup_git_env(gitdir);
>  		}
>  		if (startup_info->have_repository)
>  			repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> -- 
> 2.16.1.435.g8f24da2e1a
> 

-- 
Brandon Williams