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

From: Jeff King <peff@peff.net>
Date: Aug 30, 2021, 18:19 UTC
Message-ID: <YS0hMILaN2ODfM3y@coredump.intra.peff.net>
In-Reply-To: <60e9baf4-4e76-71a8-e6bb-1af87486994d@web.de>

On Mon, Aug 30, 2021 at 05:09:34PM +0200, René Scharfe wrote:

Show 23 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);
> >  }
> >
> >  static struct test_entry *alloc_test_entry(unsigned int hash,
> >
> 
> That's a local variable named "ignore_case", not the one declared in
> environment.c that fspathcmp() uses, so this would change the behavior.
> The helper code does not include cache.h, so this is not even a case of
> variable shadowing, just two different variables for similar purposes
> in different places having the same name.

Yikes, good catch. Perhaps it's overkill, but I wonder if a comment like:

  /*
   * Do not use fspathcmp() here; our behavior depends on the local
   * ignore_case variable, not the usual Git-wide global.
   */

would help.

I double-checked the spot I suggested. I think it is actually using the global (though I got it right through sheer luck).

-Peff