Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Junio C Hamano]

Jeff King  writes:

> Good description.
>
> Signed-off-by: Jeff King 
>
> of course.
>
>> @@ -1304,6 +1315,7 @@ test_expect_success '--show-origin with --get-regexp' '
>>  file:$HOME/.gitconfig   user.global true
>>  file:.git/configuser.local true
>>  EOF
>> +GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
>>  git config --show-origin --get-regexp "user\.[g|l].*" >output &&
>>  test_cmp expect output
>>  '
>
> This is one is trying to do a multi-file lookup, but we couldn't look in
> the system config before. But to naturally extend it, it ought to look
> like this on top:
>
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index d2476a8..4dd5ce3 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1310,11 +1310,12 @@ test_expect_success '--show-origin with single file' '
>  
>  test_expect_success '--show-origin with --get-regexp' '
>   cat >expect <<-EOF &&
> + file:$HOME/etc-gitconfiguser.system true
>   file:$HOME/.gitconfig   user.global true
>   file:.git/configuser.local true
>   EOF
>   GIT_ETC_GITCONFIG=$HOME/etc-gitconfig \
> - git config --show-origin --get-regexp "user\.[g|l].*" >output &&
> + git config --show-origin --get-regexp "user\.[g|l|s].*" >output &&
>   test_cmp expect output
>  '

Makes sense modulo you inherited useless vertical bars from the
original.  I'll squash something like that in but without || ;-)

Thanks.

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Jeff King]

On Thu, Sep 29, 2016 at 02:03:39PM -0700, Junio C Hamano wrote:

> > -   git config --show-origin --get-regexp "user\.[g|l].*" >output &&
> > +   git config --show-origin --get-regexp "user\.[g|l|s].*" >output &&
> > test_cmp expect output
> >  '
> 
> Makes sense modulo you inherited useless vertical bars from the
> original.  I'll squash something like that in but without || ;-)

Heh, I glossed over that completely. Thanks.

-Peff

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Junio C Hamano]

Jeff King  writes:

> I just don't see it being a problem. Adding core.abbrev for the whole
> test suite is just about not having a big flag day where we change all
> the tests. Changing one or two tests (and again, I'd be surprised if we
> even have to do that) doesn't seem like a big deal.

I've already wasted several hours whipping t1300 into shape, because
it was done in not so forward-looking future-proofed way.  I am not
worried about core.abbrev but I am worried more about the next thing
that requires us to add an entry to t/gitconfig-for-test.  Adding a
corresponding entry to retain the old default for that new config to
two places may not be a big deal, but it still makes me feel a bit
uneasy.

In any case, I suspect that Linus's "auto" thing may still need the
custom system config with t1300 clean-up to pass the test, even
though I suspect it would compute that 7 is enough for most of the
tiny repositories our tests use, so I'll polish this a bit more
while waiting for that discussion to settle.

Thanks.

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Jeff King]

On Thu, Sep 29, 2016 at 12:06:15PM -0700, Junio C Hamano wrote:

> I think it deserves a separate patch and the result is more
> understandable.  I've queued this for now (on top of a revised 1/4
> that uses GIT_CONFIG_SYSTEM_PATH instead).

Thanks, makes sense (and I like the new variable name better, by the
way).

> -- >8 --
> From: Jeff King 
> Date: Thu, 29 Sep 2016 11:29:10 -0700
> Subject: [PATCH] t1300: check also system-wide configuration file in
>  --show-origin tests
> 
> Because we used to run our tests with GIT_CONFIG_NOSYSTEM, these did
> not test that the system-wide configuration file is also read and
> shown as one of the origins.  Create a custom/fake system-wide
> configuration file and make sure it appears in the output, using the
> newly introduced GIT_CONFIG_SYSTEM_PATH mechanism.
> 
> Signed-off-by: Junio C Hamano 

Good description.

Signed-off-by: Jeff King 

of course.

> @@ -1304,6 +1315,7 @@ test_expect_success '--show-origin with --get-regexp' '
>   file:$HOME/.gitconfig   user.global true
>   file:.git/configuser.local true
>   EOF
> + GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
>   git config --show-origin --get-regexp "user\.[g|l].*" >output &&
>   test_cmp expect output
>  '

This is one is trying to do a multi-file lookup, but we couldn't look in
the system config before. But to naturally extend it, it ought to look
like this on top:

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index d2476a8..4dd5ce3 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1310,11 +1310,12 @@ test_expect_success '--show-origin with single file' '
 
 test_expect_success '--show-origin with --get-regexp' '
