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
