Re: [PATCH 2/4] t13xx: do not assume system config is empty
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
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
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
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
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
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
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
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
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
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 '
[PATCH 2/4] t13xx: do not assume system config is empty
Most parts of these two tests want to read from the local configuration file they prepare and make sure expected names and values appear with "git config --list". Once we add custom configuration items that we want to affect the tests with globally to t/gitconfig-for-test file, these will start seeing the contents from there and break. Clarify with --local that they only care about the contents from their local configuration. 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. Signed-off-by: Junio C Hamano --- t/t1300-repo-config.sh | 24 +--- t/t1308-config-set.sh | 1 + 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 1184f43..b998568 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -341,13 +341,11 @@ version.1.2.3eX.alpha=beta EOF test_expect_success 'working --list' ' - git config --list > output && + git config --local --list > output && test_cmp expect output ' -cat > expect << EOF -EOF - -test_expect_success '--list without repo produces empty output' ' +test_expect_success '--list without repo shows only from the global' ' + git config --system --list >expect && git --git-dir=nonexistent config --list >output && test_cmp expect output ' @@ -360,7 +358,7 @@ version.1.2.3eX.alpha EOF test_expect_success '--name-only --list' ' - git config --name-only --list >output && + git config --local --name-only --list >output && test_cmp expect output ' @@ -370,7 +368,7 @@ nextsection.nonewline wow2 for me EOF test_expect_success '--get-regexp' ' - git config --get-regexp in >output && + git config --local --get-regexp in >output && test_cmp expect output ' @@ -380,7 +378,7 @@ nextsection.nonewline EOF test_expect_success '--name-only --get-regexp' ' - git config --name-only --get-regexp in >output && + git config --local --name-only --get-regexp in >output && test_cmp expect output ' @@ -391,7 +389,7 @@ EOF test_expect_success '--add' ' git config --add nextsection.nonewline "wow4 for you" && - git config --get-all nextsection.nonewline > output && + git config --local --get-all nextsection.nonewline > output && test_cmp expect output ' @@ -935,7 +933,7 @@ section.quotecont=cont;inued EOF test_expect_success 'value continued on next line' ' - git config --list > result && + git config --local --list > result && test_cmp result expect ' @@ -959,7 +957,7 @@ Qsection.sub=section.val4 Qsection.sub=section.val5Q EOF test_expect_success '--null --list' ' - git config --null --list >result.raw && + git config --null --local --list >result.raw && nul_to_q result && echo >>result && test_cmp expect result @@ -1264,6 +1262,7 @@ 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 -c user.cmdline=true config --list --show-origin >output && test_cmp expect output ' @@ -1281,6 +1280,7 @@ test_expect_success '--show-origin with --list --null' ' includeQcommand line:Quser.cmdline trueQ EOF + GIT_CONFIG_NOSYSTEM=1 \ 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 +1304,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 config --show-origin --get-regexp "user\.[g|l].*" >output && test_cmp expect output ' @@ -1312,6 +1313,7 @@ test_expect_success '--show-origin getting a single key' ' cat >expect <<-\EOF && file:.git/configlocal EOF + GIT_CONFIG_NOSYSTEM=1 \ git config --show-origin user.override >output && test_cmp expect output ' diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 7655c94..5d5adb1 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -260,6 +260,7 @@ test_expect_success 'iteration shows correct origins' ' name= scope=cmdline EOF + GIT_CONFIG_NOSYSTEM=1 \ GIT_CONFIG_PARAMETERS=$cmdline_config test-config iterate >actual && test_cmp expect actual ' -- 2.10.0-584-gc9e068c