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

Elijah Newren [off-list ref] 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:

quoted

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