Hi Peff,
ACK on the three patches, and I think your outline is valid (post
v2.23.0, possibly an Outreachy project?).
Thanks,
Dscho
P.S.: Sorry for top-posting, but I felt that I did not even respond to
anything you said, concretely, so...
On Tue, 6 Aug 2019, Jeff King wrote:
> On Tue, Aug 06, 2019 at 08:27:58AM -0400, Jeff King wrote:
>
> > diff --git a/config.c b/config.c
> > index 3900e4947b..cc637363bb 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -275,7 +275,7 @@ static int include_by_branch(const char *cond, size_t cond_len)
> > int flags;
> > int ret;
> > struct strbuf pattern = STRBUF_INIT;
> > - const char *refname = !the_repository || !the_repository->gitdir ?
> > + const char *refname = !the_repository->gitdir ?
> > NULL : resolve_ref_unsafe("HEAD", 0, NULL, &flags);
>
> I stopped here, even though I argued earlier that this probably ought to
> be have_git_dir() to match the rest of the config code. What I found
> when I started poking at it is that there are a few odd cases still in
> the startup code, and that IMHO the final form we want to end up with
> actually matches what's written here (but I'm not quite ready to sink
> the time into taking us there just yet). Read on for the gory details.
>
> The most immediate issue I saw was that the_repository->gitdir doesn't
> actually tell you whether or not we have a repository! I.e., that entry
> may be non-NULL even though startup_info->have_repository is 0.
>
> So these two tests fail:
>
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index d20b4d150d..7ec91c4c1c 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -348,6 +348,18 @@ test_expect_success 'conditional include, onbranch, implicit /** for /' '
> test_cmp expect actual
> '
>
> +test_expect_success 'onbranch include not fooled by fake GIT_DIR' '
> + mkdir not-a-git-dir &&
> + echo "ref: refs/heads/master" >not-a-git-dir/HEAD &&
> + git config --file=inner the.value inner &&
> + git config --file=outer the.value outer &&
> + git config --file=outer includeIf.onbranch:master.path inner &&
> + GIT_DIR=not-a-git-dir \
> + git config --file=outer --includes the.value >actual &&
> + echo outer >expect &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'include cycles are detected' '
> git init --bare cycle &&
> git -C cycle config include.path cycle &&
> diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
> index 3a0de0ddaa..91af2c2f89 100755
> --- a/t/t1309-early-config.sh
> +++ b/t/t1309-early-config.sh
> @@ -99,4 +99,12 @@ test_expect_success 'onbranch config outside of git repo' '
> nongit git help
> '
>
> +test_expect_success 'early config is not fooled by bogus GIT_DIR' '
> + mkdir not-a-git-dir &&
> + git config --file=not-a-git-dir/config \
> + alias.should-not-run \
> + "!echo should-not-run" &&
> + test_must_fail env GIT_DIR="not-a-git-dir" git should-not-run
> +'
> +
> test_done
>
> As you can see from that case, it's a curiosity that we still set the
> internal gitdir variable from $GIT_DIR, even if we found that it's not a
> valid repo.
>
> The onbranch case is fooled by our direct check of gitdir, but likewise
> have_git_dir() allows either have_repository or a non-NULL gitdir (as I
> mentioned earlier, I think this is vestigial at this point).
>
> So doing this fixes the onbranch case:
>
> diff --git a/config.c b/config.c
> index cc637363bb..134637f3ad 100644
> --- a/config.c
> +++ b/config.c
> @@ -275,7 +275,7 @@ static int include_by_branch(const char *cond, size_t cond_len)
> int flags;
> int ret;
> struct strbuf pattern = STRBUF_INIT;
> - const char *refname = !the_repository->gitdir ?
> + const char *refname = !have_git_dir() ?
> NULL : resolve_ref_unsafe("HEAD", 0, NULL, &flags);
> const char *shortname;
>
> diff --git a/environment.c b/environment.c
> index 89af47cb85..08bdf79d6b 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -203,8 +203,7 @@ int is_bare_repository(void)
>
> int have_git_dir(void)
> {
> - return startup_info->have_repository
> - || the_repository->gitdir;
> + return startup_info->have_repository;
> }
>
> const char *get_git_dir(void)
>
>
> but curiously it _doesn't_ fix the alias one, because after seeing that
> we have no configured git dir, we then fall back to discover_git_dir(),
> which happily reports back the same bogus $GIT_DIR.
>
> So I think there'd be some more cleanup required there. But the general
> direction is that in the near future we probably ought to be checking
> startup_info->have_repository and not the_repository->gitdir.
>
> But I think in the long-term, we probably ought to be getting rid of
> startup_info->have_repository itself. It's really just hiding a subtle
> dependency on whether the_repository is valid, and we're better off
> actually seeing all the spots that depend on the_repository (and
> eventually converting them to take a "struct repository" as
> appropriate).
>
> Where we want to end up is (I think):
>
> - stop setting the_repository->gitdir when we know that there's no
> repo (and possibly clear $GIT_DIR as well to avoid confusion). That
> should make it a robust way to check whether a "struct repository"
> is valid.
>
> This is the step I've stalled on, because I'm a bit worried that
> there's some subtle case that depends on it working this way.
>
> - drop startup_info->have_repository entirely. At that point it would
> be redundant with checking the_repository->gitdir.
>
> If you do this without the first step, t4201 will notice and fail
> (because it expects GIT_DIR=not-a-repo to notice that there is no
> repo).
>
> - optionally, drop have_git_dir() in favor of just checking
> the_repository->gitdir. This loses a layer of abstraction, but I
> think it makes it much more clear where we'd depending on
> the_repository.
>
> And so in the end, the current state of include_by_branch() is what we
> want, and all the other call sites should change to match it. Whether it
> should be changed in the interim (until we fix the discrepancy between
> gitdir/have_repository), I don't feel strongly about. There are
> user-visible cases that I believe we get wrong today, but as you can see
> from the tests above, they're pretty ridiculous and unlikely in
> practice.
>
> -Peff
>
