* [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing @ 2021-04-08 13:34 Ævar Arnfjörð Bjarmason 2021-04-08 13:34 ` [PATCH 1/5] config.c: add a comment about why value=NULL is true Ævar Arnfjörð Bjarmason ` (4 more replies) 0 siblings, 5 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Ævar Arnfjörð Bjarmason Refactor some of our internal C code to use a new git_config_tristate() functino instead of checking for "auto" or "bool" in several places. This isn't just Yet Another Ævar Series, I'm using this to re-roll my outstanding userdiff series, the start of that topic was a partial refactor of userdiff.c which is better done here. While writing this I discovered that the recently added --bool-or-type option added in https://lore.kernel.org/git/pull.781.v18.git.git.1599895268433.gitgitgadget@gmail.com/ didn't have any tests (and we didn't notice in 18! iterations of it:) 2/5 adds that, 1/5 adds a comment on some bool parsing code that's puzzled me for the Nth time. Ævar Arnfjörð Bjarmason (5): config.c: add a comment about why value=NULL is true config tests: test for --bool-or-str git-config: document --bool-or-str and --type=bool-or-str config.c: add a "tristate" helper config: add --type=bool-or-auto switch Documentation/git-config.txt | 7 ++ builtin/config.c | 19 ++++++ builtin/log.c | 13 ++-- compat/mingw.c | 6 +- config.c | 20 ++++++ config.h | 12 ++++ http.c | 5 +- mergetools/meld | 2 +- t/t1300-config.sh | 121 +++++++++++++++++++++++++++++++++++ userdiff.c | 6 +- 10 files changed, 195 insertions(+), 16 deletions(-) -- 2.31.1.527.g9b8f7de2547 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/5] config.c: add a comment about why value=NULL is true 2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 ` Ævar Arnfjörð Bjarmason 2021-04-08 18:10 ` Junio C Hamano 2021-04-08 13:34 ` [PATCH 2/5] config tests: test for --bool-or-str Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Ævar Arnfjörð Bjarmason It's not very intuitive that git_parse_maybe_bool_text() would consider NULL to be a true value. Add a small comment about it. See a789ca70e7 (config: teach "git -c" to recognize an empty string, 2014-08-04) for the behavior of "git -c", but we'll end up here in both the config file parsing and command-line parsing cases. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- config.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config.c b/config.c index 6428393a41..fc28dbd97c 100644 --- a/config.c +++ b/config.c @@ -1229,6 +1229,10 @@ ssize_t git_config_ssize_t(const char *name, const char *value) static int git_parse_maybe_bool_text(const char *value) { if (!value) + /* + * "[foo]\nbar\n" and "-c foo.bar" on the command-line + * are true. + */ return 1; if (!*value) return 0; -- 2.31.1.527.g9b8f7de2547 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true 2021-04-08 13:34 ` [PATCH 1/5] config.c: add a comment about why value=NULL is true Ævar Arnfjörð Bjarmason @ 2021-04-08 18:10 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2021-04-08 18:10 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Subject: Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true A few unproductive nitpicks. The question "why does '[core] ignorecase' mean core.ignorecase is true?" has only a single answer, which is "because that is how we designed (loosely imitating .ini format) and implemented." > It's not very intuitive that git_parse_maybe_bool_text() would > consider NULL to be a true value. Add a small comment about it. Do you mean if (!value) return 1; is unintuitive? Or do you mean [core] ignorecase being a valid way to say "the filesystem ignores case differences"? The answer to the question is crucial to justify the title. Only when "the config syntax is perfectly understandable, the way the logic to parse that syntax is expressed in C is not necessarily intuitive" is true, the comment makes sense, I would think. > See a789ca70e7 (config: teach "git -c" to recognize an empty string, > 2014-08-04) for the behavior of "git -c", but we'll end up here in > both the config file parsing and command-line parsing cases. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > config.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/config.c b/config.c > index 6428393a41..fc28dbd97c 100644 > --- a/config.c > +++ b/config.c > @@ -1229,6 +1229,10 @@ ssize_t git_config_ssize_t(const char *name, const char *value) > static int git_parse_maybe_bool_text(const char *value) > { > if (!value) > + /* > + * "[foo]\nbar\n" and "-c foo.bar" on the command-line > + * are true. > + */ This is a "we do not explain why '[foo] bar' is true, but to be prepared to parse it, we check if value is NULL" comment. And I think it is a good one, except that I do not see the point of writing "\n" at all. Replacing them with spaces would make it far easier to read. /* * "[foo] bar" in the configuration file, and * "git -c foo.bar" on the command-line, mean * that the variable foo.bar is true. A NULL is * passed as 'value' in these cases. */ > return 1; > if (!*value) > return 0; ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/5] config tests: test for --bool-or-str 2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason 2021-04-08 13:34 ` [PATCH 1/5] config.c: add a comment about why value=NULL is true Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 ` Ævar Arnfjörð Bjarmason 2021-04-08 18:21 ` Junio C Hamano 2021-04-08 13:34 ` [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Ævar Arnfjörð Bjarmason Add the missing tests for the --bool-or-str code added in dbd8c09bfe (mergetool: allow auto-merge for meld to follow the vim-diff behavior, 2020-05-07). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index e0dd5d65ce..a002ec5644 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' ' test_cmp expect actual ' +test_expect_success 'get --bool-or-str' ' + cat >.git/config <<-\EOF && + [bool] + true1 + true2 = true + true3 = TRUE + true4 = yes + true5 = YES + true6 = on + true7 = ON + false1 = + false2 = false + false3 = FALSE + false4 = no + false5 = NO + false6 = off + false7 = OFF + [int] + int1 = 0 + int2 = 1 + int3 = -1 + [string] + string1 = hello + string2 = there you + EOF + cat >expect <<-\EOF && + true + true + true + true + true + true + true + false + false + false + false + false + false + false + false + false + true + true + hello + there you + EOF + { + git config --type=bool-or-str bool.true1 && + git config --bool-or-str bool.true2 && + git config --bool-or-str bool.true3 && + git config --bool-or-str bool.true4 && + git config --bool-or-str bool.true5 && + git config --bool-or-str bool.true6 && + git config --bool-or-str bool.true7 && + git config --bool-or-str bool.false1 && + git config --bool-or-str bool.false2 && + git config --bool-or-str bool.false3 && + git config --bool-or-str bool.false4 && + git config --bool-or-str bool.false5 && + git config --bool-or-str bool.false6 && + git config --bool-or-str bool.false6 && + git config --bool-or-str bool.false7 && + git config --bool-or-str int.int1 && + git config --bool-or-str int.int2 && + git config --bool-or-str int.int3 && + git config --bool-or-str string.string1 && + git config --bool-or-str string.string2 + } >actual && + test_cmp expect actual +' + cat >expect <<\EOF [bool] true1 = true -- 2.31.1.527.g9b8f7de2547 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] config tests: test for --bool-or-str 2021-04-08 13:34 ` [PATCH 2/5] config tests: test for --bool-or-str Ævar Arnfjörð Bjarmason @ 2021-04-08 18:21 ` Junio C Hamano 2021-04-08 23:11 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2021-04-08 18:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Add the missing tests for the --bool-or-str code added in > dbd8c09bfe (mergetool: allow auto-merge for meld to follow the > vim-diff behavior, 2020-05-07). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index e0dd5d65ce..a002ec5644 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' ' > test_cmp expect actual > ' > > +test_expect_success 'get --bool-or-str' ' > + cat >.git/config <<-\EOF && > + [bool] > + true1 > + true2 = true > + true3 = TRUE > + true4 = yes > + true5 = YES > + true6 = on > + true7 = ON > + false1 = > + false2 = false > + false3 = FALSE > + false4 = no > + false5 = NO > + false6 = off > + false7 = OFF > + [int] > + int1 = 0 > + int2 = 1 > + int3 = -1 > + [string] > + string1 = hello > + string2 = there you > + EOF That's fairly complete set (but misses common permutations like "Yes"). I am not sure, if we try "true" and "TRUE", if it is worth to check yes/YES and others, but at the same time, I do not know if reducing the permutations tested would improve the readability, runtime and/or maintainability of the test. > + cat >expect <<-\EOF && > + true > + true > + true > + true > + true > + true > + true > + false > + false > + false > + false > + false > + false > + false > + false > + false > + true > + true > + hello > + there you > + EOF The "right answer" is hard to read and maintain. Can we immediately spot what happened to int.int3 in this output, for example? Perhaps with something like inspect_config () { name=$1 shift printf "%s %s\n" $(git config "$@" "$name") "$name" } we can make these lines to say int.int1 false int.int2 true int.int3 true string.string1 hello string.string2 there you to make them easier to follow? Without such a hint, I would expect that a failure output from test_cmp at the end would be very hard to grok, especially with so many permutations tested that produces runs of "true" and "false". > + { > + git config --type=bool-or-str bool.true1 && > + git config --bool-or-str bool.true2 && This is a bit curious. We do not do full permutation between --type=bool-or-str and --bool-or-str here. We just check both would work only once. Feels a bit inconsistent. My gut-feeling vote is to just try true/TRUE for case insensitivity and try all the other variants in lowercase, but I can go with the full permutation if you strongly prefer it. > ... > + git config --bool-or-str int.int1 && > + git config --bool-or-str int.int2 && > + git config --bool-or-str int.int3 && > + git config --bool-or-str string.string1 && > + git config --bool-or-str string.string2 > + } >actual && > + test_cmp expect actual > +' Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] config tests: test for --bool-or-str 2021-04-08 18:21 ` Junio C Hamano @ 2021-04-08 23:11 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 23:11 UTC (permalink / raw) To: Junio C Hamano Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar On Thu, Apr 08 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Add the missing tests for the --bool-or-str code added in >> dbd8c09bfe (mergetool: allow auto-merge for meld to follow the >> vim-diff behavior, 2020-05-07). >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 72 insertions(+) >> >> diff --git a/t/t1300-config.sh b/t/t1300-config.sh >> index e0dd5d65ce..a002ec5644 100755 >> --- a/t/t1300-config.sh >> +++ b/t/t1300-config.sh >> @@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' ' >> test_cmp expect actual >> ' >> >> +test_expect_success 'get --bool-or-str' ' >> + cat >.git/config <<-\EOF && >> + [bool] >> + true1 >> + true2 = true >> + true3 = TRUE >> + true4 = yes >> + true5 = YES >> + true6 = on >> + true7 = ON >> + false1 = >> + false2 = false >> + false3 = FALSE >> + false4 = no >> + false5 = NO >> + false6 = off >> + false7 = OFF >> + [int] >> + int1 = 0 >> + int2 = 1 >> + int3 = -1 >> + [string] >> + string1 = hello >> + string2 = there you >> + EOF > > That's fairly complete set (but misses common permutations like > "Yes"). I am not sure, if we try "true" and "TRUE", if it is worth > to check yes/YES and others, but at the same time, I do not know if > reducing the permutations tested would improve the readability, > runtime and/or maintainability of the test. Sure, I was trying to do just enough to cover strcasecmp(). I don't think we need to do black-box testing here. >> + cat >expect <<-\EOF && >> + true >> + true >> + true >> + true >> + true >> + true >> + true >> + false >> + false >> + false >> + false >> + false >> + false >> + false >> + false >> + false >> + true >> + true >> + hello >> + there you >> + EOF > > The "right answer" is hard to read and maintain. Can we immediately > spot what happened to int.int3 in this output, for example? > > Perhaps with something like > > inspect_config () { > name=$1 > shift > printf "%s %s\n" $(git config "$@" "$name") "$name" > } > > we can make these lines to say > > int.int1 false > int.int2 true > int.int3 true > string.string1 hello > string.string2 there you > > to make them easier to follow? Without such a hint, I would expect > that a failure output from test_cmp at the end would be very hard to > grok, especially with so many permutations tested that produces runs > of "true" and "false". It's a general established pattern in t1300-config.sh used by several other existing tests, e.g. the one for bool, path etc. I'd rather not get into a general refactoring of that file. >> + { >> + git config --type=bool-or-str bool.true1 && >> + git config --bool-or-str bool.true2 && > > This is a bit curious. We do not do full permutation between > --type=bool-or-str and --bool-or-str here. We just check both > would work only once. Feels a bit inconsistent. Yeah, I was just trying to stick a --type=X v.s. --X test somewhere. I can add it to another test_expect_success or something. > My gut-feeling vote is to just try true/TRUE for case insensitivity > and try all the other variants in lowercase, but I can go with the > full permutation if you strongly prefer it. > >> ... >> + git config --bool-or-str int.int1 && >> + git config --bool-or-str int.int2 && >> + git config --bool-or-str int.int3 && >> + git config --bool-or-str string.string1 && >> + git config --bool-or-str string.string2 >> + } >actual && >> + test_cmp expect actual >> +' > > Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str 2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason 2021-04-08 13:34 ` [PATCH 1/5] config.c: add a comment about why value=NULL is true Ævar Arnfjörð Bjarmason 2021-04-08 13:34 ` [PATCH 2/5] config tests: test for --bool-or-str Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 ` Ævar Arnfjörð Bjarmason 2021-04-08 18:22 ` Junio C Hamano 2021-04-08 13:34 ` [PATCH 4/5] config.c: add a "tristate" helper Ævar Arnfjörð Bjarmason 2021-04-08 13:34 ` [PATCH 5/5] config: add --type=bool-or-auto switch Ævar Arnfjörð Bjarmason 4 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Ævar Arnfjörð Bjarmason Document the new "bool-or-str" facility added in dbd8c09bfe (mergetool: allow auto-merge for meld to follow the vim-diff behavior, 2020-05-07). Unfortunately that commit also added a --bool-or-str option, even though we've preferred to deprecate that form ever since fb0dc3bac1 (builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`, 2018-04-18). Since we've got it already let's document it along with the preferred --type=* form, and change our own code to use the --type=bool-or-str form over --bool-or-str. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-config.txt | 3 +++ mergetools/meld | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 4b4cc5c5e8..4ae9ef210c 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -187,6 +187,8 @@ Valid `<type>`'s include: 1073741824 upon input. - 'bool-or-int': canonicalize according to either 'bool' or 'int', as described above. +- 'bool-or-str: canonicalize according to either 'bool' (as described + above), or emit the value as-is. - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and `~user` to the home directory for the specified user. This specifier has no effect when setting the value (but you can use `git config section.variable @@ -202,6 +204,7 @@ Valid `<type>`'s include: --bool:: --int:: --bool-or-int:: +--bool-or-str:: --path:: --expiry-date:: Historical options for selecting a type specifier. Prefer instead `--type` diff --git a/mergetools/meld b/mergetools/meld index aab4ebb935..8386e0574e 100644 --- a/mergetools/meld +++ b/mergetools/meld @@ -59,7 +59,7 @@ check_meld_for_features () { if test -z "$meld_use_auto_merge_option" then meld_use_auto_merge_option=$( - git config --bool-or-str mergetool.meld.useAutoMerge + git config --type=bool-or-str mergetool.meld.useAutoMerge ) case "$meld_use_auto_merge_option" in true | false) -- 2.31.1.527.g9b8f7de2547 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str 2021-04-08 13:34 ` [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str Ævar Arnfjörð Bjarmason @ 2021-04-08 18:22 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2021-04-08 18:22 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Document the new "bool-or-str" facility added in > dbd8c09bfe (mergetool: allow auto-merge for meld to follow the > vim-diff behavior, 2020-05-07). > > Unfortunately that commit also added a --bool-or-str option, even > though we've preferred to deprecate that form ever since > fb0dc3bac1 (builtin/config.c: support `--type=<type>` as preferred > alias for `--<type>`, 2018-04-18). > > Since we've got it already let's document it along with the preferred > --type=* form, and change our own code to use the --type=bool-or-str > form over --bool-or-str. It was a mistake to introduce a new option that is immediately deprecated X-<. Thanks for spotting and correcting. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > Documentation/git-config.txt | 3 +++ > mergetools/meld | 2 +- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index 4b4cc5c5e8..4ae9ef210c 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -187,6 +187,8 @@ Valid `<type>`'s include: > 1073741824 upon input. > - 'bool-or-int': canonicalize according to either 'bool' or 'int', as described > above. > +- 'bool-or-str: canonicalize according to either 'bool' (as described > + above), or emit the value as-is. > - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and > `~user` to the home directory for the specified user. This specifier has no > effect when setting the value (but you can use `git config section.variable > @@ -202,6 +204,7 @@ Valid `<type>`'s include: > --bool:: > --int:: > --bool-or-int:: > +--bool-or-str:: > --path:: > --expiry-date:: > Historical options for selecting a type specifier. Prefer instead `--type` > diff --git a/mergetools/meld b/mergetools/meld > index aab4ebb935..8386e0574e 100644 > --- a/mergetools/meld > +++ b/mergetools/meld > @@ -59,7 +59,7 @@ check_meld_for_features () { > if test -z "$meld_use_auto_merge_option" > then > meld_use_auto_merge_option=$( > - git config --bool-or-str mergetool.meld.useAutoMerge > + git config --type=bool-or-str mergetool.meld.useAutoMerge > ) > case "$meld_use_auto_merge_option" in > true | false) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/5] config.c: add a "tristate" helper 2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-04-08 13:34 ` [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 ` Ævar Arnfjörð Bjarmason 2021-04-08 18:33 ` Junio C Hamano 2021-04-08 13:34 ` [PATCH 5/5] config: add --type=bool-or-auto switch Ævar Arnfjörð Bjarmason 4 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Ævar Arnfjörð Bjarmason Add "tristate" functions to go along with the "bool" functions and migrate the common pattern of checking if something is "bool" or "auto" in various places over to the new functions. We also have e.g. "repo_config_get_bool" and "config_error_nonbool". I'm not adding corresponding "tristate" functions as they're not needed by anything, but we could add those in the future if they are. I'm not migrating over "core.abbrev" parsing as part of this change. When "core.abbrev" was made optionally boolean in a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the "die if empty" code added in g48d5014dd4 (config.abbrev: document the new default that auto-scales, 2016-11-01) wasn't adjusted. It thus behaves unlike all other "maybe bool" config variables. I have a planned series to start adding some tests for "core.abbrev", but AFAICT there's not even a test for "core.abbrev=true", and I'd like to focus on thing that have no behavior change here, so let's leave it for now. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/log.c | 13 +++++++------ compat/mingw.c | 6 +++--- config.c | 16 ++++++++++++++++ config.h | 12 ++++++++++++ http.c | 5 +++-- userdiff.c | 6 ++---- 6 files changed, 43 insertions(+), 15 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 8acd285daf..0d945313d8 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -868,11 +868,12 @@ static int git_format_config(const char *var, const char *value, void *cb) return 0; } if (!strcmp(var, "format.numbered")) { - if (value && !strcasecmp(value, "auto")) { + int tristate = git_config_tristate(var, value); + if (tristate == 2) { auto_number = 1; return 0; } - numbered = git_config_bool(var, value); + numbered = tristate; auto_number = auto_number && numbered; return 0; } @@ -904,11 +905,11 @@ static int git_format_config(const char *var, const char *value, void *cb) if (!strcmp(var, "format.signaturefile")) return git_config_pathname(&signature_file, var, value); if (!strcmp(var, "format.coverletter")) { - if (value && !strcasecmp(value, "auto")) { + int tristate = git_config_tristate(var, value); + if (tristate == 2) config_cover_letter = COVER_AUTO; - return 0; - } - config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF; + else + config_cover_letter = tristate ? COVER_ON : COVER_OFF; return 0; } if (!strcmp(var, "format.outputdirectory")) diff --git a/compat/mingw.c b/compat/mingw.c index a43599841c..e6e85ae99a 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -247,11 +247,11 @@ int mingw_core_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "core.restrictinheritedhandles")) { - if (value && !strcasecmp(value, "auto")) + int tristate = git_config_tristate(var, value); + if (tristate == 2) core_restrict_inherited_handles = -1; else - core_restrict_inherited_handles = - git_config_bool(var, value); + core_restrict_inherited_handles = tristate; return 0; } diff --git a/config.c b/config.c index fc28dbd97c..74d2b2c0df 100644 --- a/config.c +++ b/config.c @@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value) return -1; } +int git_parse_maybe_tristate(const char *value) +{ + int v = git_parse_maybe_bool(value); + if (v < 0 && !strcasecmp(value, "auto")) + return 2; + return v; +} + int git_config_bool_or_int(const char *name, const char *value, int *is_bool) { int v = git_parse_maybe_bool_text(value); @@ -1268,6 +1276,14 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) return git_config_int(name, value); } +int git_config_tristate(const char *name, const char *value) +{ + int v = git_parse_maybe_tristate(value); + if (v < 0) + die(_("bad tristate config value '%s' for '%s'"), value, name); + return v; +} + int git_config_bool(const char *name, const char *value) { int v = git_parse_maybe_bool(value); diff --git a/config.h b/config.h index 19a9adbaa9..c5129e4392 100644 --- a/config.h +++ b/config.h @@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *); */ int git_parse_maybe_bool(const char *); +/** + * Same as `git_parse_maybe_bool`, except that "auto" is recognized and + * will return "2". + */ +int git_parse_maybe_tristate(const char *); + /** * Parse the string to an integer, including unit factors. Dies on error; * otherwise, returns the parsed result. @@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *); */ int git_config_bool(const char *, const char *); +/** + * Like git_config_bool() except "auto" is also recognized and will + * return "2" + */ +int git_config_tristate(const char *, const char *); + /** * Allocates and copies the value string into the `dest` parameter; if no * string is given, prints an error message and returns -1. diff --git a/http.c b/http.c index 406410f884..b54a232e90 100644 --- a/http.c +++ b/http.c @@ -406,10 +406,11 @@ static int http_options(const char *var, const char *value, void *cb) return git_config_string(&user_agent, var, value); if (!strcmp("http.emptyauth", var)) { - if (value && !strcmp("auto", value)) + int tristate = git_config_tristate(var, value); + if (tristate == 2) curl_empty_auth = -1; else - curl_empty_auth = git_config_bool(var, value); + curl_empty_auth = tristate; return 0; } diff --git a/userdiff.c b/userdiff.c index 3f81a2261c..7ff010961f 100644 --- a/userdiff.c +++ b/userdiff.c @@ -277,10 +277,8 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k, static int parse_tristate(int *b, const char *k, const char *v) { - if (v && !strcasecmp(v, "auto")) - *b = -1; - else - *b = git_config_bool(k, v); + int tristate = git_config_tristate(k, v); + *b = tristate == 2 ? -1 : tristate; return 0; } -- 2.31.1.527.g9b8f7de2547 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-08 13:34 ` [PATCH 4/5] config.c: add a "tristate" helper Ævar Arnfjörð Bjarmason @ 2021-04-08 18:33 ` Junio C Hamano 2021-04-08 23:23 ` Ævar Arnfjörð Bjarmason 2021-04-09 20:05 ` Jeff King 0 siblings, 2 replies; 21+ messages in thread From: Junio C Hamano @ 2021-04-08 18:33 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Add "tristate" functions to go along with the "bool" functions and > migrate the common pattern of checking if something is "bool" or > "auto" in various places over to the new functions. > > We also have e.g. "repo_config_get_bool" and > "config_error_nonbool". I'm not adding corresponding "tristate" > functions as they're not needed by anything, but we could add those in > the future if they are. > > I'm not migrating over "core.abbrev" parsing as part of this > change. When "core.abbrev" was made optionally boolean in > a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the > "die if empty" code added in g48d5014dd4 (config.abbrev: document the > new default that auto-scales, 2016-11-01) wasn't adjusted. It thus > behaves unlike all other "maybe bool" config variables. > > I have a planned series to start adding some tests for "core.abbrev", > but AFAICT there's not even a test for "core.abbrev=true", and I'd > like to focus on thing that have no behavior change here, so let's > leave it for now. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/log.c | 13 +++++++------ > compat/mingw.c | 6 +++--- > config.c | 16 ++++++++++++++++ > config.h | 12 ++++++++++++ > http.c | 5 +++-- > userdiff.c | 6 ++---- > 6 files changed, 43 insertions(+), 15 deletions(-) > diff --git a/config.c b/config.c > index fc28dbd97c..74d2b2c0df 100644 > --- a/config.c > +++ b/config.c > @@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value) > return -1; > } > > +int git_parse_maybe_tristate(const char *value) > +{ > + int v = git_parse_maybe_bool(value); > + if (v < 0 && !strcasecmp(value, "auto")) > + return 2; > + return v; > +} This is not parse_mayb_bool_text(), so "1" and "-1" written in the configuration file are "true", "0" is "false", like the "bool" case. I wonder if written without an unnecessary extra variable, i.e. if (value && !strcasecmp(value, "auto")) return 2; return git_parse_maybe_bool(value); is easier to follow, though, as it is quite clear that it is mostly the same as maybe_bool and the only difference is when "auto" is given. > +int git_config_tristate(const char *name, const char *value) > +{ > + int v = git_parse_maybe_tristate(value); > + if (v < 0) > + die(_("bad tristate config value '%s' for '%s'"), value, name); > + return v; > +} OK. > diff --git a/config.h b/config.h > index 19a9adbaa9..c5129e4392 100644 > --- a/config.h > +++ b/config.h > @@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *); > */ > int git_parse_maybe_bool(const char *); > > +/** > + * Same as `git_parse_maybe_bool`, except that "auto" is recognized and > + * will return "2". > + */ > +int git_parse_maybe_tristate(const char *); A false being 0 and a true being 1 is understandable for readers without symbolic constant, but "2" deserves to have a symbolic constant, doesn't it? > @@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *); > */ > int git_config_bool(const char *, const char *); > > +/** > + * Like git_config_bool() except "auto" is also recognized and will > + * return "2" > + */ > +int git_config_tristate(const char *, const char *); Likewise. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-08 18:33 ` Junio C Hamano @ 2021-04-08 23:23 ` Ævar Arnfjörð Bjarmason 2021-04-08 23:51 ` Junio C Hamano 2021-04-08 23:54 ` Junio C Hamano 2021-04-09 20:05 ` Jeff King 1 sibling, 2 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 23:23 UTC (permalink / raw) To: Junio C Hamano Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Jeff King On Thu, Apr 08 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Add "tristate" functions to go along with the "bool" functions and >> migrate the common pattern of checking if something is "bool" or >> "auto" in various places over to the new functions. >> >> We also have e.g. "repo_config_get_bool" and >> "config_error_nonbool". I'm not adding corresponding "tristate" >> functions as they're not needed by anything, but we could add those in >> the future if they are. >> >> I'm not migrating over "core.abbrev" parsing as part of this >> change. When "core.abbrev" was made optionally boolean in >> a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the >> "die if empty" code added in g48d5014dd4 (config.abbrev: document the >> new default that auto-scales, 2016-11-01) wasn't adjusted. It thus >> behaves unlike all other "maybe bool" config variables. >> >> I have a planned series to start adding some tests for "core.abbrev", >> but AFAICT there's not even a test for "core.abbrev=true", and I'd >> like to focus on thing that have no behavior change here, so let's >> leave it for now. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> builtin/log.c | 13 +++++++------ >> compat/mingw.c | 6 +++--- >> config.c | 16 ++++++++++++++++ >> config.h | 12 ++++++++++++ >> http.c | 5 +++-- >> userdiff.c | 6 ++---- >> 6 files changed, 43 insertions(+), 15 deletions(-) > >> diff --git a/config.c b/config.c >> index fc28dbd97c..74d2b2c0df 100644 >> --- a/config.c >> +++ b/config.c >> @@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value) >> return -1; >> } >> >> +int git_parse_maybe_tristate(const char *value) >> +{ >> + int v = git_parse_maybe_bool(value); >> + if (v < 0 && !strcasecmp(value, "auto")) >> + return 2; >> + return v; >> +} > > This is not parse_mayb_bool_text(), so "1" and "-1" written in the > configuration file are "true", "0" is "false", like the "bool" case. > > I wonder if written without an unnecessary extra variable, i.e. > > if (value && !strcasecmp(value, "auto")) > return 2; > return git_parse_maybe_bool(value); > > is easier to follow, though, as it is quite clear that it is mostly > the same as maybe_bool and the only difference is when "auto" is > given. I guess it could be either way around, I think it makes more sense to have git_parse_maybe_bool() handle as much as possible right from the get-go, since it already handles NULL if we call it first we just need to care about cases where it's looked at all the "is this bool-like" permutations and decided to reject it. >> +int git_config_tristate(const char *name, const char *value) >> +{ >> + int v = git_parse_maybe_tristate(value); >> + if (v < 0) >> + die(_("bad tristate config value '%s' for '%s'"), value, name); >> + return v; >> +} > > OK. > >> diff --git a/config.h b/config.h >> index 19a9adbaa9..c5129e4392 100644 >> --- a/config.h >> +++ b/config.h >> @@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *); >> */ >> int git_parse_maybe_bool(const char *); >> >> +/** >> + * Same as `git_parse_maybe_bool`, except that "auto" is recognized and >> + * will return "2". >> + */ >> +int git_parse_maybe_tristate(const char *); > > A false being 0 and a true being 1 is understandable for readers > without symbolic constant, but "2" deserves to have a symbolic > constant, doesn't it? > >> @@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *); >> */ >> int git_config_bool(const char *, const char *); >> >> +/** >> + * Like git_config_bool() except "auto" is also recognized and will >> + * return "2" >> + */ >> +int git_config_tristate(const char *, const char *); > > Likewise. I'd prefer to just make these "enum", which means we'll have the aid of the compiler in checking all the callsites, as in the patch-on-top (which I can squash appropriately, need to update the doc comments though) at the end of this E-Mail. It's a bit of boilerplate, and it's unfortunate that subset/supersets of enums aren't better supported in C (so e.g. you could have different types share some of the same label names), but I think being able to look at any given use and know the compiler is checking whether the case statements (there's no "default") are exhaustively enumerated is worth it. But I wasn't sure you'd prefet that, especially (and maybe I read too much into it) with me replacing -1 with OBJ_BAD in another topic, as GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD does here. In this case most of the callsites don't need to deal with the "BAD" value, but for other things in config.c and this sort of code in general if we're going to insist on "v < 0" over explicit labels the benefit of using enum/switch pretty much goes away. I mean, we could not do the enum/switch/case implementation and just have a "#define" to give "2" a pretty name, but at that point anything beyond that is pretty pointless, i.e. we can just make the function return an "int" if we're not hoping to have the compiler check the enum values. diff --git a/builtin/config.c b/builtin/config.c index 039a4f0961..a5d7efc8bc 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -268,11 +268,18 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value else strbuf_addstr(buf, v ? "true" : "false"); } else if (type == TYPE_BOOL_OR_AUTO) { - int v = git_config_tristate(key_, value_); - if (v == 2) + enum git_config_type_bool_or_auto v = git_config_tristate(key_, value_); + switch (v) { + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE: + strbuf_addstr(buf, "false"); + break; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE: + strbuf_addstr(buf, "true"); + break; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO: strbuf_addstr(buf, "auto"); - else - strbuf_addstr(buf, v ? "true" : "false"); + break; + } } else if (type == TYPE_PATH) { const char *v; if (git_config_pathname(&v, key_, value_) < 0) @@ -446,13 +453,17 @@ static char *normalize_value(const char *key, const char *value) return xstrdup(v ? "true" : "false"); } if (type == TYPE_BOOL_OR_AUTO) { - int v = git_parse_maybe_tristate(value); - if (v < 0) + enum git_config_type_maybe_bool_or_auto v = git_parse_maybe_tristate(value); + switch (v) { + case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD: return xstrdup(value); - else if (v == 2) - xstrdup("auto"); - else - return xstrdup(v ? "true" : "false"); + case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE: + return xstrdup("false"); + case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE: + return xstrdup("true"); + case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO: + return xstrdup("auto"); + } } if (type == TYPE_COLOR) { char v[COLOR_MAXLEN]; diff --git a/builtin/log.c b/builtin/log.c index 0d945313d8..f363c841a7 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -868,14 +868,17 @@ static int git_format_config(const char *var, const char *value, void *cb) return 0; } if (!strcmp(var, "format.numbered")) { - int tristate = git_config_tristate(var, value); - if (tristate == 2) { + enum git_config_type_bool_or_auto v = git_config_tristate(var, value); + switch (v) { + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE: auto_number = 1; return 0; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE: + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO: + numbered = v; + auto_number = auto_number && numbered; + return 0; } - numbered = tristate; - auto_number = auto_number && numbered; - return 0; } if (!strcmp(var, "format.attach")) { if (value && *value) @@ -905,12 +908,18 @@ static int git_format_config(const char *var, const char *value, void *cb) if (!strcmp(var, "format.signaturefile")) return git_config_pathname(&signature_file, var, value); if (!strcmp(var, "format.coverletter")) { - int tristate = git_config_tristate(var, value); - if (tristate == 2) + enum git_config_type_bool_or_auto v = git_config_tristate(var, value); + switch (v) { + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE: + config_cover_letter = COVER_OFF; + return 0; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE: + config_cover_letter = COVER_ON; + return 0; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO: config_cover_letter = COVER_AUTO; - else - config_cover_letter = tristate ? COVER_ON : COVER_OFF; - return 0; + return 0; + } } if (!strcmp(var, "format.outputdirectory")) return git_config_string(&config_output_directory, var, value); diff --git a/compat/mingw.c b/compat/mingw.c index e6e85ae99a..b76ccc0a15 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -247,12 +247,16 @@ int mingw_core_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "core.restrictinheritedhandles")) { - int tristate = git_config_tristate(var, value); - if (tristate == 2) + enum git_config_type_bool_or_auto v = git_config_tristate(var, value); + switch (v) { + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE: + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE: + core_restrict_inherited_handles = v; + return 0; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO: core_restrict_inherited_handles = -1; - else - core_restrict_inherited_handles = tristate; - return 0; + return 0; + } } return 0; diff --git a/config.c b/config.c index 74d2b2c0df..3375895b80 100644 --- a/config.c +++ b/config.c @@ -1257,11 +1257,11 @@ int git_parse_maybe_bool(const char *value) return -1; } -int git_parse_maybe_tristate(const char *value) +enum git_config_type_maybe_bool_or_auto git_parse_maybe_tristate(const char *value) { int v = git_parse_maybe_bool(value); if (v < 0 && !strcasecmp(value, "auto")) - return 2; + return GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO; return v; } @@ -1276,9 +1276,10 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) return git_config_int(name, value); } -int git_config_tristate(const char *name, const char *value) +enum git_config_type_bool_or_auto git_config_tristate(const char *name, + const char *value) { - int v = git_parse_maybe_tristate(value); + enum git_config_type_maybe_bool_or_auto v = git_parse_maybe_tristate(value); if (v < 0) die(_("bad tristate config value '%s' for '%s'"), value, name); return v; diff --git a/config.h b/config.h index c5129e4392..70684ecb3c 100644 --- a/config.h +++ b/config.h @@ -201,7 +201,14 @@ int git_parse_maybe_bool(const char *); * Same as `git_parse_maybe_bool`, except that "auto" is recognized and * will return "2". */ -int git_parse_maybe_tristate(const char *); +enum git_config_type_maybe_bool_or_auto { + GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD = -1, + GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE = 0, + GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE = 1, + GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO = 2, +}; + +enum git_config_type_maybe_bool_or_auto git_parse_maybe_tristate(const char *); /** * Parse the string to an integer, including unit factors. Dies on error; @@ -236,7 +243,13 @@ int git_config_bool(const char *, const char *); * Like git_config_bool() except "auto" is also recognized and will * return "2" */ -int git_config_tristate(const char *, const char *); +enum git_config_type_bool_or_auto { + GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE, + GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE, + GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO, +}; +enum git_config_type_bool_or_auto git_config_tristate(const char *name, + const char *value); /** * Allocates and copies the value string into the `dest` parameter; if no diff --git a/http.c b/http.c index b54a232e90..6dd6191517 100644 --- a/http.c +++ b/http.c @@ -406,12 +406,16 @@ static int http_options(const char *var, const char *value, void *cb) return git_config_string(&user_agent, var, value); if (!strcmp("http.emptyauth", var)) { - int tristate = git_config_tristate(var, value); - if (tristate == 2) + enum git_config_type_bool_or_auto v = git_config_tristate(var, value); + switch (v) { + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE: + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE: + curl_empty_auth = v; + return 0; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO: curl_empty_auth = -1; - else - curl_empty_auth = tristate; - return 0; + return 0; + } } if (!strcmp("http.delegation", var)) { diff --git a/userdiff.c b/userdiff.c index 7ff010961f..fe001563c3 100644 --- a/userdiff.c +++ b/userdiff.c @@ -277,8 +277,16 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k, static int parse_tristate(int *b, const char *k, const char *v) { - int tristate = git_config_tristate(k, v); - *b = tristate == 2 ? -1 : tristate; + enum git_config_type_bool_or_auto tristate = git_config_tristate(k, v); + switch (tristate) { + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE: + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE: + *b = tristate; + break; + case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO: + *b = -1; + break; + } return 0; } ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-08 23:23 ` Ævar Arnfjörð Bjarmason @ 2021-04-08 23:51 ` Junio C Hamano 2021-04-09 1:33 ` Ævar Arnfjörð Bjarmason 2021-04-08 23:54 ` Junio C Hamano 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2021-04-08 23:51 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Jeff King Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I'd prefer to just make these "enum", which means we'll have the aid of > the compiler in checking all the callsites, as in the patch-on-top > (which I can squash appropriately, need to update the doc comments > though) at the end of this E-Mail. I think enum is oversold by some people (not me). C Compilers won't do much when you use them interchangeably with int, simply because they are designed to be used that way, no? If existing code used 0 as false and 1 as true, and it learns an "auto" value with a new definition, #define TRISTATE_AUTO 2 without TRISTATE_{TRUE,FALSE} defined to 0 and 1, that would be a good place to stop. I'd be quite happy with that. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-08 23:51 ` Junio C Hamano @ 2021-04-09 1:33 ` Ævar Arnfjörð Bjarmason 2021-04-09 12:53 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-09 1:33 UTC (permalink / raw) To: Junio C Hamano Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Jeff King On Fri, Apr 09 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I'd prefer to just make these "enum", which means we'll have the aid of >> the compiler in checking all the callsites, as in the patch-on-top >> (which I can squash appropriately, need to update the doc comments >> though) at the end of this E-Mail. > > I think enum is oversold by some people (not me). C Compilers won't > do much when you use them interchangeably with int, simply because > they are designed to be used that way, no? They are just ints, not a seperate type like in C++. > If existing code used 0 as false and 1 as true, and it learns an > "auto" value with a new definition, > > #define TRISTATE_AUTO 2 > > without TRISTATE_{TRUE,FALSE} defined to 0 and 1, that would be a > good place to stop. I'd be quite happy with that. The benefit in this case is to human readers of the code who know they're being helped by the enum checking in "case" arms. So you can immediately look at code like: enum foo x = git_config_foo(); switch (x) [...] And know without having to jump to the definition of "git_config_foo()" that all the return values are being handled. Just to clarify, do you have a dislike of the sort of code in http://lore.kernel.org/git/875z0wicmp.fsf@evledraar.gmail.com for all uses in the codebase, or in this case because the diff of adapting existing code is larger as a result? I think if we're not going to use enum/switch for this and similar things in config.c in the future it makes sense to pass a pointer to a "is_auto" parameter to these new tristate() functions, similar to e.g. the existing git_config_bool_or_int(). ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-09 1:33 ` Ævar Arnfjörð Bjarmason @ 2021-04-09 12:53 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2021-04-09 12:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Jeff King Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The benefit in this case is to human readers of the code who know > they're being helped by the enum checking in "case" arms. Well, do we have tristate that is handled with switch/case? And more importantly, do tristates benefit from getting handled with switch/case? The one I cited as an existing example to decide that we should favour "special case 'auto' and then let it fall through to the normal bool" is in userdiff.c static int parse_tristate(int *b, const char *k, const char *v) { if (v && !strcasecmp(v, "auto")) *b = -1; else *b = git_config_bool(k, v); return 0; } and the caller uses it to set -1/0/1 in driver->binary member. And the member is often used like so: ... else if (userdiff->binary != -1) { is_binary = userdiff->binary; } else { is_binary = auto_detect_based_on_contents(...); } if (is_binary) { ... do the binary thing ... } This is because "auto" is the only value among the three that needs "preprocessing" before it is turned into a concrete yes/no that is usable in the downstream code. So with AUTO being the only spelled out value, a construct like this: if ((driver->binary != TRISTATE_AUTO) ? driver->binary : auto_detect()) ...; /* do the binary thing */ else ...; /* do the non-binary thing */ would be sufficient to help human readers. You do not need enum to help, and you do not need switch/case. You could use switch/case with enum if you really wanted to, but switch (temp = driver->binary) { case TRISTATE_AUTO: temp = auto_detect(); break; /* ????? */ case TRISTATE_FALSE: /* do the non-binary thing */ ...; break; case TRISTATE_TRUE: /* do the binary thing */ ...; break; } would not be a useful construct for the "auto" use case, where tristate is used to mean "we cannot decide at the configuration time, as the appropriate value depends on other factors that are available when it actually is used, e.g. the contents in the buffer, if fd=1 is connected to the terminal, etc." If you really wanted to, you could still use switch/case for such a use case, perhaps like this: switch (temp = driver->binary) { case TRISTATE_AUTO: temp = auto_detect(); /* fallthrough */ default: if (!temp) { /* TRISTATE_FALSE */ /* do the non-binary thing */ ...; } else { /* TRISTATE_TRUE */ /* do the binary thing */ ...; } break; } and switch may help making sure that all enum values are handled, but I do not see a value in it. The earlier code that used "if auto, run autodetect, otherwise use the value as is" as the condition for an if/else statement would be far easier to follow, and equally safe. > ... in config.c in the future it makes sense to pass a pointer to a > "is_auto" parameter to these new tristate() functions, similar to > e.g. the existing git_config_bool_or_int(). I am not sure what you are trying to gain by introducing is_auto here. For bool_or_int(), is_bool pointer makes perfect sense, because the value spelled as "true" cannot be anything but bool, but "1" can be either boolean true or an integer. An extra is_bool bit can be used by callers that care if the user said "true/yes" or "1" and want to behave differently, when the configuration can take either bool or integer. For example, if you originally have a "do you want this operation to be verbose?" yes/no variable, and later extended it to add verbosity level, "yes/true" might mean "yes, please use the default verbosity level", while "1", "2", ... would mean "yes, and please set the verbosity level to this number". And the default verbosity level may not be "1", so the distinction between "yes" and "1" does matter. I do not see such a disambiguation need for tristate parsing, so the immense usefulness of is_bool is not a good analogy to draw value for the proposed is_auto from. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-08 23:23 ` Ævar Arnfjörð Bjarmason 2021-04-08 23:51 ` Junio C Hamano @ 2021-04-08 23:54 ` Junio C Hamano 1 sibling, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2021-04-08 23:54 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Jeff King Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> +int git_parse_maybe_tristate(const char *value) >>> +{ >>> + int v = git_parse_maybe_bool(value); >>> + if (v < 0 && !strcasecmp(value, "auto")) >>> + return 2; >>> + return v; >>> +} >> >> This is not parse_maybe_bool_text(), so "1" and "-1" written in the >> configuration file are "true", "0" is "false", like the "bool" case. >> >> I wonder if written without an unnecessary extra variable, i.e. >> >> if (value && !strcasecmp(value, "auto")) >> return 2; >> return git_parse_maybe_bool(value); >> >> is easier to follow, though, as it is quite clear that it is mostly >> the same as maybe_bool and the only difference is when "auto" is >> given. > > I guess it could be either way around,... Having seen another example in the current code recently, static int parse_tristate(int *b, const char *k, const char *v) { - if (v && !strcasecmp(v, "auto")) - *b = -1; - else - *b = git_config_bool(k, v); I upgrade my earlier "I wonder" to "I do think that". Let's swap the order so that it is clear that we are special-casing "auto". ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-08 18:33 ` Junio C Hamano 2021-04-08 23:23 ` Ævar Arnfjörð Bjarmason @ 2021-04-09 20:05 ` Jeff King 2021-04-09 22:11 ` Junio C Hamano 1 sibling, 1 reply; 21+ messages in thread From: Jeff King @ 2021-04-09 20:05 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, git, Lin Sun, Đoàn Trần Công Danh, David Aguilar On Thu, Apr 08, 2021 at 11:33:13AM -0700, Junio C Hamano wrote: > > +/** > > + * Same as `git_parse_maybe_bool`, except that "auto" is recognized and > > + * will return "2". > > + */ > > +int git_parse_maybe_tristate(const char *); > > A false being 0 and a true being 1 is understandable for readers > without symbolic constant, but "2" deserves to have a symbolic > constant, doesn't it? I had the same thought. This "tristate" really has four outcomes: true, false, auto, or error. Since the caller has to then check for error or for "auto" themselves, it feels like it is not actually making their lives any easier. I.e., without it: if (value && !strcmp(value, "auto")) do_auto(); else { int b = git_parse_maybe_bool(value); if (b < 0) do_error(); else if (b) do_true(); else do_false(); } but with it: b = git_parse_maybe_tristate(value); if (b < 0) do_error(); else if (b == 2) do_auto(); else if (b) do_true(); else do_false(); which is the same thing, swapping string comparison for a numeric one. And a caller of git-config has the same thing: it still has to check for "auto" in the output it gets (which it could just do via bool-or-str). It seems like it would be more convenient if you could pass it a bool value to turn the "auto" into. E.g., if you could do: b = git_parse_maybe_tristate(value, 1); /* default to "1" */ if (b < 0) do_error(); if (b) do_true(); /* true, or maybe "auto" */ else do_false(); I dunno. That would probably be hard to represent via "git config --type". And some callers probably do care about "auto" versus "true". It also feels funny calling this "tristate". Aren't there other tristates we could have besides "auto"? The command-line option is bool-or-auto, which is descriptive. Should this use a similar name? -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-09 20:05 ` Jeff King @ 2021-04-09 22:11 ` Junio C Hamano 2021-04-10 1:23 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2021-04-09 22:11 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Lin Sun, Đoàn Trần Công Danh, David Aguilar Jeff King <peff@peff.net> writes: > It seems like it would be more convenient if you could pass it a bool > value to turn the "auto" into. E.g., if you could do: > > b = git_parse_maybe_tristate(value, 1); /* default to "1" */ > if (b < 0) > do_error(); > if (b) > do_true(); /* true, or maybe "auto" */ > else > do_false(); > > I dunno. That would probably be hard to represent via "git config > --type". And some callers probably do care about "auto" versus "true". It would work well for codepaths where computing the default value is cheap (or even possible). I think the point of using "auto" is to delay the decision as late as possible. E.g. in-core parsed config and attribute may still want to stay "auto", until we actually get our hands on the blob contents to see if it is binary, until we know how heavily loaded the system is, until we know isatty(1), etc. Some are cheap to compute in advance, some are expensive and impossible until we meet the data. So I still think the canonical use pattern for the "auto" thing is is_frotz = git_parse_bool_or_auto(value); ... arbitrary number of things can happen here ... the above may even be done in a git_config() ... callback, and is_frotz may not even be used. if (is_frotz == AUTO) is_frotz = auto_detect_frotz(); if (is_frotz) ...; /* do the frotz thing */ else ...; /* do the non-frotz thing */ > It also feels funny calling this "tristate". Aren't there other > tristates we could have besides "auto"? The command-line option is > bool-or-auto, which is descriptive. Should this use a similar name? I like that bool-or-auto name very much. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-09 22:11 ` Junio C Hamano @ 2021-04-10 1:23 ` Jeff King 2021-04-10 1:43 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2021-04-10 1:23 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, git, Lin Sun, Đoàn Trần Công Danh, David Aguilar On Fri, Apr 09, 2021 at 03:11:10PM -0700, Junio C Hamano wrote: > > I dunno. That would probably be hard to represent via "git config > > --type". And some callers probably do care about "auto" versus "true". > > It would work well for codepaths where computing the default value > is cheap (or even possible). > > I think the point of using "auto" is to delay the decision as late > as possible. E.g. in-core parsed config and attribute may still > want to stay "auto", until we actually get our hands on the blob > contents to see if it is binary, until we know how heavily loaded > the system is, until we know isatty(1), etc. Some are cheap to > compute in advance, some are expensive and impossible until we meet > the data. Hmm, yeah, that's the "do care about auto versus true" thing. > So I still think the canonical use pattern for the "auto" thing is > > is_frotz = git_parse_bool_or_auto(value); > > ... arbitrary number of things can happen here > ... the above may even be done in a git_config() > ... callback, and is_frotz may not even be used. > > if (is_frotz == AUTO) > is_frotz = auto_detect_frotz(); > > if (is_frotz) > ...; /* do the frotz thing */ > else > ...; /* do the non-frotz thing */ That makes sense. Usually we represent "undecided" in such a tristate with -1, so something that returned -1/0/1 would feel very natural to me (and probably wouldn't need symbolic constants even). But -1 is also error. So a function like: int git_parse_tristate(const char *value, int *out); which returned success/error via its return value, and put the value into "out" would feel pretty natural to me. I dunno. I admit I don't care _that_ much, but somehow Ævar's series have a way of sniping me into responding anyway. :) -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] config.c: add a "tristate" helper 2021-04-10 1:23 ` Jeff King @ 2021-04-10 1:43 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2021-04-10 1:43 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Lin Sun, Đoàn Trần Công Danh, David Aguilar Jeff King <peff@peff.net> writes: > So a function like: > > int git_parse_tristate(const char *value, int *out); > > which returned success/error via its return value, and put the value > into "out" would feel pretty natural to me. Yeah, with s/tristate/bool-or-auto/, and if we do this throughout the types, that would be ideal. FWIW git_parse_ulong() and friends for sized numerics already follow that pattern, but helpers for boolean like git_parse_maybe_bool() don't, which is unfortunate. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/5] config: add --type=bool-or-auto switch 2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-04-08 13:34 ` [PATCH 4/5] config.c: add a "tristate" helper Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 ` Ævar Arnfjörð Bjarmason 2021-04-08 18:36 ` Junio C Hamano 4 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-08 13:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Lin Sun, Đoàn Trần Công Danh, David Aguilar, Ævar Arnfjörð Bjarmason Now that we're using git_config_tristate() internally let's expose it via "git config" like we do "bool", "int" etc for completeness, and so that we can easily test it. Unlike the --type=bool-or-str option added in dbd8c09bfe (mergetool: allow auto-merge for meld to follow the vim-diff behavior, 2020-05-07) we don't have or anticipate any in-tree user of this except the tests. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-config.txt | 4 +++ builtin/config.c | 19 ++++++++++++++ t/t1300-config.sh | 49 ++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 4ae9ef210c..1af8222e82 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -189,6 +189,10 @@ Valid `<type>`'s include: above. - 'bool-or-str: canonicalize according to either 'bool' (as described above), or emit the value as-is. +- 'bool-or-auto: canonicalize according to either 'bool', as described + above, or whether the value is "auto". This is used by various + "tristate" variables such as `core.restrictInheritedHandles`, + `format.numbered` etc. - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and `~user` to the home directory for the specified user. This specifier has no effect when setting the value (but you can use `git config section.variable diff --git a/builtin/config.c b/builtin/config.c index f71fa39b38..039a4f0961 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -68,6 +68,7 @@ static int fixed_value; #define TYPE_EXPIRY_DATE 5 #define TYPE_COLOR 6 #define TYPE_BOOL_OR_STR 7 +#define TYPE_BOOL_OR_AUTO 8 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \ { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \ @@ -99,6 +100,8 @@ static int option_parse_type(const struct option *opt, const char *arg, new_type = TYPE_BOOL_OR_INT; else if (!strcmp(arg, "bool-or-str")) new_type = TYPE_BOOL_OR_STR; + else if (!strcmp(arg, "bool-or-auto")) + new_type = TYPE_BOOL_OR_AUTO; else if (!strcmp(arg, "path")) new_type = TYPE_PATH; else if (!strcmp(arg, "expiry-date")) @@ -156,6 +159,7 @@ static struct option builtin_config_options[] = { OPT_CALLBACK_VALUE(0, "int", &type, N_("value is decimal number"), TYPE_INT), OPT_CALLBACK_VALUE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT), OPT_CALLBACK_VALUE(0, "bool-or-str", &type, N_("value is --bool or string"), TYPE_BOOL_OR_STR), + /* No bool-or-auto! The --<type> form is deprecated in favor of --type=<what> */ OPT_CALLBACK_VALUE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH), OPT_CALLBACK_VALUE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE), OPT_GROUP(N_("Other")), @@ -263,6 +267,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value strbuf_addstr(buf, value_); else strbuf_addstr(buf, v ? "true" : "false"); + } else if (type == TYPE_BOOL_OR_AUTO) { + int v = git_config_tristate(key_, value_); + if (v == 2) + strbuf_addstr(buf, "auto"); + else + strbuf_addstr(buf, v ? "true" : "false"); } else if (type == TYPE_PATH) { const char *v; if (git_config_pathname(&v, key_, value_) < 0) @@ -435,6 +445,15 @@ static char *normalize_value(const char *key, const char *value) else return xstrdup(v ? "true" : "false"); } + if (type == TYPE_BOOL_OR_AUTO) { + int v = git_parse_maybe_tristate(value); + if (v < 0) + return xstrdup(value); + else if (v == 2) + xstrdup("auto"); + else + return xstrdup(v ? "true" : "false"); + } if (type == TYPE_COLOR) { char v[COLOR_MAXLEN]; if (git_config_color(v, key, value)) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index a002ec5644..952d9e9ed9 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -874,6 +874,55 @@ test_expect_success 'get --bool-or-str' ' test_cmp expect actual ' +test_expect_success 'there is no --bool-or-auto, --<type> is deprecated in favor of --type=<type>' ' + test_expect_code 129 git config --bool-or-auto +' + +test_expect_success 'get --type=bool-or-auto' ' + cat >.git/config <<-\EOF && + [bool] + true1 + true2 = true + false = false + [int] + int1 = 0 + int2 = 1 + int3 = -1 + [string] + string1 = hello + string2 = there you + [auto] + auto1 = auto + auto2 = AUTO + [bad-auto] + bad-auto1 = AUTOMATIC + EOF + cat >expect <<-\EOF && + true + true + false + false + true + true + auto + auto + EOF + { + git config --type=bool-or-auto bool.true1 && + git config --type=bool-or-auto bool.true2 && + git config --type=bool-or-auto bool.false && + git config --type=bool-or-auto int.int1 && + git config --type=bool-or-auto int.int2 && + git config --type=bool-or-auto int.int3 && + git config --type=bool-or-auto auto.auto1 && + git config --type=bool-or-auto auto.auto2 + } >actual && + test_cmp expect actual && + + test_must_fail git config --type=bool-or-auto --get bad-auto.bad-auto1 2>err && + grep "bad tristate config value" err +' + cat >expect <<\EOF [bool] true1 = true -- 2.31.1.527.g9b8f7de2547 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] config: add --type=bool-or-auto switch 2021-04-08 13:34 ` [PATCH 5/5] config: add --type=bool-or-auto switch Ævar Arnfjörð Bjarmason @ 2021-04-08 18:36 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2021-04-08 18:36 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Lin Sun, Đoàn Trần Công Danh, David Aguilar Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Now that we're using git_config_tristate() internally let's expose it > via "git config" like we do "bool", "int" etc for completeness, and so > that we can easily test it. > > Unlike the --type=bool-or-str option added in dbd8c09bfe (mergetool: > allow auto-merge for meld to follow the vim-diff behavior, 2020-05-07) > we don't have or anticipate any in-tree user of this except the tests. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> OK. > @@ -263,6 +267,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value > strbuf_addstr(buf, value_); > else > strbuf_addstr(buf, v ? "true" : "false"); > + } else if (type == TYPE_BOOL_OR_AUTO) { > + int v = git_config_tristate(key_, value_); > + if (v == 2) > + strbuf_addstr(buf, "auto"); > + else > + strbuf_addstr(buf, v ? "true" : "false"); Makes sense. > +test_expect_success 'there is no --bool-or-auto, --<type> is deprecated in favor of --type=<type>' ' > + test_expect_code 129 git config --bool-or-auto > +' > + > +test_expect_success 'get --type=bool-or-auto' ' > + cat >.git/config <<-\EOF && > + [bool] > + true1 > + true2 = true > + false = false > + [int] > + int1 = 0 > + int2 = 1 > + int3 = -1 > + [string] > + string1 = hello > + string2 = there you > + [auto] > + auto1 = auto > + auto2 = AUTO > + [bad-auto] > + bad-auto1 = AUTOMATIC > + EOF > + cat >expect <<-\EOF && > + true > + true > + false > + false > + true > + true > + auto > + auto > + EOF Almost the same comment applies to the expected output as the earlier patch. Other than that, (and adjusting for 2 that should be turned into symbolic constant in an earlier step in a reroll), this step looks quite sensible. Thanks. > + { > + git config --type=bool-or-auto bool.true1 && > + git config --type=bool-or-auto bool.true2 && > + git config --type=bool-or-auto bool.false && > + git config --type=bool-or-auto int.int1 && > + git config --type=bool-or-auto int.int2 && > + git config --type=bool-or-auto int.int3 && > + git config --type=bool-or-auto auto.auto1 && > + git config --type=bool-or-auto auto.auto2 > + } >actual && > + test_cmp expect actual && > + > + test_must_fail git config --type=bool-or-auto --get bad-auto.bad-auto1 2>err && > + grep "bad tristate config value" err > +' > + > cat >expect <<\EOF > [bool] > true1 = true ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-04-10 1:43 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason 2021-04-08 13:34 ` [PATCH 1/5] config.c: add a comment about why value=NULL is true Ævar Arnfjörð Bjarmason 2021-04-08 18:10 ` Junio C Hamano 2021-04-08 13:34 ` [PATCH 2/5] config tests: test for --bool-or-str Ævar Arnfjörð Bjarmason 2021-04-08 18:21 ` Junio C Hamano 2021-04-08 23:11 ` Ævar Arnfjörð Bjarmason 2021-04-08 13:34 ` [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str Ævar Arnfjörð Bjarmason 2021-04-08 18:22 ` Junio C Hamano 2021-04-08 13:34 ` [PATCH 4/5] config.c: add a "tristate" helper Ævar Arnfjörð Bjarmason 2021-04-08 18:33 ` Junio C Hamano 2021-04-08 23:23 ` Ævar Arnfjörð Bjarmason 2021-04-08 23:51 ` Junio C Hamano 2021-04-09 1:33 ` Ævar Arnfjörð Bjarmason 2021-04-09 12:53 ` Junio C Hamano 2021-04-08 23:54 ` Junio C Hamano 2021-04-09 20:05 ` Jeff King 2021-04-09 22:11 ` Junio C Hamano 2021-04-10 1:23 ` Jeff King 2021-04-10 1:43 ` Junio C Hamano 2021-04-08 13:34 ` [PATCH 5/5] config: add --type=bool-or-auto switch Ævar Arnfjörð Bjarmason 2021-04-08 18:36 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).