On 5/7/2021 12:04 AM, Elijah Newren via GitGitGadget wrote:
> This patchset fixes a few directory traversal issues, where fill_directory()
> would traverse into directories that it shouldn't and not traverse into
> directories that it should. One of these issues was reported recently on
> this list[1], another was found at $DAYJOB.
>
> The fifth patch might have backward compatibility implications, but is easy
> to review. Even if the logic in dir.c makes your eyes glaze over, at least
> take a look at the fifth patch.
My eyes were glazing over, so I went to read the whole treat_directory()
method and its related documentation comment. I found it to be a bit
confusing that it was referencing names that were deprecated 12 years ago.
Here is a patch that you could add to this series to improve these
comments.
Thanks,
-Stolee
-- >8 --
>From 587a94ac396c969b6e7734ee46afeac20e87ccb9 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
Date: Fri, 7 May 2021 12:14:13 -0400
Subject: [PATCH] dir: update stale description of treat_directory()
The documentation comment for treat_directory() was originally written
in 095952 (Teach directory traversal about subprojects, 2007-04-11)
which was before the 'struct dir_struct' split its bitfield of named
options into a 'flags' enum in 7c4c97c0 (Turn the flags in struct
dir_struct into a single variable, 2009-02-16). When those flags
changed, the comment became stale, since members like
'show_other_directories' transitioned into flags like
DIR_SHOW_OTHER_DIRECTORIES.
Update the comments for treat_directory() to use these flag names rather
than the old member names.
Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
---
dir.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/dir.c b/dir.c
index 3beb8e17a83..0a0138bc1aa 100644
--- a/dir.c
+++ b/dir.c
@@ -1749,13 +1749,13 @@ static enum exist_status directory_exists_in_index(struct index_state *istate,
* Case 3: if we didn't have it in the index previously, we
* have a few sub-cases:
*
- * (a) if "show_other_directories" is true, we show it as
- * just a directory, unless "hide_empty_directories" is
+ * (a) if DIR_SHOW_OTHER_DIRECTORIES flag is set, we show it as
+ * just a directory, unless DIR_HIDE_EMPTY_DIRECTORIES is
* also true, in which case we need to check if it contains any
* untracked and / or ignored files.
- * (b) if it looks like a git directory, and we don't have
- * 'no_gitlinks' set we treat it as a gitlink, and show it
- * as a directory.
+ * (b) if it looks like a git directory and we don't have the
+ * DIR_NO_GITLINKS flag, then we treat it as a gitlink, and
+ * show it as a directory.
* (c) otherwise, we recurse into it.
*/
static enum path_treatment treat_directory(struct dir_struct *dir,
@@ -1843,7 +1843,6 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
return path_recurse;
}
- /* This is the "show_other_directories" case */
assert(dir->flags & DIR_SHOW_OTHER_DIRECTORIES);
/*
@@ -1858,7 +1857,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
/* Special cases for where this directory is excluded/ignored */
if (excluded) {
/*
- * In the show_other_directories case, if we're not
+ * If DIR_SHOW_OTHER_DIRECTORIES is set and we're not
* hiding empty directories, there is no need to
* recurse into an ignored directory.
*/
--
2.31.1.vfs.0.0.80.gb082c853c0e
