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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2026, 16:43 UTC
Message-ID: <xmqqy0dh3r2k.fsf@gitster.g>
In-Reply-To: <CABPp-BF6hps9DibSV4ghbowkOD-NfEsHYFdLoKab0hCfEi9rgw@mail.gmail.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,...

Show 16 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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