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

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 21, 2022, 17:26 UTC
Message-ID: <04b363f3-58d7-5cfc-6261-0ebcca18ecec@github.com>
In-Reply-To: <RFC-patch-v2-02.36-8f6e4f12e8a-20220418T165545Z-avarab@gmail.com>

On 4/18/2022 1:23 PM, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

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

Show 11 quoted lines

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

Show 14 quoted lines

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

Show 12 quoted lines

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