| Message ID | ec1b5bdd176e6a3f093b76b732fd9e960a7880ca.1704802213.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: mark "files"-backend specific tests | expand |
On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote: > The t1300 test suite exercises the git-config(1) tool. To do so we > overwrite ".git/config" to contain custom contents. While this is easy > enough to do, it may create problems when using a non-default repository > format because we also overwrite the repository format version as well > as any potential extensions. > > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the > problem. An alternative would be to carry over mandatory config keys > into the rewritten config file. But the effort does not seem worth it > given that the system under test is git-config(1), which is at a lower > level than the repository format. I think I am missing something obvious here ;-). > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > t/t1300-config.sh | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index f4e2752134..1e953a0fc2 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' ' > test_must_fail git config --file=linktolinktonada --list > ' > > -test_expect_success 'check split_cmdline return' " > +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' " > git config alias.split-cmdline-fix 'echo \"' && > test_must_fail git split-cmdline-fix && > echo foo > foo && > @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' ' > test_cmp expect actual > ' Looking at this first test, for example, I see two places where we modify the configuration file: - git config alias.split-cmdline-fix 'echo \"' - git config branch.main.mergeoptions 'echo \"' I think I am missing some detail about why we can't do this when we have extensions enabled? Thanks, Taylor
On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote: > The t1300 test suite exercises the git-config(1) tool. To do so we > overwrite ".git/config" to contain custom contents. While this is easy > enough to do, it may create problems when using a non-default repository > format because we also overwrite the repository format version as well > as any potential extensions. > > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the > problem. An alternative would be to carry over mandatory config keys > into the rewritten config file. But the effort does not seem worth it > given that the system under test is git-config(1), which is at a lower > level than the repository format. If I'm understanding correctly, with the approach taken by this patch, won't we undesirably lose some git-config test coverage if the file-based backend is ever retired, or if tests specific to it are ever disabled by default? As such, it seems like the alternative "fix" you mention above would be preferable to ensure that coverage of git-config doesn't get diluted. Or am I misunderstanding something?
On Tue, Jan 09, 2024 at 01:41:57PM -0500, Taylor Blau wrote: > On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote: > > The t1300 test suite exercises the git-config(1) tool. To do so we > > overwrite ".git/config" to contain custom contents. While this is easy > > enough to do, it may create problems when using a non-default repository > > format because we also overwrite the repository format version as well > > as any potential extensions. > > > > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the > > problem. An alternative would be to carry over mandatory config keys > > into the rewritten config file. But the effort does not seem worth it > > given that the system under test is git-config(1), which is at a lower > > level than the repository format. > > I think I am missing something obvious here ;-). > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > t/t1300-config.sh | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > > index f4e2752134..1e953a0fc2 100755 > > --- a/t/t1300-config.sh > > +++ b/t/t1300-config.sh > > @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' ' > > test_must_fail git config --file=linktolinktonada --list > > ' > > > > -test_expect_success 'check split_cmdline return' " > > +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' " > > git config alias.split-cmdline-fix 'echo \"' && > > test_must_fail git split-cmdline-fix && > > echo foo > foo && > > @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' ' > > test_cmp expect actual > > ' > > Looking at this first test, for example, I see two places where we > modify the configuration file: > > - git config alias.split-cmdline-fix 'echo \"' > - git config branch.main.mergeoptions 'echo \"' > > I think I am missing some detail about why we can't do this when we have > extensions enabled? The issue is not directly visible in the tests I'm amending here, but happens in the setup code. What we do is to overwrite the repository's config like this: ``` cat > .git/config << EOF [beta] ; silly comment # another comment noIndent= sillyValue ; 'nother silly comment # empty line ; comment haha ="beta" # last silly comment haha = hello haha = bello [nextSection] noNewline = ouch EOF ``` The problem here is that we drop any extensions that the repository has been initialized with originally. This seems to work alright in the context of SHA256 repositories. But with the reftable backend this pattern will cause test failures because the discarded "refStorage" extension will make us assume that the repostiory uses the "files" backend instead of the "reftable" backend. And that starts to go downhill quite fast when trying to read or write refs. A "proper" fix for this issue would be to rewrite tests such that we know to retain those extensions. But I'm just not sure whether that is really worth it, mostly because the system under test is at a lower level and thus shouldn't care about repository extensions. After all, extensions build on top of our config code. Patrick
On Tue, Jan 09, 2024 at 02:35:29PM -0500, Eric Sunshine wrote: > On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote: > > The t1300 test suite exercises the git-config(1) tool. To do so we > > overwrite ".git/config" to contain custom contents. While this is easy > > enough to do, it may create problems when using a non-default repository > > format because we also overwrite the repository format version as well > > as any potential extensions. > > > > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the > > problem. An alternative would be to carry over mandatory config keys > > into the rewritten config file. But the effort does not seem worth it > > given that the system under test is git-config(1), which is at a lower > > level than the repository format. > > If I'm understanding correctly, with the approach taken by this patch, > won't we undesirably lose some git-config test coverage if the > file-based backend is ever retired, or if tests specific to it are > ever disabled by default? As such, it seems like the alternative "fix" > you mention above would be preferable to ensure that coverage of > git-config doesn't get diluted. > > Or am I misunderstanding something? A valid remark indeed, even though this is thinking quite far into the future. I'll investigate how much of a pain it would be to instead "do the right thing" and retain the repositroy format version as well as extensions. Patrick
diff --git a/t/t1300-config.sh b/t/t1300-config.sh index f4e2752134..1e953a0fc2 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' ' test_must_fail git config --file=linktolinktonada --list ' -test_expect_success 'check split_cmdline return' " +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' " git config alias.split-cmdline-fix 'echo \"' && test_must_fail git split-cmdline-fix && echo foo > foo && @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' ' test_cmp expect actual ' -test_expect_success 'aliases can be CamelCased' ' +test_expect_success DEFAULT_REPO_FORMAT 'aliases can be CamelCased' ' test_config alias.CamelCased "rev-parse HEAD" && git CamelCased >out && git rev-parse HEAD >expect && @@ -2051,7 +2051,7 @@ test_expect_success '--show-origin stdin with file include' ' test_cmp expect output ' -test_expect_success '--show-origin blob' ' +test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob' ' blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") && cat >expect <<-EOF && blob:$blob user.custom=true @@ -2060,7 +2060,7 @@ test_expect_success '--show-origin blob' ' test_cmp expect output ' -test_expect_success '--show-origin blob ref' ' +test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob ref' ' cat >expect <<-\EOF && blob:main:custom.conf user.custom=true EOF
The t1300 test suite exercises the git-config(1) tool. To do so we overwrite ".git/config" to contain custom contents. While this is easy enough to do, it may create problems when using a non-default repository format because we also overwrite the repository format version as well as any potential extensions. Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the problem. An alternative would be to carry over mandatory config keys into the rewritten config file. But the effort does not seem worth it given that the system under test is git-config(1), which is at a lower level than the repository format. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t1300-config.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)