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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2021, 16:55 UTC
Message-ID: <xmqqeeaa6fey.fsf@gitster.g>
In-Reply-To: <YSvsQcGNpCMZwS8o@nand.local>

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:

Show 13 quoted lines

> 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.