Re: [PATCH 4/4] config: don't implicitly use gitdir

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 13, 2017, 01:05 UTC
Message-ID: <20170613010518.GB133952@aiede.mtv.corp.google.com>
In-Reply-To: <20170612213406.83247-5-bmwill@google.com>

Hi,

Brandon Williams wrote:

Show 12 quoted lines

> Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
> not set up) added a 'git_dir' field to the config_options struct.  Let's
> use this option field explicitly all the time instead of occasionally
> falling back to calling 'git_pathdup("config")' to get the path to the
> local repository configuration.  This allows 'do_git_config_sequence()'
> to not implicitly rely on global repository state.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  builtin/config.c | 2 ++
>  config.c         | 6 ++----
>  2 files changed, 4 insertions(+), 4 deletions(-)

Unlike the previous 3, this one makes me pause for a moment: it means that "gitdir:" includes and .git/config discovery would stop working if the caller does not remember to set git_dir in their config_options.

So we have to inspect callers.

Callers that set respect_includes = 1:

- read_early_config carefully sets git_dir *phew*
- git_config_raw doesn't and is used approximately everywhere.

do_git_config_sequence call chain:
- called by git_config_with_options, which is called by
  - read_early_config
  - git_config_raw
  - various callers in builtin/config.c, using &config_options

Show 8 quoted lines

> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -539,6 +539,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		config_options.respect_includes = !given_config_source.file;
>  	else
>  		config_options.respect_includes = respect_includes_opt;
> +	if (have_git_dir())
> +		config_options.git_dir = get_git_common_dir();

nit: because of the context, this 'if' can be "if (!nongit)".

[...]

Show 10 quoted lines

> --- a/config.c
> +++ b/config.c
> @@ -219,8 +219,6 @@ static int include_by_gitdir(const struct config_options *opts,
>  
>  	if (opts->git_dir)
>  		git_dir = opts->git_dir;
> -	else if (have_git_dir())
> -		git_dir = get_git_dir();
>  	else
>  		goto done;

I wonder if this should have a sanity-check:

	else if (have_git_dir())
		BUG("caller forgot to set opts->git_dir");

Alternatively, could this patch rename git_config_with_options? That way any other patch in flight that calls git_config_with_options would conflict with this patch, giving us an opportunity to make sure it also sets git_dir. As another nice side benefit it would make it easy for someone reading the patch to verify it didn't miss any callers.

Show 8 quoted lines

> @@ -1548,8 +1546,6 @@ static int do_git_config_sequence(const struct config_options *opts,
>  
>  	if (opts->git_dir)
>  		repo_config = mkpathdup("%s/config", opts->git_dir);
> -	else if (have_git_dir())
> -		repo_config = git_pathdup("config");
>  	else
>  		repo_config = NULL;

Likewise: either this should get a sanity check

	else if (have_git_dir())
		BUG("caller forgot to set opts->git_dir");

or the public interface git_config_with_options should be renamed.

Show 6 quoted lines

> @@ -1613,6 +1609,8 @@ static void git_config_raw(config_fn_t fn, void *data)
>  	struct config_options opts = {0};
>  
>  	opts.respect_includes = 1;
> +	if (have_git_dir())
> +		opts.git_dir = get_git_common_dir();

curious: Why get_git_common_dir() instead of get_git_dir()?

Thanks, Jonathan