cat >expect <<-EOF &&
+   file:$HOME/etc-gitconfiguser.system true
file:$HOME/.gitconfig   user.global true
file:.git/configuser.local true
EOF
GIT_ETC_GITCONFIG=$HOME/etc-gitconfig \
-   git config --show-origin --get-regexp "user\.[g|l].*" >output &&
+   git config --show-origin --get-regexp "user\.[g|l|s].*" >output &&
test_cmp expect output
 '
 
> @@ -1312,6 +1324,7 @@ test_expect_success '--show-origin getting a single 
> key' '
>   cat >expect <<-\EOF &&
>   file:.git/configlocal
>   EOF
> + GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
>   git config --show-origin user.override >output &&
>   test_cmp expect output
>  '

And I was tempted to say this one should not need to care, but I guess
it is testing that we correctly read the override from the local config
over the global one. So likewise, it is good to check that we also
override the system config (it does not effect the "expect" output, but
that does not mean it is not enhancing the test).

-Peff

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Jeff King]

On Thu, Sep 29, 2016 at 11:57:02AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> "either" meaning "we do not need to add --local and we do not need
> >> GIT_CONFIG_NOSYSTEM"?
> >
> > Yes. I didn't test it with your core.abbrev patch 4/4, but I _didn't_
> > have to touch their expected output after pointing them at a non-empty
> > etc-gitconfig file in the trash directory. Which implies to me they
> > don't care either way (which makes sense; they are asking for a specific
> > key which is supposed to be found in one of the other files).
> 
> There is a bit of problem here, though.
> 
>  * If we make t1300 point at its own system-wide config, it will be
>in control of its contents, so "find this key" will find only it
>wants to find (or we found a regression).
> 
>  * But then if it ever does something that depends on the default
>value of core.abbrev (or whatever we'd tweak in response to the
>next suggestion by Linus ;-), we cannot really allow it to do
>so.  We'd want t/gitconfig-for-test to be the single place that
>we can tweak these things, but we'll have to know t1300 uses its
>own and need to make the same change there, too.

Right, but I think that's fine. Tests that care deeply about the
contents of etc-gitconfig are unlikely to care about core.abbrev. And in
the off chance that they do, then the worst case is...they get updated
to handle core.abbrev (either passing a command line option, or just
putting core.abbrev in their test file).

I just don't see it being a problem. Adding core.abbrev for the whole
test suite is just about not having a big flag day where we change all
the tests. Changing one or two tests (and again, I'd be surprised if we
even have to do that) doesn't seem like a big deal.

-Peff

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Junio C Hamano]

Jeff King  writes:

> On Thu, Sep 29, 2016 at 11:13:45AM -0700, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > I think anytime you would use GIT_CONFIG_NOSYSTEM over --local, it is an
>> > indication that the test is trying to check how multiple sources
>> > interact. And the right thing to do for them is to set GIT_ETC_GITCONFIG
>> > to some known quantity. We just couldn't do that before, so we skipped
>> > it.  IOW, something like the patch below (on top of yours).
>> 
>> OK, that way we can make sure that "multiple sources" operations do
>> look at the system-wide stuff.
>
> Exactly.

I think it deserves a separate patch and the result is more
understandable.  I've queued this for now (on top of a revised 1/4
that uses GIT_CONFIG_SYSTEM_PATH instead).

-- >8 --
From: Jeff King 
Date: Thu, 29 Sep 2016 11:29:10 -0700
Subject: [PATCH] t1300: check also system-wide configuration file in
 --show-origin tests

Because we used to run our tests with GIT_CONFIG_NOSYSTEM, these did
not test that the system-wide configuration file is also read and
shown as one of the origins.  Create a custom/fake system-wide
configuration file and make sure it appears in the output, using the
newly introduced GIT_CONFIG_SYSTEM_PATH mechanism.

Signed-off-by: Junio C Hamano 
---
 t/t1300-repo-config.sh | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 0543b62227bf..aa25577709c5 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1236,6 +1236,11 @@ test_expect_success 'set up --show-origin tests' '
[user]
relative = include
EOF
+   cat >"$HOME"/etc-gitconfig <<-\EOF &&
+   [user]
+   system = true
+   override = system
+   EOF
cat >"$HOME"/.gitconfig <<-EOF &&
[user]
global = true
@@ -1254,6 +1259,8 @@ test_expect_success 'set up --show-origin tests' '
 
 test_expect_success '--show-origin with --list' '
