git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Emily Shaffer <emilyshaffer@google.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Jonathan Nieder" <jrnieder@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jeff King" <peff@peff.net>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH v2 5/7] config: plumb --fixed-value into config API
Date: Wed, 25 Nov 2020 10:41:59 -0500
Message-ID: <d18a2f7f-67e9-05eb-95d1-ce0b4bd0b187@gmail.com> (raw)
In-Reply-To: <20201123222123.GE499823@google.com>

On 11/23/2020 5:21 PM, Emily Shaffer wrote:
> 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.

>> @@ -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'.

>> +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.

>> +	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.)

>> +	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.

>> +
>> +	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.

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

  parent reply	other threads:[~2020-11-25 15:54 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19 15:52 [PATCH 0/7] config: add --literal-value option Derrick Stolee via GitGitGadget
2020-11-19 15:52 ` [PATCH 1/7] t1300: test "set all" mode with value_regex Derrick Stolee via GitGitGadget
2020-11-19 22:24   ` Junio C Hamano
2020-11-20  2:09     ` brian m. carlson
2020-11-20  2:33       ` Junio C Hamano
2020-11-20 18:39     ` Jeff King
2020-11-20 22:35       ` Junio C Hamano
2020-11-21 22:27       ` brian m. carlson
2020-11-22  3:31         ` Junio C Hamano
2020-11-24  2:38           ` Jeff King
2020-11-24 19:43             ` Junio C Hamano
2020-11-19 15:52 ` [PATCH 2/7] t1300: add test for --replace-all " Derrick Stolee via GitGitGadget
2020-11-19 15:52 ` [PATCH 3/7] config: convert multi_replace to flags Derrick Stolee via GitGitGadget
2020-11-19 22:32   ` Junio C Hamano
2020-11-19 15:52 ` [PATCH 4/7] config: add --literal-value option, un-implemented Derrick Stolee via GitGitGadget
2020-11-19 22:42   ` Junio C Hamano
2020-11-20  6:35   ` Martin Ågren
2020-11-19 15:52 ` [PATCH 5/7] config: plumb --literal-value into config API Derrick Stolee via GitGitGadget
2020-11-19 22:45   ` Junio C Hamano
2020-11-19 15:52 ` [PATCH 6/7] config: implement --literal-value with --get* Derrick Stolee via GitGitGadget
2020-11-19 15:52 ` [PATCH 7/7] maintenance: use 'git config --literal-value' Derrick Stolee via GitGitGadget
2020-11-19 23:17   ` Junio C Hamano
2020-11-20 13:19 ` [PATCH 0/7] config: add --literal-value option Ævar Arnfjörð Bjarmason
2020-11-20 13:23   ` Derrick Stolee
2020-11-20 18:30     ` Junio C Hamano
2020-11-20 18:51       ` Derrick Stolee
2020-11-20 21:52         ` Junio C Hamano
2020-11-24 12:35     ` Ævar Arnfjörð Bjarmason
2020-11-23 16:05 ` [PATCH v2 0/7] config: add --fixed-value option Derrick Stolee via GitGitGadget
2020-11-23 16:05   ` [PATCH v2 1/7] t1300: test "set all" mode with value_regex Derrick Stolee via GitGitGadget
2020-11-23 19:37     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 2/7] t1300: add test for --replace-all " Derrick Stolee via GitGitGadget
2020-11-23 19:40     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 3/7] config: convert multi_replace to flags Derrick Stolee via GitGitGadget
2020-11-23 21:43     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 4/7] config: add --fixed-value option, un-implemented Derrick Stolee via GitGitGadget
2020-11-23 19:37     ` Junio C Hamano
2020-11-23 21:51     ` Emily Shaffer
2020-11-23 22:41       ` Junio C Hamano
2020-11-25 14:08         ` Derrick Stolee
2020-11-25 17:22           ` Derrick Stolee
2020-11-25 17:28           ` Eric Sunshine
2020-11-25 19:30             ` Junio C Hamano
2020-11-25 19:29           ` Junio C Hamano
2020-11-23 16:05   ` [PATCH v2 5/7] config: plumb --fixed-value into config API Derrick Stolee via GitGitGadget
2020-11-23 22:21     ` Emily Shaffer
2020-11-24  0:52       ` Eric Sunshine
2020-11-25 15:41       ` Derrick Stolee [this message]
2020-11-25 17:55         ` Eric Sunshine
2020-11-23 16:05   ` [PATCH v2 6/7] config: implement --fixed-value with --get* Derrick Stolee via GitGitGadget
2020-11-23 19:53     ` Junio C Hamano
2020-11-23 22:43     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 7/7] maintenance: use 'git config --fixed-value' Derrick Stolee via GitGitGadget
2020-11-23 21:39     ` Junio C Hamano
2020-11-23 22:48     ` Emily Shaffer
2020-11-23 23:27       ` Junio C Hamano
2020-11-23 19:33   ` [PATCH v2 0/7] config: add --fixed-value option Junio C Hamano
2020-11-25 22:12   ` [PATCH v3 0/8] " Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 1/8] config: convert multi_replace to flags Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 2/8] config: replace 'value_regex' with 'value_pattern' Derrick Stolee via GitGitGadget
2020-11-25 22:50       ` Eric Sunshine
2020-11-25 22:12     ` [PATCH v3 3/8] t1300: test "set all" mode with value-pattern Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 4/8] t1300: add test for --replace-all " Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 5/8] config: add --fixed-value option, un-implemented Derrick Stolee via GitGitGadget
2020-11-25 23:04       ` Eric Sunshine
2020-11-25 22:12     ` [PATCH v3 6/8] config: plumb --fixed-value into config API Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 7/8] config: implement --fixed-value with --get* Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 8/8] maintenance: use 'git config --fixed-value' Derrick Stolee via GitGitGadget
2020-11-25 23:09       ` Junio C Hamano
2020-11-25 23:00     ` [PATCH v3 0/8] config: add --fixed-value option Junio C Hamano
2020-11-26 11:17       ` Derrick Stolee
2020-12-01  4:45         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d18a2f7f-67e9-05eb-95d1-ce0b4bd0b187@gmail.com \
    --to=stolee@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jrnieder@gmail.com \
    --cc=martin.agren@gmail.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git