Am 30.08.21 um 18:55 schrieb Junio C Hamano: > Taylor Blau <me@xxxxxxxxxxxx> 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é