cat >expect <<-EOF &&
+   file:$HOME/etc-gitconfiguser.system=true
+   file:$HOME/etc-gitconfiguser.override=system
file:$HOME/.gitconfig   user.global=true
file:$HOME/.gitconfig   user.override=global
file:$HOME/.gitconfig   
include.path=$INCLUDE_DIR/absolute.include
@@ -1264,13 +1271,16 @@ test_expect_success '--show-origin with --list' '
file:.git/../include/relative.include   user.relative=include
command line:   user.cmdline=true
EOF
+   GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git -c user.cmdline=true config --list --show-origin >output &&
test_cmp expect output
 '
 
 test_expect_success '--show-origin with --list --null' '
cat >expect <<-EOF &&
-   file:$HOME/.gitconfigQuser.global
+   file:$HOME/etc-gitconfigQuser.system
+   trueQfile:$HOME/etc-gitconfigQuser.override
+   systemQfile:$HOME/.gitconfigQuser.global
trueQfile:$HOME/.gitconfigQuser.override
globalQfile:$HOME/.gitconfigQinclude.path

$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
@@ -1281,6 +1291,7 @@ test_expect_success '--show-origin with --list --null' '
includeQcommand line:Quser.cmdline
trueQ
EOF
+   GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git -c user.cmdline=true config --null --list --show-origin >output.raw 
&&
nul_to_q output &&
# The here-doc above adds a newline that the --null output would not
@@ -1304,6 +1315,7 @@ test_expect_success '--show-origin with --get-regexp' '
file:$HOME/.gitconfig   user.global true
file:.git/configuser.local true
EOF
+   GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git config --show-origin --get-regexp "user\.[g|l].*" >output &&
test_cmp expect output
 '
@@ -1312,6 +1324,7 @@ test_expect_success '--show-origin getting a single key' '
cat >expect <<-\EOF &&
file:.git/configlocal
EOF
+   GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git config --show-origin user.override >output &&
test_cmp expect output
 '
-- 
2.10.0-589-g5adf4e1

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Junio C Hamano]

Jeff King  writes:

>> "either" meaning "we do not need to add --local and we do not need
>> GIT_CONFIG_NOSYSTEM"?
>
> Yes. I didn't test it with your core.abbrev patch 4/4, but I _didn't_
> have to touch their expected output after pointing them at a non-empty
> etc-gitconfig file in the trash directory. Which implies to me they
> don't care either way (which makes sense; they are asking for a specific
> key which is supposed to be found in one of the other files).

There is a bit of problem here, though.

 * If we make t1300 point at its own system-wide config, it will be
   in control of its contents, so "find this key" will find only it
   wants to find (or we found a regression).

 * But then if it ever does something that depends on the default
   value of core.abbrev (or whatever we'd tweak in response to the
   next suggestion by Linus ;-), we cannot really allow it to do
   so.  We'd want t/gitconfig-for-test to be the single place that
   we can tweak these things, but we'll have to know t1300 uses its
   own and need to make the same change there, too.

So, I dunno.

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Jeff King]

On Thu, Sep 29, 2016 at 11:13:45AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I think anytime you would use GIT_CONFIG_NOSYSTEM over --local, it is an
> > indication that the test is trying to check how multiple sources
> > interact. And the right thing to do for them is to set GIT_ETC_GITCONFIG
> > to some known quantity. We just couldn't do that before, so we skipped
> > it.  IOW, something like the patch below (on top of yours).
> 
> OK, that way we can make sure that "multiple sources" operations do
> look at the system-wide stuff.

Exactly.

> > Note that the
> > commands that are doing a "--get" and not a "--list" don't actually seem
> > to need either (because they are getting the values out of the local
> > file anyway), so we could drop the setting of GIT_ETC_GITCONFIG from
> > them entirely.
> 
> "either" meaning "we do not need to add --local and we do not need
> GIT_CONFIG_NOSYSTEM"?

Yes. I didn't test it with your core.abbrev patch 4/4, but I _didn't_
have to touch their expected output after pointing them at a non-empty
etc-gitconfig file in the trash directory. Which implies to me they
don't care either way (which makes sense; they are asking for a specific
key which is supposed to be found in one of the other files).

-Peff

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Junio C Hamano]

Jeff King  writes:

> I think anytime you would use GIT_CONFIG_NOSYSTEM over --local, it is an
> indication that the test is trying to check how multiple sources
> interact. And the right thing to do for them is to set GIT_ETC_GITCONFIG
> to some known quantity. We just couldn't do that before, so we skipped
> it.  IOW, something like the patch below (on top of yours).

