git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 6/8] dir: avoid unnecessary traversal into ignored directory

From
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Date
May 11, 2021, 18:34 UTC
Message-ID
<89cc01ef8598b5faa529a4aea7e1e296465ef8e3.1620758049.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1020.v4.git.git.1620758049.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>

The show_other_directories case in treat_directory() tried to handle both excludes and untracked files with the same logic, and mishandled both the excludes and the untracked files in the process, in different ways. Split that logic apart, and then focus on the logic for the excludes; a subsequent commit will address the logic for untracked files.

For show_other_directories, an excluded directory means that every path underneath that directory will also be excluded. Given that the calling code requested to just show directories when everything under a directory had the same state (that's what the "DIR_SHOW_OTHER_DIRECTORIES" flag means), we generally do not need to traverse into such directories and can just immediately mark them as ignored (i.e. as path_excluded). The only reason we cannot just immediately return path_excluded is the DIR_HIDE_EMPTY_DIRECTORIES flag and the possibility that the ignored directory is an empty directory. The code previously treated DIR_SHOW_IGNORED_TOO in most cases as an exception as well, which was wrong. It can sometimes reduce the number of cases where we need to recurse (namely if DIR_SHOW_IGNORED_TOO_MODE_MATCHING is also set), but should not be able to increase the number of cases where we need to recurse. Fix the logic accordingly.

Some sidenotes about possible confusion with dir.c:
* "ignored" often refers to an untracked ignore", i.e. a file which is
  not tracked which matches one of the ignore/exclusion rules.  But you
  can also have a "tracked ignore", a tracked file that happens to match
  one of the ignore/exclusion rules and which dir.c has to worry about
  since "git ls-files -c -i" is supposed to list them.
* The dir code often uses "ignored" and "excluded" interchangeably,
  which you need to keep in mind while reading the code.
* "exclude" is used multiple ways in the code:
  * As noted above, "exclude" is often a synonym for "ignored".
  * The logic for parsing .gitignore files was re-used in
    .git/info/sparse-checkout, except there it is used to mark paths that
    the user wants to *keep*.  This was mostly addressed by commit
    65edd96aec ("treewide: rename 'exclude' methods to 'pattern'",
    2019-09-03), but every once in a while you'll find a comment about
    "exclude" referring to these patterns that might in fact be in use
    by the sparse-checkout machinery for inclusion rules.
  * The word "EXCLUDE" is also used for pathspec negation, as in
      (pathspec->items[3].magic & PATHSPEC_EXCLUDE)
    Thus if a user had a .gitignore file containing
      *~
      *.log
      !settings.log
    And then ran
      git add -- 'settings.*' ':^settings.log'
    Then :^settings.log is a pathspec negation making settings.log not
    be requested to be added even though all other settings.* files are
    being added.  Also, !settings.log in the gitignore file is a negative
    exclude pattern meaning that settings.log is normally a file we
    want to track even though all other *.log files are ignored.

Sometimes it feels like dir.c needs its own glossary with its many definitions, including the multiply-defined terms.

Reported-by: Jason Gore <Jason.Gore@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 dir.c            | 44 +++++++++++++++++++++++++++++---------------
 t/t7300-clean.sh |  2 +-
 2 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/dir.c b/dir.c
index 69b8c9d7f9fb..0126e2f08af7 100644
--- a/dir.c
+++ b/dir.c
@@ -1844,6 +1844,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 	}
 
 	/* This is the "show_other_directories" case */
+	assert(dir->flags & DIR_SHOW_OTHER_DIRECTORIES);
 
 	/*
 	 * If we have a pathspec which could match something _below_ this
@@ -1854,27 +1855,40 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 	if (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC)
 		return path_recurse;
 
+	/* Special cases for where this directory is excluded/ignored */
+	if (excluded) {
+		/*
+		 * In the show_other_directories case, if we're not
+		 * hiding empty directories, there is no need to
+		 * recurse into an ignored directory.
+		 */
+		if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
+			return path_excluded;
+
+		/*
+		 * Even if we are hiding empty directories, we can still avoid
+		 * recursing into ignored directories for DIR_SHOW_IGNORED_TOO
+		 * if DIR_SHOW_IGNORED_TOO_MODE_MATCHING is also set.
+		 */
+		if ((dir->flags & DIR_SHOW_IGNORED_TOO) &&
+		    (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING))
+			return path_excluded;
+	}
+
 	/*
-	 * Other than the path_recurse case immediately above, we only need
-	 * to recurse into untracked/ignored directories if either of the
-	 * following bits is set:
+	 * Other than the path_recurse case above, we only need to
+	 * recurse into untracked directories if either of the following
+	 * bits is set:
 	 *   - DIR_SHOW_IGNORED_TOO (because then we need to determine if
 	 *                           there are ignored entries below)
 	 *   - DIR_HIDE_EMPTY_DIRECTORIES (because we have to determine if
 	 *                                 the directory is empty)
 	 */
