Recovering from 'fatal: core.bare and core.worktree do not make sense'

Recovering from 'fatal: core.bare and core.worktree do not make sense'

[SZEDER Gábor]

Hi,

the other day I said 'git config core.worktree /somewhere' in a bare
repo while thinking I was in a regular one, user error.  The 'fatal:
core.bare and core.worktree do not make sense' error from the next
command made me realize immediately that I was wrong, that's good.
However...

OK, let's have a look and recover from the situation:

    $ git config --edit
    fatal: core.bare and core.worktree do not make sense

Well, all was well before I set 'core.worktree', so let's unset it:

    $ git config --unset core.worktree
    fatal: core.bare and core.worktree do not make sense

Hmph, not expecting much, but how about unsetting the other
variable?

    $ git config --unset core.bare
    fatal: core.bare and core.worktree do not make sense

Good, at least it's pretty consistent, though I still don't get what
'git config' has to do with the worktree that is so important that
it has to bail out.  Time to look for help:

    $ git help config
    fatal: core.bare and core.worktree do not make sense

WTF :)
Alright, I give up:

    $ vim config
    $ # happy

It was two days later that I had a bit of a lightbulb moment,
reproduced the situation and just for fun tried this:

    $ git -c core.bare=false config --unset core.bare

I didn't expect, but it worked!  Great.

Some thoughts:

    1) Perhaps 'git config' should be more careful in the first place
       and refuse to set 'core.worktree' when 'core.bare' is already
       true and vice versa.

    2) The damage was done with 'git config', so I expected that I can
       repair it with "plain" 'git config' (i.e. without 'git -c') as
       well.  'git config' has nothing to do with the path to the
       worktree after all.  And 'git config --edit' should work
       regardless of the mess that might be in the config file.

    3) 'git help <cmd>' should always work, shouldn't it?  (Though
       that's the easiest to remedy, just cd out of the repo, or fire
       up a new terminal window.)


Gábor

[PATCH] setup_git_directory: delay core.bare/core.worktree errors

[Jeff King]

On Wed, May 27, 2015 at 12:31:47PM +0200, SZEDER Gábor wrote:

> Some thoughts:
> 
>    1) Perhaps 'git config' should be more careful in the first place
>       and refuse to set 'core.worktree' when 'core.bare' is already
>       true and vice versa.

Right now git-config doesn't know anything about the meaning of the
config variables; that logic is spread throughout the code, at the
actual users of the variables. So I'm a little hesitant to special-case
this one problem (especially because I think we can fix this in a
different way).

>    2) The damage was done with 'git config', so I expected that I can
>       repair it with "plain" 'git config' (i.e. without 'git -c') as
>       well.  'git config' has nothing to do with the path to the
>       worktree after all.  And 'git config --edit' should work
>       regardless of the mess that might be in the config file.

The problem is that setup_git_directory is overly eager to die in this
case (even when we use the "gentle" form to indicate we can handle being
outside of a repo). Patch is below.

>    3) 'git help <cmd>' should always work, shouldn't it?  (Though
>       that's the easiest to remedy, just cd out of the repo, or fire
>       up a new terminal window.)

It looks in the repo (gently) so that it can find repo-specific config
like help.format. And thus gets caught in the same problem as
git-config.

-- >8 --
Subject: setup_git_directory: delay core.bare/core.worktree errors

If both core.bare and core.worktree are set, we complain
about the bogus config and die. Dying is good, because it
avoids commands running and doing damage in a potentially
incorrect setup. But dying _there_ is bad, because it means
that commands which do not even care about the work tree
cannot run. This can make repairing the situation harder:

  [setup]
  $ git config core.bare true
  $ git config core.worktree /some/path

  [OK, expected.]
  $ git status
  fatal: core.bare and core.worktree do not make sense

  [Hrm...]
  $ git config --unset core.worktree
  fatal: core.bare and core.worktree do not make sense

  [Nope...]
  $ git config --edit
  fatal: core.bare and core.worktree do not make sense

  [Gaaah.]
  $ git help config
  fatal: core.bare and core.worktree do not make sense

Instead, let's issue a warning about the bogus config when
we notice it (i.e., for all commands), but only die when the
command tries to use the work tree (by calling setup_work_tree).
So we now get:

  $ git status
  warning: core.bare and core.worktree do not make sense
  fatal: unable to set up work tree using invalid config

  $ git config --unset core.worktree
  warning: core.bare and core.worktree do not make sense

We have to update t1510 to accomodate this; it uses
symbolic-ref to check whether the configuration works or
not, but of course that command does not use the working
tree. Instead, we switch it to use `git status`, as it
requires a work-tree, does not need any special setup, and
is read-only (so a failure will not adversely affect further
tests).

In addition, we add a new test that checks the desired
behavior (i.e., that running "git config" with the bogus
config does in fact work).

