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

From: Junio C Hamano <[email-masked]>
Date: Mon, 30 Aug 2021 09:55:17 -0700

Taylor Blau <[email-masked]> 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.

Thanks.