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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2020, 02:33 UTC
Message-ID: <xmqq8sawvjsp.fsf@gitster.c.googlers.com>
In-Reply-To: <20201120020924.GF389879@camp.crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 26 quoted lines

> On 2020-11-19 at 22:24:33, Junio C Hamano wrote:
>> "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.
>
> I haven't looked at the code, but if you're just using git config in a
> test, then overwriting the config file shouldn't be a problem with
> SHA-256.  If you're trying to read or write objects or the index, then
> that's definitely a problem, and you'll definitely notice exciting
> failures if you do that.

Yes, that is exactly what worries me.

And I was trolling for ideas from those on cc: list to come up with a better convention to test the behaviour of "git config". I think taking "git config --list [--file FILE]" before and after the ops of interest and basing the good/bad decision on the difference would be a saner approach. It obviously relies on "git config --list" to be working correctly, which the current approach that uses hardcoded "initial" state and expects result that is byte-for-byte identical to a hardcoded file contents after the ops does not have to rely on, but at the same time, the current approach assumes too much on the actual file format (e.g. no user or script would care how "key = val" line is indented, but the current approach insists that they are indented exactly with one tab), so overall it may even be an improvement.

Thanks.