Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up

[Duy Nguyen <pclouds@gmail.com>]

On Sun, Apr 16, 2017 at 11:51:32AM -0400, Jeff King wrote:
> > diff --git a/cache.h b/cache.h
> > index e29a093839..27b7286f99 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -1884,6 +1884,8 @@ enum config_origin_type {
> >  
> >  struct config_options {
> >  	unsigned int respect_includes : 1;
> > +	unsigned int early_config : 1;
> > +	const char *git_dir; /* only valid when early_config is true */
> >  };
> 
> Why do we need both the flag and the string? Later, you do:
> 
> > -static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
> > +static int include_by_gitdir(const struct config_options *opts,
> > +			     const char *cond, size_t cond_len, int icase)
> >  {
> >  	struct strbuf text = STRBUF_INIT;
> >  	struct strbuf pattern = STRBUF_INIT;
> >  	int ret = 0, prefix;
> >  
> > -	strbuf_add_absolute_path(&text, get_git_dir());
> > +	if (!opts->early_config)
> > +		strbuf_add_absolute_path(&text, get_git_dir());
> > +	else if (opts->git_dir)
> > +		strbuf_add_absolute_path(&text, opts->git_dir);
> > +	else
> > +		goto done;
> 
> So we call get_git_dir() always when we're not in early config. Even if
> we don't have a git dir! Doesn't this mean that programs operating
> outside of a repo will still hit the BUG? I.e.:
> 
>   git config --global includeif.gitdir:/whatever.path foo
>   cd /not/a/git/dir
>   git diff --no-index foo bar
> 
> ?
> 
> I think instead the logic should be:
> 
>   if (opts->git_dir)
> 	strbuf_add_absolute_path(&text, opts->git_dir);
>   else if (have_git_dir())
> 	strbuf_add_absolute_path(&text, get_git_dir());
>   else
> 	goto done;

Do we care about the case when the override instruction is "we don't
have $GIT_DIR, act as if it does not exist, even though have_git_dir()
returns true"?

I'm guessing no, we won't run into that situation (and am inclined to
restructure the code as you suggested). Just throwing it out there if
I'm mistaken.
--
Duy