Re: [RFC PATCH v2 02/36] dir API: add a generalized path_match_flags() function

[Derrick Stolee <derrickstolee@github.com>]

On 4/18/2022 1:23 PM, Ævar Arnfjörð Bjarmason wrote:
> Add a path_match_flags() function and have the two sets of
> starts_with_dot_{,dot_}slash() functions added in
> 63e95beb085 (submodule: port resolve_relative_url from shell to C,
> 2016-04-15) and a2b26ffb1a8 (fsck: convert gitmodules url to URL
> passed to curl, 2020-04-18) be thin wrappers for it.
> 
> As the latter of those notes the fsck version was copied from the
> initial builtin/submodule--helper.c version.
> 
> Since the code added in a2b26ffb1a8 was doing really doing the same as

s/doing really doing/really doing/

> win32_is_dir_sep() added in 1cadad6f658 (git clone <url>
> C:\cygwin\home\USER\repo' is working (again), 2018-12-15) let's move
> the latter to git-compat-util.h is a is_xplatform_dir_sep(). We can
> then call either it or the platform-specific is_dir_sep() from this
> new function.
> 
> Let's likewise change code in various other places that was hardcoding
> checks for "'/' || '\\'" with the new is_xplatform_dir_sep(). As can
> be seen in those callers some of them still concern themselves with
> ':' (Mac OS classic?), but let's leave the question of whether that
> should be consolidated for some other time.

This feels like it could be its own change before the refactor
of the starts_with_dot_{,dot}slash() functions. The diff is pretty
big and all over the place.

If you start with the addition of is_xplatform_dir_sep() (and maybe
the change of how is_dir_sep() is created) then the rest of the
change is more focused.
 
> As we expect to make wider use of the "native" case in the future,
> define and use two starts_with_dot_{,dot_}slash_native() convenience
> wrappers. This makes the diff in builtin/submodule--helper.c much
> smaller.

> +static int starts_with_dot_slash(const char *const path)
> +{
> +	return starts_with_dot_slash_native(path);;

Double semi-colon.

> +int path_match_flags(const char *const str, const enum path_match_flags flags)

I feel like "path_match_flags()" is too generic of a name here.

Maybe something like "path_starts_with_dotslash_flags()" would be
sufficiently descriptive.

> +{
> +	const char *p = str;
> +
> +	if (flags & PATH_MATCH_NATIVE &&
> +	    flags & PATH_MATCH_XPLATFORM)
> +		BUG("path_match_flags() must get one match kind, not multiple!");
> +	else if (!(flags & PATH_MATCH_KINDS_MASK))
> +		BUG("path_match_flags() must get at least one match kind!");
> +
> +	if (flags & PATH_MATCH_STARTS_WITH_DOT_SLASH &&
> +	    flags & PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH)
> +		BUG("path_match_flags() must get one platform kind, not multiple!");
> +	else if (!(flags & PATH_MATCH_PLATFORM_MASK))
> +		BUG("path_match_flags() must get at least one platform kind!");

These would be easier and more robust if we had a simple
popcount function. It's not worth extracting one out of
ewah/ewok.h just for this, though.

> +	if (*p++ != '.')
> +		return 0;
> +	if (flags & PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH &&
> +	    *p++ != '.')
> +		return 0;
> +
> +	if (flags & PATH_MATCH_NATIVE)
> +		return is_dir_sep(*p);
> +	else if (flags & PATH_MATCH_XPLATFORM)
> +		return is_xplatform_dir_sep(*p);
> +	BUG("unreachable");
> +}

Thanks,
-Stolee