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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]