Re: [PATCH v2 3/7] win32: override `fspathcmp()` with a directory separator-aware version

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Johannes

On 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> On Windows, the backslash is the directory separator, even if the
> forward slash can be used, too, at least since Windows NT.
> 
> This means that the paths `a/b` and `a\b` are equivalent, and
> `fspathcmp()` needs to be made aware of that fact.

How does fspathncmp() behave? It would be good for the two to match. 
I've left a couple of thoughts below but I'm not sure they're worth 
re-rolling for

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/win32/path-utils.c | 25 +++++++++++++++++++++++++
>   compat/win32/path-utils.h |  2 ++
>   dir.c                     |  2 +-
>   dir.h                     |  2 +-
>   git-compat-util.h         |  5 +++++
>   5 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
> index ebf2f12eb66..af7ef957bbf 100644
> --- a/compat/win32/path-utils.c
> +++ b/compat/win32/path-utils.c
> @@ -50,3 +50,28 @@ int win32_offset_1st_component(const char *path)
>   
>   	return pos + is_dir_sep(*pos) - path;
>   }
> +
> +int win32_fspathcmp(const char *a, const char *b)
> +{
> +	int diff;
> +
> +	for (;;) {
> +		if (!*a)
> +			return !*b ? 0 : -1;

Personally I'd find this easier to read as

	return *b ? -1 : 0;

> +		if (!*b)
> +			return +1;
> +
> +		if (is_dir_sep(*a)) {
> +			if (!is_dir_sep(*b))
> +				return -1;
> +			a++;
> +			b++;
> +			continue;
> +		} else if (is_dir_sep(*b))
> +			return +1;
> +
> +		diff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));

There is a subtle difference between this and strcasecmp() because the 
latter is locale dependent but our tolower() is not because we override 
the standard library's version. As they're both useless on multibyte 
locales it probably doesn't make much difference in practice.

Thanks

Phillip

> +		if (diff)
> +			return diff;
> +	}
> +}
> diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
> index 65fa3b9263a..e3c2a79df74 100644
> --- a/compat/win32/path-utils.h
> +++ b/compat/win32/path-utils.h
> @@ -29,5 +29,7 @@ static inline int win32_has_dir_sep(const char *path)
>   #define has_dir_sep(path) win32_has_dir_sep(path)
>   int win32_offset_1st_component(const char *path);
>   #define offset_1st_component win32_offset_1st_component
> +int win32_fspathcmp(const char *a, const char *b);
> +#define fspathcmp win32_fspathcmp
>   
>   #endif
> diff --git a/dir.c b/dir.c
> index b7a6625ebda..37d8e266b2c 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -95,7 +95,7 @@ int count_slashes(const char *s)
>   	return cnt;
>   }
>   
> -int fspathcmp(const char *a, const char *b)
> +int git_fspathcmp(const char *a, const char *b)
>   {
>   	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
>   }
> diff --git a/dir.h b/dir.h
> index 69a76d8bdd3..947e3d77442 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -541,7 +541,7 @@ int remove_dir_recursively(struct strbuf *path, int flag);
>    */
>   int remove_path(const char *path);
>   
> -int fspathcmp(const char *a, const char *b);
> +int git_fspathcmp(const char *a, const char *b);
>   int fspatheq(const char *a, const char *b);
>   int fspathncmp(const char *a, const char *b, size_t count);
>   unsigned int fspathhash(const char *str);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index ca7678a379d..ac564a68987 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -506,6 +506,11 @@ static inline int git_offset_1st_component(const char *path)
>   #define offset_1st_component git_offset_1st_component
>   #endif
>   
> +
> +#ifndef fspathcmp
> +#define fspathcmp git_fspathcmp
> +#endif
> +
>   #ifndef is_valid_path
>   #define is_valid_path(path) 1
>   #endif