git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Jeff Hostetler <git@jeffhostetler.com>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 3/3] config: stop checking whether the_repository is NULL
Date: Tue, 6 Aug 2019 08:49:55 -0400	[thread overview]
Message-ID: <20190806124954.GA13649@sigill.intra.peff.net> (raw)
In-Reply-To: <20190806122758.GC13513@sigill.intra.peff.net>

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

  reply	other threads:[~2019-08-06 12:49 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-31 19:53 [PATCH 0/1] Make the includeif:onbranch feature more robust Johannes Schindelin via GitGitGadget
2019-07-31 19:53 ` [PATCH 1/1] config: work around bug with includeif:onbranch and early config Johannes Schindelin via GitGitGadget
2019-07-31 21:37   ` Junio C Hamano
2019-07-31 20:06 ` [PATCH v2 0/1] Make the includeif:onbranch feature more robust Johannes Schindelin via GitGitGadget
2019-07-31 20:06   ` [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config Johannes Schindelin via GitGitGadget
2019-07-31 22:02     ` Jeff King
2019-07-31 22:13       ` Johannes Schindelin
2019-07-31 23:12         ` Jeff King
2019-08-01  0:49           ` Jeff King
2019-08-01 17:24             ` Jeff Hostetler
2019-08-06 12:26               ` [PATCH 0/3] the_repository initialization cleanup Jeff King
2019-08-06 12:26                 ` [PATCH 1/3] t1309: use short branch name in includeIf.onbranch test Jeff King
2019-08-06 12:27                 ` [PATCH 2/3] common-main: delay trace2 initialization Jeff King
2019-08-06 12:27                 ` [PATCH 3/3] config: stop checking whether the_repository is NULL Jeff King
2019-08-06 12:49                   ` Jeff King [this message]
2019-08-08 19:48                     ` Johannes Schindelin
2019-08-06 12:56               ` [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190806124954.GA13649@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).