Jeff King <peff@xxxxxxxx> writes: > The mention of "recursive" in the function we call made me what wonder > if we'd need more normalization. And I think we do. Try this > modification to your test: > > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > index 321e305979..be549b1d4b 100755 > --- a/t/t4205-log-pretty-formats.sh > +++ b/t/t4205-log-pretty-formats.sh > @@ -61,8 +61,9 @@ test_expect_success 'alias user-defined format' ' > > test_expect_success 'alias user-defined format is matched case-insensitively' ' > git log --pretty="format:%h" >expected && > - git config pretty.testalias "format:%h" && > - git log --pretty=testAlias >actual && > + git config pretty.testone "format:%h" && > + git config pretty.testtwo testOne && > + git log --pretty=testTwo >actual && > test_cmp expected actual > ' Very good thinking. I totally missed the short-cut to another short-cut while reading the patch. >> +test_expect_success 'alias user-defined format is matched case-insensitively' ' >> + git log --pretty="format:%h" >expected && >> + git config pretty.testalias "format:%h" && >> + git log --pretty=testAlias >actual && >> + test_cmp expected actual >> +' > > Modern style would be to use "test_config" here (or just "git -c"), but > I see the surrounding tests are too old to do so. So I'd be OK with > matching them (but cleaning up all of the surrounding ones would be > nice, too). Yup. I do not mind seeing it done either way, as a preliminary clean-up before the main fix, just a fix with more modern style while leaving the clean-up as #leftoverbits to be done after the dust settles. > PS The matching rules in find_commit_format_recursive() seem weird > to me. We do a prefix match, and then return the entry whose name is > the shortest? And break ties based on which came first? So: > > git -c pretty.abcd=format:one \ > -c pretty.abc=format:two \ > -c pretty.abd=format:three \ > log -1 --format=ab > > quietly chooses "two". I guess the "shortest wins" is meant to allow > "foo" to be chosen over "foobar" if you specify the whole name. But > the fact that we don't flag an ambiguity between "abc" and "abd" > seems strange. > That is all orthogonal to your patch, of course, but just a > head-scratcher I noticed while looking at the code. I think it is not just strange but outright wrong. I agree that it is orthogonal to this fix. Thanks, both.
