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

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

[Johannes Schindelin]

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.

This addresses https://github.com/git-for-windows/git/issues/643 and
https://groups.google.com/forum/#!topic/git-for-windows/fVRdnDIKVuw

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	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).

 builtin/config.c | 3 +++
 1 file changed, 3 insertions(+)

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");
 
-- 
2.7.2.windows.1.2.gbc859c8

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

[Jeff King]

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

> 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

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

[John Keeping]

On Wed, Feb 24, 2016 at 03:26:57AM -0500, Jeff King wrote:
> On Wed, Feb 24, 2016 at 08:47:00AM +0100, Johannes Schindelin wrote:
> 
> > 	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.

Could we use GIT_CEILING_DIRECTORIES for this?  If it's set to
TEST_OUTPUT_DIRECTORY won't that cover the in-tree and out-of-tree test
cases?

We probably do still want Peff's REPOLESS prereq just in case someone is
collecting test results in a repository, but I think that will see much
better coverage than relying on people running tests from the tarball.

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

[Jeff King]

On Wed, Feb 24, 2016 at 10:14:03AM +0000, John Keeping wrote:

> On Wed, Feb 24, 2016 at 03:26:57AM -0500, Jeff King wrote:
> > On Wed, Feb 24, 2016 at 08:47:00AM +0100, Johannes Schindelin wrote:
> > 
> > > 	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.
> 
> Could we use GIT_CEILING_DIRECTORIES for this?  If it's set to
> TEST_OUTPUT_DIRECTORY won't that cover the in-tree and out-of-tree test
> cases?

Oh, right. That's much less nasty than my suggestion.

> We probably do still want Peff's REPOLESS prereq just in case someone is
> collecting test results in a repository,

We can create arbitrary hierarchies within the trash directory. So even
without removing the trash-dir .git, we could probably do:

  mkdir -p non-repo/foo &&
  (
	cd non-repo &&
	GIT_CEILING_DIRECTORIES=$(pwd) &&
	export GIT_CEILING_DIRECTORIES &&
	cd foo &&
	git config foo.bar baz
  )

or something. That should work everywhere.

> but I think that will see much
> better coverage than relying on people running tests from the tarball.

With mine I think we'd see coverage from git devs, as it works with
--root, too (and if you're not using --root with a RAM disk, I highly
recommend it for the speedup). But your suggestion is way better.

-Peff

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

[Johannes Schindelin]

Hi Peff, John & Junio (who recommended GIT_CEILING_DIRECTORIES first,

On Wed, 24 Feb 2016, Jeff King wrote:

> On Wed, Feb 24, 2016 at 10:14:03AM +0000, John Keeping wrote:
> 
> > On Wed, Feb 24, 2016 at 03:26:57AM -0500, Jeff King wrote:
> > > On Wed, Feb 24, 2016 at 08:47:00AM +0100, Johannes Schindelin wrote:
> > > 
> > > > 	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.
> > 
> > Could we use GIT_CEILING_DIRECTORIES for this?

Absolutely. I created a test and essentially duplicated Peff's finding:
this is a Windows-only issue.

Will keep you posted,
Dscho

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

[Johannes Schindelin]

Hi,

On Wed, 24 Feb 2016, Johannes Schindelin wrote:

> I created a test and essentially duplicated Peff's finding: this is a
> Windows-only issue.

Indeed it is in our patches:

	https://github.com/git-for-windows/git/commit/3a4c37b22

(short version: we carry some patches that simplify locking the config
file before building on top of it, and somehow we agreed back in the days
when this patch was written that it would be a good idea if that code
created leading directories.)

I will fix this issue in Git for Windows, but I still think that my patch
brings a usability improvement: the error message is just so much more to
the point (will submit v2 in a moment).

Ciao,
Dscho

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

[Jeff King]

On Wed, Feb 24, 2016 at 01:13:39PM +0100, Johannes Schindelin wrote:

> > I created a test and essentially duplicated Peff's finding: this is a
> > Windows-only issue.
> 
> Indeed it is in our patches:
> 
> 	https://github.com/git-for-windows/git/commit/3a4c37b22
> 
> (short version: we carry some patches that simplify locking the config
> file before building on top of it, and somehow we agreed back in the days
> when this patch was written that it would be a good idea if that code
> created leading directories.)

Thanks for tracking it down.

> I will fix this issue in Git for Windows, but I still think that my patch
> brings a usability improvement: the error message is just so much more to
> the point (will submit v2 in a moment).

Yeah, I agree it is still worth doing.

-Peff

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Wed, 24 Feb 2016, Johannes Schindelin wrote:
>
>> I created a test and essentially duplicated Peff's finding: this is a
>> Windows-only issue.
>
> Indeed it is in our patches:
>
> 	https://github.com/git-for-windows/git/commit/3a4c37b22
>
> (short version: we carry some patches that simplify locking the config
> file before building on top of it, and somehow we agreed back in the days
> when this patch was written that it would be a good idea if that code
> created leading directories.)

The log message there says this:

    This is particularly important when calling

         git config -f /non/existing/directory/config ...

I think that the change should be ejected.

	cat >/non/existing/directory/file
	mkdir /non/existing/directory/dir

would fail, and I do not see any valid reason for "git config" to be
different.

> I will fix this issue in Git for Windows, but I still think that my patch
> brings a usability improvement: the error message is just so much more to
> the point (will submit v2 in a moment).

Thanks.  I think your patch in this thread is an improvement.

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

[Duy Nguyen]

On Wed, Feb 24, 2016 at 3:26 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 24, 2016 at 08:47:00AM +0100, Johannes Schindelin wrote:
>
>> 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

Yep, tried the same thing before I asked about the .git dir thing on
gfw list. Same result.

> 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).

