Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
On Mon, Feb 20, 2017 at 01:58:07AM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> Since nothing seems to have happened in the meantime, here is what
> I'll queue so that we won't forget for now. Lars's tests based on
> how the scripted "git submodule" uses "git config" may still be
> valid, but it is somewhat a roundabout way to demonstrate the
> breakage and not very effective way to protect the fix, so I added a
> new test that directly tests "git -c <var>=<val> <command>".
Yeah, I agree that is the best way to cover this code.
> I am not sure if this updated one is worth doing, or the previous
> "strchr and strrchr" I originally wrote was easier to understand.
I think either is OK. I would actually have written it halfway in between, like:
static void canonicalize_config_variable_name(char *varname)
{
const char *p; /* downcase the first segment */
for (p = varname; *p; p++) {
if (*p == '.')
break;
*p = tolower(*p);
} /* locate end of middle segment, if there is one */
p = strrchr(p, '.');
if (!p)
return; /* invalid single-level var, but let it pass */
for (; *p; p++)
*p = tolower(*p);
}You can toss that on the bikeshed heap. :)
The other change from yours is that it flips the order of the strrchr and return. Yours is more efficient in the sense that we know there is no dot, so we do not have to keep searching at all (though of course this case is invalid and we would not expect to see it in practice).
But it did take me a minute in yours to figure out why last_dot was always set to a dot (the answer is that it starts at "cp", which must itself be a dot, because we would already have returned otherwise).
> One thing I noticed is that "git config --get X" will correctly
> diagnose that a dot-less X is not a valid variable name, but we do
> not seem to diagnose "git -c X=V <cmd>" as invalid.
I don't think that was intentional. We just never caught the case. It might be reasonable to do so (and it's easy to catch here while canonicalizing). I think there are probably some other cases we _could_ catch, too (e.g., invalid characters for keynames). But I'm not excited about duplicating the logic from the file-parser.
> Perhaps we should, but it is not the focus of this topic.
Show 11 quoted lines
> diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh
> new file mode 100755
> index 0000000000..acb8dc3b15
> --- /dev/null
> +++ b/t/t1351-config-cmdline.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='git -c var=val'
> +
> +. ./test-lib.sh
There are a bunch of other "git -c" tests inside t1300. I don't know if it would be better to put them all together.
Arguably, those other ones should get pulled out here to the new script. More scripts means that the tests have fewer hidden dependencies, and we can parallelize the run more. I admit I've shied away from new scripts in the past because the number allocation is such a pain.
Show 6 quoted lines
> +test_expect_success 'last one wins: two level vars' '
> + echo VAL >expect &&
> +
> + # sec.var and sec.VAR are the same variable, as the first
> + # and the last level of a configuration variable name is
> + # case insensitive. Test both setting and querying.
I assume by "setting and querying" here you mean "setting via -c, querying via git-config".
Show 9 quoted lines
> + git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
> + test_cmp expect actual &&
> + git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
> + test_cmp expect actual &&
> +
> + git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
> + test_cmp expect actual &&
> + git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
> + test_cmp expect actual
Show 22 quoted lines
> +test_expect_success 'last one wins: three level vars' '
> + echo val >expect &&
> +
> + # v.a.r and v.A.r are not the same variable, as the middle
> + # level of a three-level configuration variable name is
> + # case sensitive. Test both setting and querying.
> +
> + git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
> + test_cmp expect actual &&
> +
> + echo VAL >expect &&
> + git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
> + test_cmp expect actual
> +'
There are two blocks of tests, each with their own "expect" value. Should the top "expect" here be moved down with its block to make that more clear?
Other than that nit, the tests look good to me.
-Peff
Lars Schneider
