Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()

> Taylor Blau <me@ttaylorr.com> writes:
>
>> Looks obviously right to me. I found another spot in
>> t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the
>> same way. But this looks fine with or without the following diff:
>
>> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
>> index 36ff07bd4b..ab34bdfecd 100644
>> --- a/t/helper/test-hashmap.c
>> +++ b/t/helper/test-hashmap.c
>> @@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,
>>         e1 = container_of(eptr, const struct test_entry, ent);
>>         e2 = container_of(entry_or_key, const struct test_entry, ent);
>>
>> -       if (ignore_case)
>> -               return strcasecmp(e1->key, key ? key : e2->key);
>> -       else
>> -               return strcmp(e1->key, key ? key : e2->key);
>> +       return fspathcmp(e1->key, key ? key : e2->key);
>
> Sorry but I think this patch is wrong.  Before the precontext of the
> patch, there is a local variable decl for ignore_case---the existing
> code looks at ignore_case that is different from the global
> ignore_case fspathcmp() looks at.
>
> Admittedly, it was probably not an excellent idea to give a name so
> bland and unremarkable, 'ignore_case', to a global that affects so
> many code paths in the system.  But the variable is already very
> established that renaming it would not contribute to improving the
> code at all.
>
> It however may not be a bad idea to catch these code paths where a
> local variable masks 'ignore_case' (and possibly other globals) and
> rename these local ones to avoid a mistake like this.