From: Junio C Hamano Date: Fri, 04 Sep 2026 16:43:31 GMT Subject: Re: [PATCH v2 2/2] dir: find common prefix among non-exclude pathspec items Message-ID: In-Reply-To: Elijah Newren 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=" 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.