Reported-by: SZEDER Gábor <szeder@ira.uka.de>
Signed-off-by: Jeff King <peff@peff.net>
---
The message for "git status" is a little clunky (first we warn, then we
say "wait, that warning was important!"). I also considered dropping the
warning (so you'd see nothing in the "git config" case) and then just
moving the die() to setup_work_tree().

 setup.c               | 12 ++++++++++--
 t/t1510-repo-setup.sh | 24 ++++++++++++++++--------
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/setup.c b/setup.c
index 863ddfd..82c0cc2 100644
--- a/setup.c
+++ b/setup.c
@@ -4,6 +4,7 @@
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
+static int work_tree_config_is_bogus;
 
 /*
  * The input parameter must contain an absolute path, and it must already be
@@ -327,6 +328,10 @@ void setup_work_tree(void)
 
 	if (initialized)
 		return;
+
+	if (work_tree_config_is_bogus)
+		die("unable to set up work tree using invalid config");
+
 	work_tree = get_git_work_tree();
 	git_dir = get_git_dir();
 	if (!is_absolute_path(git_dir))
@@ -495,8 +500,11 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	if (work_tree_env)
 		set_git_work_tree(work_tree_env);
 	else if (is_bare_repository_cfg > 0) {
-		if (git_work_tree_cfg) /* #22.2, #30 */
-			die("core.bare and core.worktree do not make sense");
+		if (git_work_tree_cfg) {
+			/* #22.2, #30 */
+			warning("core.bare and core.worktree do not make sense");
+			work_tree_config_is_bogus = 1;
+		}
 
 		/* #18, #26 */
 		set_git_dir(gitdirenv);
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 33c1a58..13ae12d 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -599,11 +599,20 @@ test_expect_success '#20b/c: core.worktree and core.bare conflict' '
 	mkdir -p 20b/.git/wt/sub &&
 	(
 		cd 20b/.git &&
-		test_must_fail git symbolic-ref HEAD >/dev/null
+		test_must_fail git status >/dev/null
 	) 2>message &&
 	grep "core.bare and core.worktree" message
 '
 
+test_expect_success '#20d: core.worktree and core.bare OK when working tree not needed' '
+	setup_repo 20d non-existent "" true &&
+	mkdir -p 20d/.git/wt/sub &&
+	(
+		cd 20d/.git &&
+		git config foo.bar value
+	)
+'
+
 # Case #21: core.worktree/GIT_WORK_TREE overrides core.bare' '
 test_expect_success '#21: setup, core.worktree warns before overriding core.bare' '
 	setup_repo 21 non-existent "" unset &&
@@ -612,7 +621,7 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare
 		cd 21/.git &&
 		GIT_WORK_TREE="$here/21" &&
 		export GIT_WORK_TREE &&
-		git symbolic-ref HEAD >/dev/null
+		git status >/dev/null
 	) 2>message &&
 	! test -s message
 
@@ -701,13 +710,13 @@ test_expect_success '#22.2: core.worktree and core.bare conflict' '
 		cd 22/.git &&
 		GIT_DIR=. &&
 		export GIT_DIR &&
-		test_must_fail git symbolic-ref HEAD 2>result
+		test_must_fail git status 2>result
 	) &&
 	(
 		cd 22 &&
 		GIT_DIR=.git &&
 		export GIT_DIR &&
-		test_must_fail git symbolic-ref HEAD 2>result
+		test_must_fail git status 2>result
 	) &&
 	grep "core.bare and core.worktree" 22/.git/result &&
 	grep "core.bare and core.worktree" 22/result
@@ -753,9 +762,8 @@ test_expect_success '#28: core.worktree and core.bare conflict (gitfile case)' '
 	setup_repo 28 "$here/28" gitfile true &&
 	(
 		cd 28 &&
-		test_must_fail git symbolic-ref HEAD
+		test_must_fail git status
 	) 2>message &&
-	! grep "^warning:" message &&
 	grep "core.bare and core.worktree" message
 '
 
@@ -767,7 +775,7 @@ test_expect_success '#29: setup' '
 		cd 29 &&
 		GIT_WORK_TREE="$here/29" &&
 		export GIT_WORK_TREE &&
-		git symbolic-ref HEAD >/dev/null
+		git status
 	) 2>message &&
 	! test -s message
 '
@@ -778,7 +786,7 @@ test_expect_success '#30: core.worktree and core.bare conflict (gitfile version)
 	setup_repo 30 "$here/30" gitfile true &&
 	(
 		cd 30 &&
-		test_must_fail env GIT_DIR=.git git symbolic-ref HEAD 2>result
+		test_must_fail env GIT_DIR=.git git status 2>result
 	) &&
 	grep "core.bare and core.worktree" 30/result
 '
-- 
2.4.2.668.gc3b1ade.dirty