Re: includeIf breaks calling dashed externals

From: Jeff King <peff@peff.net>
Date: Apr 16, 2017, 04:50 UTC
Message-ID: <20170416045004.2ghhiv7jzgroejgw@sigill.intra.peff.net>
In-Reply-To: <20170415114901.GA5727@ash>

On Sat, Apr 15, 2017 at 06:49:01PM +0700, Duy Nguyen wrote:

Show 25 quoted lines

> > Probably this fixes it:
> > 
> > diff --git a/config.c b/config.c
> > index b6e4a57b9..8d66bdf56 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
> >  	struct strbuf pattern = STRBUF_INIT;
> >  	int ret = 0, prefix;
> >  
> > +	if (!have_git_dir())
> > +		return 0;
> > +
> >  	strbuf_add_absolute_path(&text, get_git_dir());
> >  	strbuf_add(&pattern, cond, cond_len);
> >  	prefix = prepare_include_condition_pattern(&pattern);
> > 
> > But it does raise a question of reading config before/after repository
> > setup, since those will give different answers. I guess they do anyway
> > because of $GIT_DIR/config.
> 
> This happens in execv_dased_external() -> check_pager_config() ->
> read_early_config(). We probably could use the same discover_git_directory
> trick to get .git dir (because we should find it). Maybe something
> like this instead?

I know that we only kick in discover_git_directory() in certain cases when we're reading config (for the "early config"). Would it makes sense to respect the same rules here?

I.e., if we have a program that wants to look at config but explicitly _doesn't_ setup the git repository, is it right to say that we are inside that repository?

So instead of lazily doing the discovery here:

Show 15 quoted lines

> diff --git a/config.c b/config.c
> index 1a4d85537b..4f540ae578 100644
> --- a/config.c
> +++ b/config.c
> @@ -212,8 +212,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
>  	struct strbuf text = STRBUF_INIT;
>  	struct strbuf pattern = STRBUF_INIT;
>  	int ret = 0, prefix;
> +	struct strbuf gitdir = STRBUF_INIT;
>  
> -	strbuf_add_absolute_path(&text, get_git_dir());
> +	if (have_git_dir())
> +		strbuf_addstr(&gitdir, get_git_dir());
> +	else if (!discover_git_directory(&gitdir))
> +		goto done;

..should we record the discovered git dir in read_early_config(), and use it only if available? And if not, then we know the program is explicitly trying not to be in a repo (or we know we tried to discover one already and it didn't exist).

-Peff