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

[Brandon Williams <bmwill@google.com>]

On 06/12, Jonathan Nieder wrote:
> Hi,
> 
> Brandon Williams wrote:
> 
> > 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
> 
> > --- 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)".

Will do.

> 
> [...]
> > --- 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.

That kind of defeats the purpose of the patch.  The point is to not rely
on global repository state and with the BUG statement we still are
relying on global state.

And I don't know if I agree with renaming a function just to rename it.

> 
> > @@ -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.
> 
> > @@ -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()?

Needs to be commondir since the config is stored in the common git
directory and not a per worktree git directory.

-- 
Brandon Williams