Re: [PATCH v2 2/2] dir: find common prefix among non-exclude pathspec items

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

Elijah Newren <newren@gmail.com> 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.