-	if (!(dir->flags & (DIR_SHOW_IGNORED_TOO | DIR_HIDE_EMPTY_DIRECTORIES)))
-		return excluded ? path_excluded : path_untracked;
-
-	/*
-	 * ...and even if DIR_SHOW_IGNORED_TOO is set, we can still avoid
-	 * recursing into ignored directories if the path is excluded and
-	 * DIR_SHOW_IGNORED_TOO_MODE_MATCHING is also set.
-	 */
-	if (excluded &&
-	    (dir->flags & DIR_SHOW_IGNORED_TOO) &&
-	    (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING))
-		return path_excluded;
+	if (!excluded &&
+	    !(dir->flags & (DIR_SHOW_IGNORED_TOO |
+			    DIR_HIDE_EMPTY_DIRECTORIES))) {
+		return path_untracked;
+	}
 
 	/*
 	 * Even if we don't want to know all the paths under an untracked or
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 34c08c325407..21e48b3ba591 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -746,7 +746,7 @@ test_expect_success 'clean untracked paths by pathspec' '
 	test_must_be_empty actual
 '
 
-test_expect_failure 'avoid traversing into ignored directories' '
+test_expect_success 'avoid traversing into ignored directories' '
 	test_when_finished rm -f output error trace.* &&
 	test_create_repo avoid-traversing-deep-hierarchy &&
 	(
-- 
gitgitgadget
Previous: Elijah Newren via GitGitGadgetNext: Derrick Stolee via GitGitGadget
Message 72 of 90 in “Directory traversal fixes”
  1. 0/5 Directory traversal fixesElijah Newren via GitGitGadget, May 7, 2021
  2. 1/5 t7300: add testcase showing unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 7, 2021
  3. Eric SunshineMay 7, 2021
  4. Elijah NewrenMay 7, 2021
  5. Eric SunshineMay 7, 2021
  6. Elijah NewrenMay 7, 2021
  7. Eric SunshineMay 7, 2021
  8. Jeff KingMay 7, 2021
  9. Eric SunshineMay 7, 2021
  10. Elijah NewrenMay 8, 2021
  11. Eric SunshineMay 8, 2021
  12. Elijah NewrenMay 8, 2021
  13. Philip OakleyMay 8, 2021
  14. Elijah NewrenMay 8, 2021
  15. 2/5 t3001, t7300: add testcase showcasing missed directory traversalElijah Newren via GitGitGadget, May 7, 2021
  16. 3/5 dir: avoid unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 7, 2021
  17. 4/5 dir: traverse into untracked directories if they may have ignored subfilesElijah Newren via GitGitGadget, May 7, 2021
  18. 5/5 [RFC] ls-files: error out on -i unless -o or -c are specifiedElijah Newren via GitGitGadget, May 7, 2021
  19. 6/5 dir: update stale description of treat_directory()Derrick Stolee, May 7, 2021
  20. Elijah NewrenMay 7, 2021
  21. Derrick StoleeMay 7, 2021
  22. 0/8 Directory traversal fixesElijah Newren via GitGitGadget, May 8, 2021
  23. 1/8 t7300: add testcase showing unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 8, 2021
  24. Junio C HamanoMay 8, 2021
  25. Elijah NewrenMay 8, 2021
  26. Junio C HamanoMay 8, 2021
  27. Elijah NewrenMay 8, 2021
  28. 3/8 dir: avoid unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 8, 2021
  29. 2/8 t3001, t7300: add testcase showcasing missed directory traversalElijah Newren via GitGitGadget, May 8, 2021
  30. 4/8 dir: traverse into untracked directories if they may have ignored subfilesElijah Newren via GitGitGadget, May 8, 2021
  31. 5/8 [RFC] ls-files: error out on -i unless -o or -c are specifiedElijah Newren via GitGitGadget, May 8, 2021
  32. 6/8 dir: update stale description of treat_directory()Derrick Stolee via GitGitGadget, May 8, 2021
  33. 7/8 [RFC] dir: convert trace calls to trace2 equivalentsElijah Newren via GitGitGadget, May 8, 2021
  34. 8/8 [RFC] dir: reported number of visited directories and paths with trace2Elijah Newren via GitGitGadget, May 8, 2021
  35. 0/8 Directory traversal fixesElijah Newren via GitGitGadget, May 8, 2021
  36. 1/8 [RFC] dir: convert trace calls to trace2 equivalentsElijah Newren via GitGitGadget, May 8, 2021
  37. Junio C HamanoMay 10, 2021
  38. Elijah NewrenMay 11, 2021
  39. Jeff HostetlerMay 11, 2021
  40. Elijah NewrenMay 11, 2021
  41. 2/8 [RFC] dir: report number of visited directories and paths with trace2Elijah Newren via GitGitGadget, May 8, 2021
  42. Junio C HamanoMay 10, 2021
  43. 3/8 [RFC] ls-files: error out on -i unless -o or -c are specifiedElijah Newren via GitGitGadget, May 8, 2021
  44. Junio C HamanoMay 10, 2021
  45. Elijah NewrenMay 11, 2021
  46. Junio C HamanoMay 11, 2021
  47. 4/8 t7300: add testcase showing unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 8, 2021
  48. Junio C HamanoMay 10, 2021
  49. Elijah NewrenMay 11, 2021
  50. Junio C HamanoMay 11, 2021
  51. Elijah NewrenMay 12, 2021
  52. Junio C HamanoMay 12, 2021
  53. 5/8 t3001, t7300: add testcase showcasing missed directory traversalElijah Newren via GitGitGadget, May 8, 2021
  54. 6/8 dir: avoid unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 8, 2021
  55. Junio C HamanoMay 10, 2021
  56. Elijah NewrenMay 11, 2021
  57. 7/8 dir: traverse into untracked directories if they may have ignored subfilesElijah Newren via GitGitGadget, May 8, 2021
  58. 8/8 dir: update stale description of treat_directory()Derrick Stolee via GitGitGadget, May 8, 2021
  59. 0/8 Directory traversal fixesElijah Newren via GitGitGadget, May 11, 2021
  60. 1/8 dir: convert trace calls to trace2 equivalentsElijah Newren via GitGitGadget, May 11, 2021
  61. Jeff HostetlerMay 11, 2021
  62. Elijah NewrenMay 11, 2021
  63. Jeff HostetlerMay 11, 2021
  64. Elijah NewrenMay 12, 2021
  65. Jeff HostetlerMay 12, 2021
  66. Elijah NewrenMay 12, 2021
  67. 3/8 ls-files: error out on -i unless -o or -c are specifiedElijah Newren via GitGitGadget, May 11, 2021
  68. 2/8 dir: report number of visited directories and paths with trace2Elijah Newren via GitGitGadget, May 11, 2021
  69. 4/8 t7300: add testcase showing unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 11, 2021
  70. 5/8 t3001, t7300: add testcase showcasing missed directory traversalElijah Newren via GitGitGadget, May 11, 2021
  71. 7/8 dir: traverse into untracked directories if they may have ignored subfilesElijah Newren via GitGitGadget, May 11, 2021
  72. 6/8 dir: avoid unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 11, 2021
  73. 8/8 dir: update stale description of treat_directory()Derrick Stolee via GitGitGadget, May 11, 2021
  74. 0/9 Directory traversal fixesElijah Newren via GitGitGadget, May 12, 2021
  75. 1/9 dir: convert trace calls to trace2 equivalentsElijah Newren via GitGitGadget, May 12, 2021
  76. 2/9 dir: report number of visited directories and paths with trace2Elijah Newren via GitGitGadget, May 12, 2021
  77. 4/9 t7300: add testcase showing unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 12, 2021
  78. 6/9 dir: avoid unnecessary traversal into ignored directoryElijah Newren via GitGitGadget, May 12, 2021
  79. 8/9 dir: update stale description of treat_directory()Derrick Stolee via GitGitGadget, May 12, 2021
  80. Derrick StoleeMay 17, 2021
  81. Junio C HamanoMay 17, 2021
  82. Elijah NewrenMay 18, 2021
  83. Junio C HamanoMay 19, 2021
  84. 7/9 dir: traverse into untracked directories if they may have ignored subfilesElijah Newren via GitGitGadget, May 12, 2021
  85. 3/9 ls-files: error out on -i unless -o or -c are specifiedElijah Newren via GitGitGadget, May 12, 2021
  86. 5/9 t3001, t7300: add testcase showcasing missed directory traversalElijah Newren via GitGitGadget, May 12, 2021
  87. 9/9 dir: introduce readdir_skip_dot_and_dotdot() helperElijah Newren via GitGitGadget, May 12, 2021
  88. Derrick StoleeMay 17, 2021
  89. Elijah NewrenMay 18, 2021
  90. Derrick StoleeMay 17, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.