Re: [PATCH] git config: do not create .git/ if it does not exist yet

From: Jeff King <peff@peff.net>
Date: Feb 24, 2016, 08:26 UTC
Message-ID: <20160224082657.GD12511@sigill.intra.peff.net>
In-Reply-To: <c4027d758b0914dbc2e1ff5df344b0669aac4447.1456299545.git.johannes.schindelin@gmx.de>

On Wed, Feb 24, 2016 at 08:47:00AM +0100, Johannes Schindelin wrote:

Show 5 quoted lines

> It is a pilot error to call `git config section.key value` outside of
> any Git worktree.
> 
> Let's report that error instead of creating the .git/ directory and
> writing a fresh config into it.

Hmm.  I get (on my Linux machine):

  $ git config foo.bar baz
  error: could not lock config file .git/config: No such file or directory

which makes sense (though still isn't a great error message, and is kind of weird if you happen to have a .git directory that isn't a real repository).

Is Git more aggressive about auto-creating the directory for lockfiles on Windows? I tried the exact recipe you gave in the linked thread, just to be sure, but I couldn't replicate it.

> 	I cannot think of a way how to test this: all of the regression
> 	tests run inside Git's own worktree, and we cannot even assume
> 	that /tmp/ is outside of a worktree (or that it exists).

We could make the test conditional on whether we are in a repo. Anybody who builds from a tarball, or who uses --root would then run the test.

Something like this:

-- >8 --

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 52678e7..9856831 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1201,4 +1201,13 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+test_expect_success 'set up repo-less tests' '
+	mv .git tmp-git &&
+	{ git rev-parse --git-dir || test_set_prereq REPOLESS; }
+'
+
+test_expect_success REPOLESS 'cannot set repo config outside of a repo' '
+	test_must_fail git config foo.bar baz
+'
+
 test_done
-- 8< --

Though I admit it is pretty gross. It also pollutes the script state, so
anybody adding tests after will be in for a surprise. :)

One solution would be to restore "tmp-git" in the set-up, and then have
each REPOLESS test move it again, and put it back with a
test_when_finished. Or perhaps just make a new script for REPOLESS tests
(config or otherwise).

> diff --git a/builtin/config.c b/builtin/config.c
> index adc7727..78aab95 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -352,6 +352,9 @@ static int get_colorbool(const char *var, int print)
>  
>  static void check_write(void)
>  {
> +	if (!given_config_source.file && !startup_info->have_repository)
> +		die("not in a git directory");
> +
>  	if (given_config_source.use_stdin)
>  		die("writing to stdin is not supported");

I think you'd want to cover "git config --local foo.bar baz" in the same
way. You can check use_local_config for that.

-Peff