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@xxxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html