Re: [PATCH] dir: find common prefix among positive pathspecs

[Junio C Hamano <gitster@pobox.com>]

Yannik Tausch <dev@ytausch.de> writes:

> common_prefix_len() skips exclude pathspec items, but uses n == 0 to
> identify the initial item and items[0] as the comparison source. When
> an exclude item comes first, the function returns zero even when all
> positive pathspecs share a directory.
>
> Track the first positive item explicitly. Return its match and the
> common prefix length together so that common_prefix() and
> fill_directory() use the correct string. Add a unit test with an
> unrelated exclude before two positive pathspecs that share a directory.
>
> Signed-off-by: Yannik Tausch <dev@ytausch.de>
> ---
>
> This patch is based on https://lore.kernel.org/git/0CA8678D-0540-4A2E-B314-B9BEB04E2BF5@ytausch.de/T/#u.

I am not sure what you mean.  Do you mean that the other one should
have been marked as [PATCH 1/2] and this one [PATCH 2/2]?  The way
we use the phrase "based on" does not exactly match that situation.
It is more like "This patch applies on top of the other one", or
"This patch depends on the other one."

> -static size_t common_prefix_len(const struct pathspec *pathspec)
> +struct pathspec_prefix {
> +	const char *match;
> +	size_t len;
> +};
> +
> +/*
> + * Find the common prefix of positive pathspec items. The returned match
> + * points into the first positive item and is not NUL-terminated at len.
> + */
> +static struct pathspec_prefix find_common_prefix(const struct pathspec *pathspec)

Our norm in C is not to pass structures by value either as parameter
of as return value, unless there is a very good reason to do so.

Since we can easily use

	const char *common_prefix(const sturct pathspec *pathspec, size_t *len);

to return .match and store the length in *len when we return, we
cannot say that this case has a very good reason to use a structure
passed by value.

Actually, I have a feeling that we do not want find_common_prefix()
helper.  Instead perhaps

    static size_t common_prefix_len(const struct pathspec *pathspec,
				    const char **matched_prefix)

may be an alternative that is easier to work with.  Because the
existing callers assume that pathspec->items[0].match is where they
can grab the common prefix from, they should look like

	len = common_prefix_len(pathspec);
	... use the first len bytes of pathspec->items[0].match[] ...

They want to be told to do this instead now:

	const char *common_prefix;

	len = common_prefix_len(pathspec, &common_prefix);
	... use the first len bytes of common_prefix[] ...

In "use the first len bytes" logic they already have, they know not
to memdup when len == 0 (and ignore pathspec->items[0].match[] in
that case), and they know they need to memdup if they want to have
their own copies, etc., so the changes to them can be kept to the
minimum.

> +	prefix.match = first < 0 ? NULL : pathspec->items[first].match;
> +	prefix.len = max;
> +	return prefix;

So instead of these three lines, your return sequence would become

	*matched_prefix = first < 0 ? NULL : pathspec->items[first].match;
	return max;

If there is no positive element in the given pathspec (by the way,
"pathspec" refers to the whole set, and each element in it may be
either positive or negative, so "positive pathspec(s)" is a
misnomer),  the loop never touches first or max, so when the loop
exits, we won't have "match" and "len" is 0.  Your changes in the
loop to avoid assuming [0] is positive element all look correct.