Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

From: Jeff King <peff@peff.net>
Date: Nov 20, 2020, 18:39 UTC
Message-ID: <20201120183903.GA320614@coredump.intra.peff.net>
In-Reply-To: <xmqqo8jtvvby.fsf@gitster.c.googlers.com>

On Thu, Nov 19, 2020 at 02:24:33PM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> "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