Re: [PATCH 1/6] t1300: mark tests to require default repo format

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 10, 2024, 07:15 UTC
Message-ID: <ZZ5EBojTbQ4pyGx-@tanuki>
In-Reply-To: <ZZ2TdYlcrLN9zckR@nand.local>

On Tue, Jan 09, 2024 at 01:41:57PM -0500, Taylor Blau wrote:

Show 45 quoted lines

> 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