Elijah Newren <newren@xxxxxxxxx> writes:
> This to me looked more like what you are changing, and I had a hard
> time figuring out why you were changing it.
While I share this assessment,...
>
> Does the following alternative correctly capture your intent and change here? :
>
>
> dir: preserve pathspec prefix optimization with leading excludes
>
> Directory walks use the common directory prefix of non-exclude
> pathspec items to avoid scanning unrelated portions of the working
> tree or index. Exclude items only remove paths from that candidate
> set, so they do not need to widen the traversal.
>
> When an exclude item is the first pathspec item,
> common_prefix_len() fails to establish a comparison base and returns
> a zero-length prefix. The result is correct, but git unnecessarily
> traverses from a broader starting point even when all non-exclude
> items share a directory.
... I do not think this is true.
What happens inside dir.c::fill_directory() is driven only with the
return value of common_prefix_len(), which already ignores and has
always ignored the negative pathspec elements.
What this [2/2] changes is what string common_prefix() returns. If
you have "!x/b" "a/b" "a/c", common_prefix_len() goes over the two
positive ones "a/b" and "a/c" and correctly notices that "a/" is
common among the positive ones and its length is 2.
The problem this patch fixes is that common_prefix() used to always
grab the first two bytes of the element that happens to be at the
beginning of pathspec, so a pathspec ("!x/b" "a/b" "a/c") would have
given you "!x" as the common prefix string, which obviously is
bogus. The common_prefix() is only used in two code paths that are
quite distant from here. It is clear there is a bug (i.e., the code
that wants to be passed "a/" in such a case cannot be happy to see
"!x" instead), but it is totally unclear what the end-user visible
effect of that bug (i.e. what happens when overlay_tree_on_index()
passes an incorrectly computed common_prefix() when "git ls-files"
is run with "--with-tree=<treeish>" option?).
> Use the first non-exclude item as the comparison base and return its
> string together with the prefix length, allowing callers to start
> from the recovered directory prefix. Exclude matching continues to
> use full paths, so this restores the optimization without changing
> which paths are selected. Add a unit test covering an exclude item
> before two non-exclude items with a common directory.
I do not think this is what this patch does. What you are
describing is this bit:
>> -static size_t common_prefix_len(const struct pathspec *pathspec)
>> ...
>> size_t i = 0, len = 0, item_len;
>> if (pathspec->items[n].magic & PATHSPEC_EXCLUDE)
>> continue;
which dates back to the very beginning of negative pathspec elements
support introduced at ef79b1f870 (Support pathspec magic :(exclude)
and its short form :!, 2013年12月06日), I think.