Re: [PATCH v4] t: port helper/test-hashmap.c to unit-tests/t-hashmap.c
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 31, 2024, 18:50 UTC
- Message-ID
- <xmqq7cd12y5r.fsf@gitster.g>
- In-Reply-To
- <CAP8UFD04uQbWhY0QDp+nC8VyBvpUs=tBDm=w4CzKK2GcwFZdGQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
Show 29 quoted lines
>> + 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 ;-)