| Message ID | 2da2131114eb47e70ccaf8fb9c51bf7fb5b173b0.1605801143.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config: add --literal-value option | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > +test_expect_success 'set all config with value_regex' ' > + q_to_tab >initial <<-\EOF && > + [abc] > + Qkey = one > + EOF > + > + cp initial .git/config && Not a new problem with this patch, but does the above pattern introduce potential problems? I am wondering if overwriting the config file with a little piece that has only the stuff the test is interested in, while wiping the parts that may be essential for repository integrity (e.g. "extensions.objectFormat"), is OK in the long run (brian cc'ed for his sha256 work). There also are autodetected crlf settings etc. that are in the .git/config when a test repository is created, and we probably would want to keep them intact.
On 2020-11-19 at 22:24:33, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > +test_expect_success 'set all config with value_regex' ' > > + q_to_tab >initial <<-\EOF && > > + [abc] > > + Qkey = one > > + EOF > > + > > + cp initial .git/config && > > Not a new problem with this patch, but does the above pattern > introduce potential problems? I am wondering if overwriting the > config file with a little piece that has only the stuff the test is > interested in, while wiping the parts that may be essential for > repository integrity (e.g. "extensions.objectFormat"), is OK in the > long run (brian cc'ed for his sha256 work). There also are > autodetected crlf settings etc. that are in the .git/config when a > test repository is created, and we probably would want to keep them > intact. I haven't looked at the code, but if you're just using git config in a test, then overwriting the config file shouldn't be a problem with SHA-256. If you're trying to read or write objects or the index, then that's definitely a problem, and you'll definitely notice exciting failures if you do that.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2020-11-19 at 22:24:33, Junio C Hamano wrote: >> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >> > +test_expect_success 'set all config with value_regex' ' >> > + q_to_tab >initial <<-\EOF && >> > + [abc] >> > + Qkey = one >> > + EOF >> > + >> > + cp initial .git/config && >> >> Not a new problem with this patch, but does the above pattern >> introduce potential problems? I am wondering if overwriting the >> config file with a little piece that has only the stuff the test is >> interested in, while wiping the parts that may be essential for >> repository integrity (e.g. "extensions.objectFormat"), is OK in the >> long run (brian cc'ed for his sha256 work). There also are >> autodetected crlf settings etc. that are in the .git/config when a >> test repository is created, and we probably would want to keep them >> intact. > > I haven't looked at the code, but if you're just using git config in a > test, then overwriting the config file shouldn't be a problem with > SHA-256. If you're trying to read or write objects or the index, then > that's definitely a problem, and you'll definitely notice exciting > failures if you do that. Yes, that is exactly what worries me. And I was trolling for ideas from those on cc: list to come up with a better convention to test the behaviour of "git config". I think taking "git config --list [--file FILE]" before and after the ops of interest and basing the good/bad decision on the difference would be a saner approach. It obviously relies on "git config --list" to be working correctly, which the current approach that uses hardcoded "initial" state and expects result that is byte-for-byte identical to a hardcoded file contents after the ops does not have to rely on, but at the same time, the current approach assumes too much on the actual file format (e.g. no user or script would care how "key = val" line is indented, but the current approach insists that they are indented exactly with one tab), so overall it may even be an improvement. Thanks.
On Thu, Nov 19, 2020 at 02:24:33PM -0800, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > +test_expect_success 'set all config with value_regex' ' > > + q_to_tab >initial <<-\EOF && > > + [abc] > > + Qkey = one > > + EOF > > + > > + cp initial .git/config && > > Not a new problem with this patch, but does the above pattern > introduce potential problems? I am wondering if overwriting the > config file with a little piece that has only the stuff the test is > interested in, while wiping the parts that may be essential for > repository integrity (e.g. "extensions.objectFormat"), is OK in the > long run (brian cc'ed for his sha256 work). There also are > autodetected crlf settings etc. that are in the .git/config when a > test repository is created, and we probably would want to keep them > intact. t1300 is full of this kind of junk. Several years ago, while working on some of the repositoryformatversion code, I noticed that we will accept a repository that does not have core.repositoryformatversion set at all, nor even has a .git/config present! It's easy to fix in the code, but it causes failures all over t1300. So then I started converting t1300 to use "config --file" (which almost certainly didn't exist back when most of those tests were originally written). I don't remember how or why it got hairy, but it was enough that I eventually dropped it (unlike many of my other stale topics, I don't think I've even kept rebasing it forward as a WIP). Possibly I was concerned that people in the wild were relying on a blank or missing config being the same as repositoryformatversion=0. That will definitely stop working in a sha256 world anyway, though, because they'll need the objectFormat extension. So that got a bit off-track, but I think: - t1300 already is very much like this, so it's not a new thing - but I would be happy not to see it go further in that direction, even if it means inconsistency with the rest of the script -Peff
Jeff King <peff@peff.net> writes: > So that got a bit off-track, but I think: > > - t1300 already is very much like this, so it's not a new thing > > - but I would be happy not to see it go further in that direction, > even if it means inconsistency with the rest of the script Yeah, I especially like the latter half ;-)
On 2020-11-20 at 18:39:03, Jeff King wrote: > t1300 is full of this kind of junk. Several years ago, while working on > some of the repositoryformatversion code, I noticed that we will accept > a repository that does not have core.repositoryformatversion set at all, > nor even has a .git/config present! Yup. We test this in t3200 as well. I don't love it, but it exists. > It's easy to fix in the code, but it causes failures all over t1300. So > then I started converting t1300 to use "config --file" (which > almost certainly didn't exist back when most of those tests were > originally written). I don't remember how or why it got hairy, but it > was enough that I eventually dropped it (unlike many of my other stale > topics, I don't think I've even kept rebasing it forward as a WIP). > > Possibly I was concerned that people in the wild were relying on a blank > or missing config being the same as repositoryformatversion=0. That will > definitely stop working in a sha256 world anyway, though, because > they'll need the objectFormat extension. Which is exactly why that test in t3200 has a SHA1 prerequisite. I'm sure we'll hear someone complain about the fact that SHA-256 repositories have to have a config file, but I'm fine with us not supporting it. I should point out that lacking a config file also only works on Unix systems with a POSIX file system (including case-sensitive macOS), since otherwise core.ignorecase (and core.symlinks, if appropriate) aren't set correctly. It also doesn't work for bare repositories on any OS. So hopefully the number of people doing this is quite small. > So that got a bit off-track, but I think: > > - t1300 already is very much like this, so it's not a new thing > > - but I would be happy not to see it go further in that direction, > even if it means inconsistency with the rest of the script I agree we shouldn't make things worse.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: >> So that got a bit off-track, but I think: >> >> - t1300 already is very much like this, so it's not a new thing >> >> - but I would be happy not to see it go further in that direction, >> even if it means inconsistency with the rest of the script > > I agree we shouldn't make things worse. I started looking at early parts of t1300 and here is how far I managed to get before I can no longer keep staring the existing tests without vomitting. I am reasonably happy with the "let's keep the vanilla untouched one in .git/config-initial, refrain from using [core] and other sections that MUST be in the initial configuration for testing, and use a wrapper that reads expected addition to the initial one from the standard input for validation" approach I came up with, but I am not happy with the name 'compare_expect'; 'validate_config_result' might be a better name. In any case, the reason I am sending this out early is if people find this approach to clean things up a sensible one. If we can find concensus, perhaps I (or somebody else---hint, hint) can find time to do the #leftoverbits following the approach after the ds/config-literal-value and ds/maintenance-part-3 topics graduate to 'master'. t/t1300-config.sh | 139 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 71 insertions(+), 68 deletions(-) diff --git c/t/t1300-config.sh w/t/t1300-config.sh index df13afaffd..c33520d7fa 100755 --- c/t/t1300-config.sh +++ w/t/t1300-config.sh @@ -7,80 +7,84 @@ test_description='Test git config in different settings' . ./test-lib.sh -test_expect_success 'clear default config' ' - rm -f .git/config +test_expect_success 'save away default config' ' + cp .git/config .git/config-initial ' -cat > expect << EOF -[core] - penguin = little blue -EOF -test_expect_success 'initial' ' - git config core.penguin "little blue" && +compare_expect () { + { + cat .git/config-initial && + sed -e 's/^[|]//' + } >expect && test_cmp expect .git/config +} + +test_expect_success 'initial' ' + git config configtest.penguin "little blue" && + compare_expect <<-\EOF + [configtest] + | penguin = little blue + EOF ' -cat > expect << EOF -[core] - penguin = little blue - Movie = BadPhysics -EOF test_expect_success 'mixed case' ' - git config Core.Movie BadPhysics && - test_cmp expect .git/config + git config ConfigTest.Movie BadPhysics && + compare_expect <<-\EOF + [configtest] + | penguin = little blue + | Movie = BadPhysics + EOF ' -cat > expect << EOF -[core] - penguin = little blue - Movie = BadPhysics -[Cores] - WhatEver = Second -EOF test_expect_success 'similar section' ' - git config Cores.WhatEver Second && - test_cmp expect .git/config + git config ConfigTests.WhatEver Second && + compare_expect <<-\EOF + [configtest] + | penguin = little blue + | Movie = BadPhysics + [ConfigTests] + | WhatEver = Second + EOF ' -cat > expect << EOF -[core] - penguin = little blue - Movie = BadPhysics - UPPERCASE = true -[Cores] - WhatEver = Second -EOF test_expect_success 'uppercase section' ' - git config CORE.UPPERCASE true && - test_cmp expect .git/config + git config CONFIGTEST.UPPERCASE true && + compare_expect <<-\EOF + [configtest] + | penguin = little blue + | Movie = BadPhysics + | UPPERCASE = true + [ConfigTests] + | WhatEver = Second + EOF ' test_expect_success 'replace with non-match' ' - git config core.penguin kingpin !blue + git config configtest.penguin kingpin !blue ' test_expect_success 'replace with non-match (actually matching)' ' - git config core.penguin "very blue" !kingpin + git config configtest.penguin "very blue" !kingpin ' -cat > expect << EOF -[core] - penguin = very blue - Movie = BadPhysics - UPPERCASE = true - penguin = kingpin -[Cores] - WhatEver = Second -EOF - -test_expect_success 'non-match result' 'test_cmp expect .git/config' +test_expect_success 'non-match result' ' + compare_expect <<-\EOF + [configtest] + | penguin = very blue + | Movie = BadPhysics + | UPPERCASE = true + | penguin = kingpin + [ConfigTests] + | WhatEver = Second + EOF +' test_expect_success 'find mixed-case key by canonical name' ' - test_cmp_config Second cores.whatever + test_cmp_config Second configtests.whatever ' test_expect_success 'find mixed-case key by non-canonical name' ' - test_cmp_config Second CoReS.WhAtEvEr + test_cmp_config Second CoNfIgTeSts.WhAtEvEr ' test_expect_success 'subsections are not canonicalized by git-config' ' @@ -94,28 +98,27 @@ test_expect_success 'subsections are not canonicalized by git-config' ' test_cmp_config two section.SubSection.key ' -cat > .git/config <<\EOF -[alpha] -bar = foo -[beta] -baz = multiple \ -lines -foo = bar -EOF - test_expect_success 'unset with cont. lines' ' - git config --unset beta.baz + { + cat .git/config-initial && + cat <<-\EOF + [alpha] + bar = foo + [beta] + baz = multiple \ + lines + foo = bar + EOF + } >.git/config && + git config --unset beta.baz && + compare_expect <<-\EOF + [alpha] + bar = foo + [beta] + foo = bar + EOF ' -cat > expect <<\EOF -[alpha] -bar = foo -[beta] -foo = bar -EOF - -test_expect_success 'unset with cont. lines is correct' 'test_cmp expect .git/config' - cat > .git/config << EOF [beta] ; silly comment # another comment noIndent= sillyValue ; 'nother silly comment
On Sat, Nov 21, 2020 at 07:31:14PM -0800, Junio C Hamano wrote: > >> So that got a bit off-track, but I think: > >> > >> - t1300 already is very much like this, so it's not a new thing > >> > >> - but I would be happy not to see it go further in that direction, > >> even if it means inconsistency with the rest of the script > > > > I agree we shouldn't make things worse. > > I started looking at early parts of t1300 and here is how far I > managed to get before I can no longer keep staring the existing > tests without vomitting. I think my similar gastric reaction is what caused me to stop looking at it long ago. But it may also have been the test brian mentioned that explicitly checks that this case works (and for which he had to set SHA1 prereq). > I am reasonably happy with the "let's keep the vanilla untouched one > in .git/config-initial, refrain from using [core] and other sections > that MUST be in the initial configuration for testing, and use a > wrapper that reads expected addition to the initial one from the > standard input for validation" approach I came up with, but I am not > happy with the name 'compare_expect'; 'validate_config_result' might > be a better name. IMHO this is worse than just using "config --file" in most of the tests. It's more steps to remember to deal with. And most tests do not care at all what the source file is. There are a few that check the order of lookup with respect to system and user files, but they could probably be run what non-destructive changes. That said, most of the effort is in the tedium of switching each individual test. I am happy for whoever volunteers to do that work to have the final say in the approach. -Peff
Jeff King <peff@peff.net> writes: >> I am reasonably happy with the "let's keep the vanilla untouched one >> in .git/config-initial, refrain from using [core] and other sections >> that MUST be in the initial configuration for testing, and use a >> wrapper that reads expected addition to the initial one from the >> standard input for validation" approach I came up with, but I am not >> happy with the name 'compare_expect'; 'validate_config_result' might >> be a better name. > > IMHO this is worse than just using "config --file" in most of the tests. > It's more steps to remember to deal with. And most tests do not care at > all what the source file is. "Most tests do not care" only indicates the lack of test coverage. Knowing the implementation, it probably is OK to assume that things would work fine as long as "--file <file>" works correctly, though ;-) Not having to keep the minimum parts of the real configuration file, and being able to use a throw-away file for each test, certainly makes things cleaner. > That said, most of the effort is in the tedium of switching each > individual test. I am happy for whoever volunteers to do that work to > have the final say in the approach. Yup.
diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 97ebfe1f9d..ef56b08070 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1914,4 +1914,39 @@ test_expect_success '--replace-all does not invent newlines' ' test_cmp expect .git/config ' +test_expect_success 'set all config with value_regex' ' + q_to_tab >initial <<-\EOF && + [abc] + Qkey = one + EOF + + cp initial .git/config && + git config abc.key two a+ && + q_to_tab >expect <<-\EOF && + [abc] + Qkey = one + Qkey = two + EOF + test_cmp expect .git/config && + + test_must_fail git config abc.key three o+ 2>err && + test_i18ngrep "has multiple values" err && + git config abc.key three a+ && + q_to_tab >expect <<-\EOF && + [abc] + Qkey = one + Qkey = two + Qkey = three + EOF + test_cmp expect .git/config && + + cp initial .git/config && + git config abc.key three o+ && + q_to_tab >expect <<-\EOF && + [abc] + Qkey = three + EOF + test_cmp expect .git/config +' + test_done