Re: [PATCH v4] t: port helper/test-hashmap.c to unit-tests/t-hashmap.c

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

>>      +  const char *query[][2] = {
>>      +          { ignore_case ? "Key1" : "key1", "value1" },
>>      +          { ignore_case ? "keY2" : "key2", "value2" },
>>     -+          { ignore_case ? "foobarfrotz" : "fooBarFrotz", "value3" }
>>     ++          { ignore_case ? "FOObarFrotz" : "fooBarFrotz", "value3" },
>>     ++          { ignore_case ? "TENOR" : "tenor",
>>     ++            ignore_case ? "value4" : "value5" }
>>      +  };
>
> I suggested adding the following test case:
>
>                { ignore_case ? "FOOBarFrotZ" : "foobarfrotz",
>                  ignore_case ? : "value3" : "value4" }
>
> to better check that things work well especially when not ignoring the case.
>
> This is because, when not ignoring the case, there used to be a choice
> between { "fooBarFrotz", "value3" } and { "foobarfrotz", "value4" }
> that can be decided only by the case of the key in 'query'. But
> instead you removed that choice from 'key_val'.
>
> And it seems to me that the new test case you added doesn't bring much value.
>
> Maybe there was something wrong in what I suggested, but then please explain it.
>
> The other changes look good to me and seem to address the suggestions
> made by others and me.
>
> Thanks!

Thanks for a careful review, and of course thanks for working on the
patch, A U Thor ;-)