Known bug. If setup fails to find git dir and somebody (plenty of them
in fact) calls get_git_dir(), we automtaically assume git dir is
".git".

> 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.

If .git is created before lockfile code, then the lock should be
successfully created.  The fix in check_write() fixes it for Johannes,
so it must be something between

// if (action == ACTION_SET) {...
check_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
ret = git_config_set_in_file(given_config_source.file, argv[0], value);

I don't see where mkdir() can be called either.

Johannes, maybe you can force a crash in mingw_mkdir to pinpoint this
code? Just in case the bug is outside config code.
-- 
Duy

[PATCH v2] git config: report when trying to modify a non-existing repo config

[Johannes Schindelin]

It is a pilot error to call `git config section.key value` outside of
any Git worktree. The message

	error: could not lock config file .git/config: No such file or
	directory

is not very helpful in that situation, though. Let's print a helpful
message instead.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/config.c      | 3 +++
 t/t1308-config-set.sh | 9 +++++++++
 2 files changed, 12 insertions(+)

The commit message was adjusted to reflect that this is not a bug fix
(except for Git for Windows), and a test was added.

Interdiff vs v1:

 diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
 index 91235b7..f62409e 100755
 --- a/t/t1308-config-set.sh
 +++ b/t/t1308-config-set.sh
 @@ -218,4 +218,13 @@ test_expect_success 'check line errors for malformed values' '
  	test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result
  '
  
 +test_expect_success 'error on modifying repo config without repo' '
 +	mkdir no-repo &&
 +	GIT_CEILING_DIRECTORIES=$(pwd) &&
 +	export GIT_CEILING_DIRECTORIES &&
 +	cd no-repo &&
 +	test_must_fail git config a.b c 2>err &&
 +	grep "not in a git directory" err
 +'
 +
  test_done


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");
 
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 91235b7..f62409e 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -218,4 +218,13 @@ test_expect_success 'check line errors for malformed values' '
 	test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result
 '
 
+test_expect_success 'error on modifying repo config without repo' '
+	mkdir no-repo &&
+	GIT_CEILING_DIRECTORIES=$(pwd) &&
+	export GIT_CEILING_DIRECTORIES &&
+	cd no-repo &&
+	test_must_fail git config a.b c 2>err &&
+	grep "not in a git directory" err
+'
+
 test_done
-- 
2.7.2.windows.1.2.gbc859c8

Re: [PATCH v2] git config: report when trying to modify a non-existing repo config

[Duy Nguyen]

On Wed, Feb 24, 2016 at 01:48:11PM +0100, Johannes Schindelin wrote:
> +		die("not in a git directory");

Maybe wrap this string with _() for translation? Then we can pile this
patch on top to fix the rest in builtin/config.c.

