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

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