patchmerge-recursive: use fspathcmp() in path_hashmap_cmp()

> Call fspathcmp() instead of open-coding it.  This shortens the code and
> makes it less repetitive.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  merge-recursive.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 3355d50e8a..840599fd53 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,
>  	a = container_of(eptr, const struct path_hashmap_entry, e);
>  	b = container_of(entry_or_key, const struct path_hashmap_entry, e);
>
> -	if (ignore_case)
> -		return strcasecmp(a->path, key ? key : b->path);
> -	else
> -		return strcmp(a->path, key ? key : b->path);
> +	return fspathcmp(a->path, key ? key : b->path);
>  }

> > +++ b/merge-recursive.c
> > @@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,
> [...]
> 
> 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);
>  }
> 
>  static struct test_entry *alloc_test_entry(unsigned int hash,

>> -       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,
>
> Maybe also:
>
> diff --git a/dir.c b/dir.c
> index 03c4d21267..ee46290cbb 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -669,9 +669,7 @@ int pl_hashmap_cmp(const void *unused_cmp_data,
>  			 ? ee1->patternlen
>  			 : ee2->patternlen;
>  
> -	if (ignore_case)
> -		return strncasecmp(ee1->pattern, ee2->pattern, min_len);
> -	return strncmp(ee1->pattern, ee2->pattern, min_len);
> +	return fspathncmp(ee1->pattern, ee2->pattern, min_len);
>  }
>  
>  static char *dup_and_filter_pattern(const char *pattern)
>
> This is fun. :)

> Hi René,
>
> On Sat, Aug 28, 2021 at 11:30:49PM +0200, René Scharfe wrote:
>> Call fspathcmp() instead of open-coding it.  This shortens the code and
>> makes it less repetitive.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  merge-recursive.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 3355d50e8a..840599fd53 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,
>>  	a = container_of(eptr, const struct path_hashmap_entry, e);
>>  	b = container_of(entry_or_key, const struct path_hashmap_entry, e);
>>
>> -	if (ignore_case)
>> -		return strcasecmp(a->path, key ? key : b->path);
>> -	else
>> -		return strcmp(a->path, key ? key : b->path);
>> +	return fspathcmp(a->path, key ? key : b->path);
>>  }
>
> 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);
>  }
>
>  static struct test_entry *alloc_test_entry(unsigned int hash,
>

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

> 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);

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

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

> On Mon, Aug 30, 2021 at 08:22:25PM +0200, René Scharfe wrote:
>
>>> 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.
>
> Really most of these ought to be in the repository struct anyway, I
> would think. The value of ignore_case comes from core.ignorecase, which
> is going to be repository-specific. We are probably doing the wrong
> thing already by looking at the parent core.ignorecase value when
> operating in an in-process submodule, but nobody noticed because it's
> quite unlikely for a submodule to have a different setting than the
> parent.

> Am 30.08.21 um 22:49 schrieb Jeff King:
> > On Mon, Aug 30, 2021 at 08:22:25PM +0200, René Scharfe wrote:
> >
> >>> 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.
> >
> > Really most of these ought to be in the repository struct anyway, I
> > would think. The value of ignore_case comes from core.ignorecase, which
> > is going to be repository-specific. We are probably doing the wrong
> > thing already by looking at the parent core.ignorecase value when
> > operating in an in-process submodule, but nobody noticed because it's
> > quite unlikely for a submodule to have a different setting than the
> > parent.
>
> Good point.  So fspathcmp() and friends would need a repo parameter. :-|

> > Good point.  So fspathcmp() and friends would need a repo parameter. :-|
> 
> Yes, we will eventually have to pass `struct repository *r` into a _lot_
> of call chains. It'll be a disruptive change, yet if the submodule folks
> truly want to aim for in-process recursive treatment of submodules, there
> is no alternative.
> 
> FWIW on Windows there are other potentially repository-specific settings
> that are relevant in similar situations. For example, there is
> `core.symlinks`.

> Another approach is to stuff the appropriate globals into the repository
> struct, and then "push" onto the global the_repository pointer, treating
> it like a stack. And then low-level code is free to use that global
> context, even if it wasn't passed in.
> ...
>   - it's a challenge with threading (an obvious problem would be a
>     multi-threaded grep which wanted to descend into a submodule). Using
>     a thread-local global for the_repository might solve that.
>
> It's possible that this is a terrible direction to go, so I'm not
> necessarily endorsing it, but just offering it as a possibility to think
> about. The trickiest thing is that any devil would likely be in the
> details, and we wouldn't know until proceeding for a while along that
> path. Whereas passing around a context struct, while verbose and
> annoying, is a well-understood construct.

> On Mon, Sep 13, 2021 at 01:37:48PM +0200, Johannes Schindelin wrote:
>
> > > Good point.  So fspathcmp() and friends would need a repo parameter. :-|
> >
> > Yes, we will eventually have to pass `struct repository *r` into a _lot_
> > of call chains. It'll be a disruptive change, yet if the submodule folks
> > truly want to aim for in-process recursive treatment of submodules, there
> > is no alternative.
> >
> > FWIW on Windows there are other potentially repository-specific settings
> > that are relevant in similar situations. For example, there is
> > `core.symlinks`.
>
> Another approach is to stuff the appropriate globals into the repository
> struct, and then "push" onto the global the_repository pointer, treating
> it like a stack. And then low-level code is free to use that global
> context, even if it wasn't passed in.
>
> That helps the primary use case of "now I need to do something in a
> sub-module, but I'd like to do it in-process". But it's not without
> challenges:
>
>   - code which acts at the boundary of a submodule and a superproject
>     may be more awkward (since only one of them can be "the current
>     repository" at a time).
>
>   - it's a challenge with threading (an obvious problem would be a
>     multi-threaded grep which wanted to descend into a submodule). Using
>     a thread-local global for the_repository might solve that.
>
> It's possible that this is a terrible direction to go, so I'm not
> necessarily endorsing it, but just offering it as a possibility to think
> about. The trickiest thing is that any devil would likely be in the
> details, and we wouldn't know until proceeding for a while along that
> path. Whereas passing around a context struct, while verbose and
> annoying, is a well-understood construct.

> Seeing as there seems to be some appetite for refactoring Git's code on
> this list, I am thinking that the `struct repository *r` direction might
> be the one to go for. And I mean like "move the globals into that struct"
> as opposed to introducing that stack you talked about. It would even be a
> refactoring where I would understand the motivation, and agree with it,
> too.