Fix ancestor exclusion checks after gitignore negations

CHANGES_1.in.rst

@@ -20,6 +20,8 @@ New features:
 
 Bug fixes:
 
+- Directory negations no longer re-include descendants of an excluded ancestor.
+  All backends return the excluding ancestor's pattern index from ``check_file``.
 - Hyperscan backend no longer fails on multibyte UTF-8 characters.
 - `Pull #123`_: Ignore invalid gitignore bracket ranges for `GitIgnoreSpec`.
 - `Pull #128`_: Support POSIX character classes (e.g. `[[:alpha:]]`) in gitignore bracket expressions.

pathspec/_backends/hyperscan/gitignore.py

@@ -222,7 +222,7 @@ def _match(
 		"""
 		Implements :meth:`match_file`. *check_ancestors* (:class:`bool`) is
 		whether to ask if an ancestor directory of *file* is excluded; see
-		:meth:`_ancestor_excluded` for why it can be skipped.
+		:meth:`_match_ancestor` for why it can be skipped.
 		"""
 		# NOTICE: According to benchmarking, a method callback is 13% faster than
 		# using a closure here.
@@ -236,9 +236,14 @@ def _match(
 		db.scan(file.encode('utf8'), match_event_handler=self.__on_match)
 
 		dir_include, dir_index, file_include, file_index = self._out
-		if dir_include and check_ancestors and self._ancestor_excluded(file):
-			out_include, out_index = dir_include, dir_index
-		elif file_include is not None:
+		if check_ancestors and (dir_include is not None or file_include is not None):
+			# Neither file nor directory negations override an excluded ancestor.
+			# Some patterns (e.g. "a/**") match without a directory marker.
+			ancestor_index = self._match_ancestor(file)
+			if ancestor_index is not None:
+				return (True, ancestor_index)
+
+		if file_include is not None:
 			out_include, out_index = file_include, file_index
 		elif dir_include:
 			# An ancestor matched an exclude pattern, but the spec as a whole
@@ -249,26 +254,26 @@ def _match(
 
 		return (out_include, out_index if out_index != -1 else None)
 
-	def _ancestor_excluded(self, file: str) -> bool:
+	def _match_ancestor(self, file: str) -> Optional[int]:
 		"""
-		Whether any strict ancestor directory of *file* is excluded. Git stops
-		descending at the first excluded directory, so the ancestors are asked
-		outermost first, each as a directory query (trailing slash included).
+		Return the excluding pattern index for a strict ancestor, or :data:`None`.
+		Git stops descending at the first excluded directory, so the ancestors are
+		asked outermost first, each as a directory query (trailing slash included).
 		By the time an ancestor is asked, every ancestor above it is known not to
 		be excluded, so it is matched without checking its own ancestors again;
 		otherwise each level re-asks all the levels above it and the work grows
 		exponentially with depth.
 		"""
 		index = file.find('/')
 		while index != -1 and index + 1 < len(file):
-			ancestor_include, _ancestor_index = self._match(
+			ancestor_include, ancestor_index = self._match(
 				file[:index + 1], check_ancestors=False,
 			)
 			if ancestor_include:
-				return True
+				return ancestor_index
 			index = file.find('/', index + 1)
 
-		return False
+		return None
 
 
 	@override

pathspec/_backends/re2/gitignore.py

@@ -164,7 +164,7 @@ def _match(
 		"""
 		Implements :meth:`match_file`. *check_ancestors* (:class:`bool`) is
 		whether to ask if an ancestor directory of *file* is excluded; see
-		:meth:`_ancestor_excluded` for why it can be skipped.
+		:meth:`_match_ancestor` for why it can be skipped.
 		"""
 		# Find best match.
 		match_ids: Optional[list[int]] = self._set.Match(file)  # type: ignore[assignment]
@@ -197,9 +197,14 @@ def _match(
 				file_index = index
 
 		assert dir_index != -1 or file_index != -1, (dir_index, file_index)
-		if dir_include and check_ancestors and self._ancestor_excluded(file):
-			return (dir_include, dir_index)
-		elif file_include is not None:
+		if check_ancestors and (dir_include is not None or file_include is not None):
+			# Neither file nor directory negations override an excluded ancestor.
+			# Some patterns (e.g. "a/**") match without a directory marker.
+			ancestor_index = self._match_ancestor(file)
+			if ancestor_index is not None:
+				return (True, ancestor_index)
+
+		if file_include is not None:
 			return (file_include, file_index)
 		elif dir_include:
 			# An ancestor matched an exclude pattern, but the spec as a whole
@@ -208,23 +213,23 @@ def _match(
 		else:
 			return (dir_include, dir_index)
 
-	def _ancestor_excluded(self, file: str) -> bool:
+	def _match_ancestor(self, file: str) -> Optional[int]:
 		"""
-		Whether any strict ancestor directory of *file* is excluded. Git stops
-		descending at the first excluded directory, so the ancestors are asked
-		outermost first, each as a directory query (trailing slash included).
+		Return the excluding pattern index for a strict ancestor, or :data:`None`.
+		Git stops descending at the first excluded directory, so the ancestors are
+		asked outermost first, each as a directory query (trailing slash included).
 		By the time an ancestor is asked, every ancestor above it is known not to
 		be excluded, so it is matched without checking its own ancestors again;
 		otherwise each level re-asks all the levels above it and the work grows
 		exponentially with depth.
 		"""
 		index = file.find('/')
 		while index != -1 and index + 1 < len(file):
-			ancestor_include, _ancestor_index = self._match(
+			ancestor_include, ancestor_index = self._match(
 				file[:index + 1], check_ancestors=False,
 			)
 			if ancestor_include:
-				return True
+				return ancestor_index
 			index = file.find('/', index + 1)
 
-		return False
+		return None

pathspec/_backends/simple/gitignore.py

@@ -70,7 +70,7 @@ def _match(
 		"""
 		Implements :meth:`match_file`. *check_ancestors* (:class:`bool`) is
 		whether to ask if an ancestor directory of *file* is excluded; see
-		:meth:`_ancestor_excluded` for why it can be skipped.
+		:meth:`_match_ancestor` for why it can be skipped.
 		"""
 		is_reversed = self._is_reversed
 
@@ -112,9 +112,14 @@ def _match(
 					file_include = include
 					file_index = index
 
-		if dir_include and check_ancestors and self._ancestor_excluded(file):
-			return (dir_include, dir_index)
-		elif file_include is not None:
+		if check_ancestors and (dir_include is not None or file_include is not None):
+			# Neither file nor directory negations override an excluded ancestor.
+			# Some patterns (e.g. "a/**") match without a directory marker.
+			ancestor_index = self._match_ancestor(file)
+			if ancestor_index is not None:
+				return (True, ancestor_index)
+
+		if file_include is not None:
 			return (file_include, file_index)
 		elif dir_include:
 			# An ancestor matched an exclude pattern, but the spec as a whole
@@ -123,24 +128,24 @@ def _match(
 		else:
 			return (dir_include, dir_index)
 
-	def _ancestor_excluded(self, file: str) -> bool:
+	def _match_ancestor(self, file: str) -> Optional[int]:
 		"""
-		Whether any strict ancestor directory of *file* is excluded. Git stops
-		descending at the first excluded directory, so the ancestors are asked
-		outermost first, each as a directory query (trailing slash included).
+		Return the excluding pattern index for a strict ancestor, or :data:`None`.
+		Git stops descending at the first excluded directory, so the ancestors are
+		asked outermost first, each as a directory query (trailing slash included).
 		By the time an ancestor is asked, every ancestor above it is known not to
 		be excluded, so it is matched without checking its own ancestors again;
 		otherwise each level re-asks all the levels above it and the work grows
 		exponentially with depth.
 		"""
 		index = file.find('/')
 		while index != -1 and index + 1 < len(file):
-			ancestor_include, _ancestor_index = self._match(
+			ancestor_include, ancestor_index = self._match(
 				file[:index + 1], check_ancestors=False,
 			)
 			if ancestor_include:
-				return True
+				return ancestor_index
 			index = file.find('/', index + 1)
 
-		return False
+		return None
 

tests/test_06_gitignore.py

@@ -1139,7 +1139,7 @@ def test_c(self):
 
 	def test_d(self):
 		"""
-		Document terminal-globstar negation behavior without changing resolution.
+		Test terminal-globstar negation with excluded ancestor directories.
 		"""
 		for sub_test in self.parameterize_from_lines([
 			"d/**/",
@@ -1153,10 +1153,8 @@ def test_d(self):
 				self.assertFalse(spec.match_file("d/child/"))
 				self.assertFalse(spec.match_file("d/child/top.txt"))
 				self.assertTrue(spec.match_file("d/child/grand/"))
-				# Existing flat last-match resolution lets the negated directory
-				# pattern match this descendant too. Git's ancestor-sensitive result
-				# differs, but changing that separate behavior is outside this fix.
-				self.assertFalse(spec.match_file("d/child/grand/deep.txt"))
+				# Re-including child does not re-include the excluded grand directory.
+				self.assertTrue(spec.match_file("d/child/grand/deep.txt"))
 
 		for sub_test in self.parameterize_from_lines(["!d/**/"]):
 			with sub_test() as spec:

tests/test_09_gitignore_ancestor_negation.py

@@ -0,0 +1,64 @@
+"""Test directory negations below an excluded ancestor."""
+
+from unittest import TestCase
+from unittest.mock import patch
+
+from .test_06_gitignore import GitIgnoreSpecMixin
+
+
+class AncestorNegationTest(GitIgnoreSpecMixin, TestCase):
+
+	def test_excluded_ancestor(self):
+		# Expected ignore decisions and pattern indices verified with
+		# git check-ignore --no-index (Git 2.52.0.windows.1).
+		for lines, paths in (
+			(['a/', '!a/b/'], ['a/b/file.txt', 'a/b/c/deep.txt']),
+			(['a/', '!a/b/', '!*.txt'], ['a/b/file.txt']),
+			(['a/', '!a/b/', '*.txt'], ['a/b/file.txt']),
+			(['d/**/', '!d/child/'], ['d/child/grand/deep.txt']),
+			(['a/**', '!a/b/**'], ['a/b/file.txt', 'a/b/c/deep.txt']),
+			(['a/**', '!*.txt'], ['a/b/file.txt']),
+			(['*', '!*.txt'], ['a/top.txt', 'a/b/file.txt']),
+		):
+			for sub_test in self.parameterize_from_lines(lines):
+				with sub_test() as spec:
+					results = list(spec.check_files(iter(paths)))
+					self.assertEqual(
+						[(r.include, r.index) for r in results],
+						[(True, 0)] * len(paths),
+						(lines, results),
+					)
+					self.assertEqual(list(spec.match_files(iter(paths))), paths)
+					self.assertEqual(list(spec.match_files(iter(paths), negate=True)), [])
+
+	def test_reincluded_ancestors(self):
+		for lines, path in (
+			(['a/', '!a/', '!a/b/'], 'a/b/file.txt'),
+			(['a/*', '!a/b/'], 'a/b/file.txt'),
+			(['d/**/', '!d/child/'], 'd/child/top.txt'),
+			(['d/**/', '!d/child/', '!d/child/grand/'], 'd/child/grand/deep.txt'),
+			(['!a/b/'], 'a/b/file.txt'),
+			(['a/**', '!*.txt'], 'a/top.txt'),
+			(['*', '!a/', '!a/**'], 'a/b/file.txt'),
+		):
+			for sub_test in self.parameterize_from_lines(lines):
+				with sub_test() as spec:
+					self.assertFalse(spec.match_file(path), (lines, path))
+
+	def test_ancestors_checked_once(self):
+		# Keep the ancestor walk iterative even with many re-included levels.
+		path = 'a/' * 40 + 'file.txt'
+		for sub_test in self.parameterize_from_lines(['a/', '!a/']):
+			with sub_test() as spec:
+				backend_type = type(spec._backend)
+				original = backend_type._match
+				calls = []
+
+				def counted_match(backend, file, check_ancestors):
+					calls.append((file, check_ancestors))
+					return original(backend, file, check_ancestors)
+
+				with patch.object(backend_type, '_match', counted_match):
+					self.assertFalse(spec.match_file(path))
+				self.assertLessEqual(len(calls), 41)
+				self.assertTrue(all(not check for _, check in calls[1:]))