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

["René Scharfe" <l.s.r@web.de>]

Am 30.08.21 um 18:55 schrieb Junio C Hamano:
> 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.

The name itself is OK, I think, but using it at global scope is
confusing.  -Wshadow can help find such cases, but not this one, as
test-hashmap.c doesn't include the global declaration.  Moving the
global into a struct to provide a poor man's namespace would fix this
for all namesakes, assisted by the compiler.  We'd then access it as
the_config.ignore_case or even the_config.core.ignore_case.

Moving all config-related variables would be quite noisy, I guess,
and probably conflict with lots of in-flight patches, but might be
worth it.

René