-- 8< --
Subject: [PATCH] builtin/config.c: mark strings for translation

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/config.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index adc7727..7dad412 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -87,7 +87,7 @@ static struct option builtin_config_options[] = {
 static void check_argc(int argc, int min, int max) {
 	if (argc >= min && argc <= max)
 		return;
-	error("wrong number of arguments");
+	error(_("wrong number of arguments"));
 	usage_with_options(builtin_config_usage, builtin_config_options);
 }
 
@@ -188,7 +188,7 @@ static int get_value(const char *key_, const char *regex_)
 
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
-			error("invalid key pattern: %s", key_);
+			error(_("invalid key pattern: %s"), key_);
 			free(key_regexp);
 			key_regexp = NULL;
 			ret = CONFIG_INVALID_PATTERN;
@@ -209,7 +209,7 @@ static int get_value(const char *key_, const char *regex_)
 
 		regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(regexp, regex_, REG_EXTENDED)) {
-			error("invalid pattern: %s", regex_);
+			error(_("invalid pattern: %s"), regex_);
 			free(regexp);
 			regexp = NULL;
 			ret = CONFIG_INVALID_PATTERN;
@@ -353,10 +353,10 @@ static int get_colorbool(const char *var, int print)
 static void check_write(void)
 {
 	if (given_config_source.use_stdin)
-		die("writing to stdin is not supported");
+		die(_("writing to stdin is not supported"));
 
 	if (given_config_source.blob)
-		die("writing config blobs is not supported");
+		die(_("writing config blobs is not supported"));
 }
 
 struct urlmatch_current_candidate_value {
@@ -461,7 +461,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 	if (use_global_config + use_system_config + use_local_config +
 	    !!given_config_source.file + !!given_config_source.blob > 1) {
-		error("only one config file at a time.");
+		error(_("only one config file at a time."));
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
@@ -482,7 +482,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			 * location; error out even if XDG_CONFIG_HOME
 			 * is set and points at a sane location.
 			 */
-			die("$HOME not set");
+			die(_("$HOME not set"));
 
 		if (access_or_warn(user_config, R_OK, 0) &&
 		    xdg_config && !access_or_warn(xdg_config, R_OK, 0))
@@ -512,17 +512,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
+		error(_("only one type at a time."));
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
 	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
-		error("--get-color and variable type are incoherent");
+		error(_("--get-color and variable type are incoherent"));
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
 	if (HAS_MULTI_BITS(actions)) {
-		error("only one action at a time.");
+		error(_("only one action at a time."));
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 	if (actions == 0)
@@ -535,7 +535,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		}
 	if (omit_values &&
 	    !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) {
-		error("--name-only is only applicable to --list or --get-regexp");
+		error(_("--name-only is only applicable to --list or --get-regexp"));
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 	if (actions == ACTION_LIST) {
@@ -544,10 +544,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 					    &given_config_source,
 					    respect_includes) < 0) {
 			if (given_config_source.file)
-				die_errno("unable to read config file '%s'",
+				die_errno(_("unable to read config file '%s'"),
 					  given_config_source.file);
 			else
-				die("error processing config file(s)");
+				die(_("error processing config file(s)"));
 		}
 	}
 	else if (actions == ACTION_EDIT) {
@@ -555,11 +555,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		check_argc(argc, 0, 0);
 		if (!given_config_source.file && nongit)
-			die("not in a git directory");
+			die(_("not in a git directory"));
 		if (given_config_source.use_stdin)
-			die("editing stdin is not supported");
+			die(_("editing stdin is not supported"));
 		if (given_config_source.blob)
-			die("editing blobs is not supported");
+			die(_("editing blobs is not supported"));
 		git_config(git_default_config, NULL);
 		config_file = xstrdup(given_config_source.file ?
 				      given_config_source.file : git_path("config"));
@@ -584,8 +584,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1]);
 		ret = git_config_set_in_file(given_config_source.file, argv[0], value);
 		if (ret == CONFIG_NOTHING_SET)
-			error("cannot overwrite multiple values with a single value\n"
-			"       Use a regexp, --add or --replace-all to change %s.", argv[0]);
+			error(_("cannot overwrite multiple values with a single value\n"
+				"Use a regexp, --add or --replace-all to change %s."),
+			      argv[0]);
 		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
@@ -655,7 +656,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
-			die("No such section!");
+			die(_("No such section!"));
 	}
 	else if (actions == ACTION_REMOVE_SECTION) {
 		int ret;
@@ -666,7 +667,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
-			die("No such section!");
+			die(_("No such section!"));
 	}
 	else if (actions == ACTION_GET_COLOR) {
 		check_argc(argc, 1, 2);
-- 
2.7.2.531.gc9e018c

-- 8< --

Re: [PATCH v2] git config: report when trying to modify a non-existing repo config

[Johannes Schindelin]

Hi Duy,

On Wed, 24 Feb 2016, Duy Nguyen wrote:

> On Wed, Feb 24, 2016 at 01:48:11PM +0100, Johannes Schindelin wrote:
> > +		die("not in a git directory");
> 
> Maybe wrap this string with _() for translation? Then we can pile this
> patch on top to fix the rest in builtin/config.c.

Given that the context lines contain die() calls *without* _(), I would
find it more logical if your patch patched also the line I introduced,
together with the other die() calls.

Ciao,
Dscho

Re: [PATCH v2] git config: report when trying to modify a non-existing repo config

[Duy Nguyen]

On Wed, Feb 24, 2016 at 8:26 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Duy,
>
> On Wed, 24 Feb 2016, Duy Nguyen wrote:
>
>> On Wed, Feb 24, 2016 at 01:48:11PM +0100, Johannes Schindelin wrote:
>> > +           die("not in a git directory");
>>
>> Maybe wrap this string with _() for translation? Then we can pile this
>> patch on top to fix the rest in builtin/config.c.
>
> Given that the context lines contain die() calls *without* _(), I would
> find it more logical if your patch patched also the line I introduced,
> together with the other die() calls.

No problem. I'll go over all changes in 'next' and 'pu' some time
later and try to fix them all.
-- 
Duy

Re: [PATCH v2] git config: report when trying to modify a non-existing repo config

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> index 91235b7..f62409e 100755
> --- a/t/t1308-config-set.sh
> +++ b/t/t1308-config-set.sh
> @@ -218,4 +218,13 @@ test_expect_success 'check line errors for malformed values' '
>  	test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result
>  '
>  
> +test_expect_success 'error on modifying repo config without repo' '
> +	mkdir no-repo &&
> +	GIT_CEILING_DIRECTORIES=$(pwd) &&
> +	export GIT_CEILING_DIRECTORIES &&
> +	cd no-repo &&
> +	test_must_fail git config a.b c 2>err &&
> +	grep "not in a git directory" err
> +'
> +
>  test_done

Please make it a habit to run tests that go up/down in the hierarchy
in a subshell.  It is not a good excuse that this new test happens
to be at the end _right now_.

Re: [PATCH v2] git config: report when trying to modify a non-existing repo config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
>> index 91235b7..f62409e 100755
>> --- a/t/t1308-config-set.sh
>> +++ b/t/t1308-config-set.sh
>> @@ -218,4 +218,13 @@ test_expect_success 'check line errors for malformed values' '
>>  	test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result
>>  '
>>  
>> +test_expect_success 'error on modifying repo config without repo' '
>> +	mkdir no-repo &&
>> +	GIT_CEILING_DIRECTORIES=$(pwd) &&
>> +	export GIT_CEILING_DIRECTORIES &&
>> +	cd no-repo &&
>> +	test_must_fail git config a.b c 2>err &&
>> +	grep "not in a git directory" err
>> +'
>> +
>>  test_done
>
> Please make it a habit to run tests that go up/down in the hierarchy
> in a subshell.  It is not a good excuse that this new test happens
> to be at the end _right now_.

I'd squash this in.

Thanks.

 t/t1308-config-set.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index f62409e..9863d0d 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -220,11 +220,13 @@ test_expect_success 'check line errors for malformed values' '
 
 test_expect_success 'error on modifying repo config without repo' '
 	mkdir no-repo &&
-	GIT_CEILING_DIRECTORIES=$(pwd) &&
-	export GIT_CEILING_DIRECTORIES &&
-	cd no-repo &&
-	test_must_fail git config a.b c 2>err &&
-	grep "not in a git directory" err
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd no-repo &&
+		test_must_fail git config a.b c 2>err &&
+		grep "not in a git directory" err
+	)
 '
 
 test_done

Re: [PATCH v2] git config: report when trying to modify a non-existing repo config

[Johannes Schindelin]

Hi Junio,

On Wed, 24 Feb 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> >> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> >> index 91235b7..f62409e 100755
> >> --- a/t/t1308-config-set.sh
> >> +++ b/t/t1308-config-set.sh
> >> @@ -218,4 +218,13 @@ test_expect_success 'check line errors for malformed values' '
> >>  	test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result
> >>  '
> >>  
> >> +test_expect_success 'error on modifying repo config without repo' '
> >> +	mkdir no-repo &&
> >> +	GIT_CEILING_DIRECTORIES=$(pwd) &&
> >> +	export GIT_CEILING_DIRECTORIES &&
> >> +	cd no-repo &&
> >> +	test_must_fail git config a.b c 2>err &&
> >> +	grep "not in a git directory" err
> >> +'
> >> +
> >>  test_done
> >
> > Please make it a habit to run tests that go up/down in the hierarchy
> > in a subshell.  It is not a good excuse that this new test happens
> > to be at the end _right now_.
> 
> I'd squash this in.

Please do, unless you want me to resend with the squashed commit (I picked
up the change from your 'pu' branch)?

Ciao,
Dscho