OK, that way we can make sure that "multiple sources" operations do
look at the system-wide stuff.

> Note that the
> commands that are doing a "--get" and not a "--list" don't actually seem
> to need either (because they are getting the values out of the local
> file anyway), so we could drop the setting of GIT_ETC_GITCONFIG from
> them entirely.

"either" meaning "we do not need to add --local and we do not need
GIT_CONFIG_NOSYSTEM"?

Re: [PATCH 2/4] t13xx: do not assume system config is empty

[Jeff King]

On Wed, Sep 28, 2016 at 04:30:45PM -0700, Junio C Hamano wrote:

> The tests for show-origin codepath in "git config" however cannot be
> tweaked with "--local" etc., because they wants to read also from
> $HOME/.gitconfig and make sure what comes from where.  Disable
> reading from the system-wide config with GIT_CONFIG_NOSYSTEM=1 for
> these tests.

I think anytime you would use GIT_CONFIG_NOSYSTEM over --local, it is an
indication that the test is trying to check how multiple sources
interact. And the right thing to do for them is to set GIT_ETC_GITCONFIG
to some known quantity. We just couldn't do that before, so we skipped
it.

IOW, something like the patch below (on top of yours). Note that the
commands that are doing a "--get" and not a "--list" don't actually seem
to need either (because they are getting the values out of the local
file anyway), so we could drop the setting of GIT_ETC_GITCONFIG from
them entirely.

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index b998568..d2476a8 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1234,6 +1234,11 @@ test_expect_success 'set up --show-origin tests' '
[user]
relative = include
EOF
+   cat >"$HOME"/etc-gitconfig <<-\EOF &&
+   [user]
+   system = true
+   override = system
+   EOF
cat >"$HOME"/.gitconfig <<-EOF &&
[user]
global = true
@@ -1252,6 +1257,8 @@ test_expect_success 'set up --show-origin tests' '
 
 test_expect_success '--show-origin with --list' '
cat >expect <<-EOF &&
+   file:$HOME/etc-gitconfiguser.system=true
+   file:$HOME/etc-gitconfiguser.override=system
file:$HOME/.gitconfig   user.global=true
file:$HOME/.gitconfig   user.override=global
file:$HOME/.gitconfig   
include.path=$INCLUDE_DIR/absolute.include
@@ -1262,14 +1269,16 @@ test_expect_success '--show-origin with --list' '
file:.git/../include/relative.include   user.relative=include
command line:   user.cmdline=true
EOF
-   GIT_CONFIG_NOSYSTEM=1 \
+   GIT_ETC_GITCONFIG=$HOME/etc-gitconfig \
git -c user.cmdline=true config --list --show-origin >output &&
test_cmp expect output
 '
 
 test_expect_success '--show-origin with --list --null' '
cat >expect <<-EOF &&
-   file:$HOME/.gitconfigQuser.global
+   file:$HOME/etc-gitconfigQuser.system
+   trueQfile:$HOME/etc-gitconfigQuser.override
+   systemQfile:$HOME/.gitconfigQuser.global
trueQfile:$HOME/.gitconfigQuser.override
globalQfile:$HOME/.gitconfigQinclude.path

$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
@@ -1280,7 +1289,7 @@ test_expect_success '--show-origin with --list --null' '
includeQcommand line:Quser.cmdline
trueQ
EOF
-   GIT_CONFIG_NOSYSTEM=1 \
+   GIT_ETC_GITCONFIG=$HOME/etc-gitconfig \
git -c user.cmdline=true config --null --list --show-origin >output.raw 
&&
nul_to_q output &&
# The here-doc above adds a newline that the --null output would not
@@ -1304,7 +1313,7 @@ test_expect_success '--show-origin with --get-regexp' '
file:$HOME/.gitconfig   user.global true
file:.git/configuser.local true
EOF
-   GIT_CONFIG_NOSYSTEM=1 \
+   GIT_ETC_GITCONFIG=$HOME/etc-gitconfig \
git config --show-origin --get-regexp "user\.[g|l].*" >output &&
test_cmp expect output
 '
@@ -1313,7 +1322,7 @@ test_expect_success '--show-origin getting a single key' '
cat >expect <<-\EOF &&
file:.git/configlocal
EOF
-   GIT_CONFIG_NOSYSTEM=1 \
+   GIT_ETC_GITCONFIG=$HOME/etc-gitconfig \
git config --show-origin user.override >output &&
test_cmp expect output
 '