Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 29, 2024, 10:32 UTC
Message-ID: <CAP8UFD2ZoV74jx3B_adwYENGtp9fCH_oVjW4QJXTZQJ0=_aeEA@mail.gmail.com>
In-Reply-To: <ZbDP4ntRqrxX08yk@tanuki>

On Wed, Jan 24, 2024 at 9:52 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 39 quoted lines

>
> On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:
> > On Wed, Jan 10, 2024 at 10:01 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.
> >
> > Here "we" means "tests in t1300" I guess.
> >
> > > 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.
> >
> > But I am not sure that "we" in the above sentence is also "tests in
> > t1300". I think overwriting the repo format version and potential
> > extensions is done by other tests, right? Anyway it would be nice to
> > clarify this.
> >
> > > With the upcoming "reftable" ref backend
> > > the result is that we may try to access refs via the "files" backend
> > > even though the repository has been initialized with the "reftable"
> > > backend.
> >
> > Not sure here also what "we" is. When could refs be accessed via the
> > "files" backend even though the repo was initialized with the
> > "reftable" backend?
>
> Yeah, I've rephrased all of these to sey "the tests" or something
> similar.
>
> > Does this mean that some of the tests in t1300 try to access refs via
> > the "files" backend while we may want to run all the tests using the
> > reftable backend?
>
> Exactly. We overwrite the ".git/config", which contains the "refStorage"
> extension that tells us to use the "reftable" backend. So the extension
> is gone, and thus Git would fall back to use the "files" backend
> instead, which will fail.

Let's take a look at this test:

test_expect_success 'check split_cmdline return' "
    git config alias.split-cmdline-fix 'echo \"' &&
    test_must_fail git split-cmdline-fix &&
    echo foo > foo &&
    git add foo &&
    git commit -m 'initial commit' &&
    git config branch.main.mergeoptions 'echo \"' &&
    test_must_fail git merge main
"

I don't really see how it overwrites anything. When putting some debug commands before and after that test, it looks like the config file contains the following before that test:

---
[section "sub=section"]
       val1 = foo=bar
       val2 = foo\nbar
       val3 = \n\n
       val4 =
       val5
[section]
       val = foo \t  bar
---

and the following after that test:

---
[section "sub=section"]
       val1 = foo=bar
       val2 = foo\nbar
       val3 = \n\n
       val4 =
       val5
[section]
       val = foo \t  bar
[alias]
       split-cmdline-fix = echo \"
[branch "main"]
       mergeoptions = echo \"
---

So it doesn't look like it overwrites anything. To me it just adds stuff at the end of the file.

Show 12 quoted lines

> > > Refactor tests which access the refdb to be more robust by using their
> > > own separate repositories, which allows us to be more careful and not
> > > discard required extensions.
> >
> > Not sure what exactly is discarding extensions. Also robust is not
> > very clear. It would be better to give at least an example of how
> > things could fail.
>
> Hm. I don't really know how to phrase this better. The preceding
> paragraph already explains why we're discarding the extension and what
> the consequence is. I added a sentence saying ", which will cause
> failures when trying to access any refs."

To me the preceding paragraph said that we are overwriting the config file, but I just don't see how for example the above test overwrites anything. So maybe I am missing something obvious, or maybe you don't quite mean "overwrite", but I don't see how the extension would be discarded by the test which only seems to add stuff.