Re: [PATCH v2 5/7] config: plumb --fixed-value into config API

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 25, 2020, 15:41 UTC
Message-ID: <d18a2f7f-67e9-05eb-95d1-ce0b4bd0b187@gmail.com>
In-Reply-To: <20201123222123.GE499823@google.com>

On 11/23/2020 5:21 PM, Emily Shaffer wrote:

Show 7 quoted lines

> On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote:
>> +
>> +		flags = CONFIG_FLAGS_FIXED_VALUE;
> 
> I wonder whether using |= here will save someone from a headache later,
> when they want to add another flag value or move the
> CONFIG_FLAGS_MULTI_REPLACE calculation out of the tail calls below.

Good catch.

Show 10 quoted lines

>> @@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>>  			store.value_regex = NULL;
>>  		else if (value_regex == CONFIG_REGEX_NONE)
>>  			store.value_regex = CONFIG_REGEX_NONE;
>> +		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
>> +			store.literal_value = value_regex;
> 
> Ah, so we use .literal_value instead of pulling the string from
> value_regex because value_regex undergoes some special parsing and is
> packed into a regex_t instead.

Reminds me to call this 'fixed_value'.

Show 6 quoted lines

>> +test_expect_success '--fixed-value uses exact string matching' '
>> +	GLOB="a+b*c?d[e]f.g" &&
>> +	rm -f initial &&
> This tells me that when you used 'initial' earlier you probably should
> have done something like 'test_when_finished rm initial' in this test
> and your earlier ones. Whoops.

Good idea. Will do.

Show 7 quoted lines

>> +	cp initial config &&
>> +	git config --file=config --fixed-value fixed.test bogus "$GLOB" &&
>> +	git config --file=config --list >actual &&
>> +	printf "fixed.test=bogus\n" >expect &&
> It is jarring to me to see a printf here when everywhere else we use
> heredocs. 'git grep' tells me it's not unheard of, but it looks like
> those are cases where the whole file doesn't use heredocs.

I can use a heredoc just to be consistent.

(To also respond to Eric's message, I tend to use printf instead of echo because echo starts a process while printf does not.)

Show 10 quoted lines

>> +	test_cmp expect actual &&
>> +
>> +	cp initial config &&
>> +	test_must_fail git config --file=config --unset fixed.test "$GLOB" &&
>> +	git config --file=config --fixed-value --unset fixed.test "$GLOB" &&
>> +	test_must_fail git config --file=config fixed.test &&
> Is this one supposed to verify that there is a 'fixed.test' value
> already in 'config'? I'd prefer to see that explicitly checked with 'git
> config --get' rather than watching for a symptom, that is, fail to set.
> This comment applies to the next case too.

If no value is provided, then 'git config <name>' _is_ a query. It's not a failed set. 'git config <name> ""' would be the way to try and set the value to an empty string.

Show 17 quoted lines

>> +
>> +	cp initial config &&
>> +	test_must_fail git config --file=config --unset-all fixed.test "$GLOB" &&
>> +	git config --file=config --fixed-value --unset-all fixed.test "$GLOB" &&
>> +	test_must_fail git config --file=config fixed.test &&
>> +
>> +	cp initial config &&
>> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
>> +	git config --file=config --list >actual &&
>> +	cat >expect <<-EOF &&
>> +	fixed.test=$GLOB
>> +	fixed.test=bogus
>> +	EOF
>> +	test_cmp expect actual &&
> Hm, isn't this the same functionality as the tests you added at the
> beginning of this series? I guess you are setting up for the last case
> with --replace-all...

This is specifically demonstrating the difference that --fixed-value provides. The tests here show "the match doesn't work by default, but then works with --fixed-value". I'll make this clearer in the commit message.

Show 15 quoted lines

>> +
>> +	cp initial config &&
>> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
>> +	git config --file=config --list >actual &&
>> +	cat >expect <<-EOF &&
>> +	fixed.test=$GLOB
>> +	fixed.test=bogus
>> +	EOF
>> +	test_cmp expect actual &&
> 
> Is this one identical to the previous one? I think it is, but if it
> isn't and I can't tell, all the more reason that each case here should
> either be labeled with a comment or separated into its own test. (Bonus
> - you could extend the individual tests from patch 1 to make sure they
> work correctly with --fixed-value too ;) )

Yes, accidentally over-zealous copy/paste.

I'm less in favor of splitting the tests, since they rely on a shared initial config. Any failure in one part of this test is likely to also fail the rest of the commands, so grouping them by this toggle makes sense to me.

Thanks, -Stolee