[3/6] dir: fix confusion based on variable tense

Commit Message

Linus Arver via GitGitGadget Jan. 29, 2020, 10:03 p.m. UTC

From: Elijah Newren <[e-mail]>

Despite having contributed several fixes in this area, I have for months
(years?) assumed that the "exclude" variable was a directive; this
caused me to think of it as a different mode we operate in and left me
confused as I tried to build up a mental model around why we'd need such
a directive.  I mostly tried to ignore it while focusing on the pieces I
was trying to understand.

Then I finally traced this variable all back to a call to is_excluded(),
meaning it was actually functioning as an adjective.  In particular, it
was a checked property ("Does this path match a rule in .gitignore?"),
rather than a mode passed in from the caller.  Change the variable name
to match the part of speech used by the function called to define it,
which will hopefully make these bits of code slightly clearer to the
next reader.

Signed-off-by: Elijah Newren <[e-mail]>
---
 dir.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comments

Derrick Stolee Jan. 30, 2020, 3:20 p.m. UTC | #1

On 1/29/2020 5:03 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> Despite having contributed several fixes in this area, I have for months
> (years?) assumed that the "exclude" variable was a directive; this
> caused me to think of it as a different mode we operate in and left me
> confused as I tried to build up a mental model around why we'd need such
> a directive.  I mostly tried to ignore it while focusing on the pieces I
> was trying to understand.
> 
> Then I finally traced this variable all back to a call to is_excluded(),
> meaning it was actually functioning as an adjective.  In particular, it
> was a checked property ("Does this path match a rule in .gitignore?"),
> rather than a mode passed in from the caller.  Change the variable name
> to match the part of speech used by the function called to define it,
> which will hopefully make these bits of code slightly clearer to the
> next reader.

I agree that some of the terminology in the .gitignore is confusing,
especially when the terminology was used in the opposite sense for
the sparse-checkout feature. I think this rename is worth the noise.

For reference, here are some commits from ds/include-exclude that
performed similar refactors:

468ce99b77 unpack-trees: rename 'is_excluded_from_list()'
65edd96aec treewide: rename 'exclude' methods to 'pattern'
4ff89ee52c treewide: rename 'EXCL_FLAG_' to 'PATTERN_FLAG_'
caa3d55444 treewide: rename 'struct exclude_list' to 'struct pattern_list'
ab8db61390 treewide: rename 'struct exclude' to 'struct path_pattern'

Thanks,
-Stolee

SZEDER Gábor Jan. 31, 2020, 6:04 p.m. UTC | #2

On Wed, Jan 29, 2020 at 10:03:40PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> Despite having contributed several fixes in this area, I have for months
> (years?) assumed that the "exclude" variable was a directive; this
> caused me to think of it as a different mode we operate in and left me
> confused as I tried to build up a mental model around why we'd need such
> a directive.  I mostly tried to ignore it while focusing on the pieces I
> was trying to understand.
> 
> Then I finally traced this variable all back to a call to is_excluded(),
> meaning it was actually functioning as an adjective.  In particular, it
> was a checked property ("Does this path match a rule in .gitignore?"),
> rather than a mode passed in from the caller.  Change the variable name
> to match the part of speech used by the function called to define it,
> which will hopefully make these bits of code slightly clearer to the
> next reader.

Slightly related questions: Does 'excluded' always mean ignored?  Or
is it possible for a file to be excluded but for some other reason
than being ignored?

I'm never really sure, and of course it doesn't help that we have both
'.gitignore' and '.git/info/exclude' files and conditions like:

> +		if (excluded &&
> +		    (dir->flags & DIR_SHOW_IGNORED_TOO) &&
> +		    (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING)) {

Elijah Newren Jan. 31, 2020, 6:17 p.m. UTC | #3

On Fri, Jan 31, 2020 at 10:04 AM SZEDER Gábor <[e-mail]> wrote:
>
> On Wed, Jan 29, 2020 at 10:03:40PM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[e-mail]>
> >
> > Despite having contributed several fixes in this area, I have for months
> > (years?) assumed that the "exclude" variable was a directive; this
> > caused me to think of it as a different mode we operate in and left me
> > confused as I tried to build up a mental model around why we'd need such
> > a directive.  I mostly tried to ignore it while focusing on the pieces I
> > was trying to understand.
> >
> > Then I finally traced this variable all back to a call to is_excluded(),
> > meaning it was actually functioning as an adjective.  In particular, it
> > was a checked property ("Does this path match a rule in .gitignore?"),
> > rather than a mode passed in from the caller.  Change the variable name
> > to match the part of speech used by the function called to define it,
> > which will hopefully make these bits of code slightly clearer to the
> > next reader.
>
> Slightly related questions: Does 'excluded' always mean ignored?  Or
> is it possible for a file to be excluded but for some other reason
> than being ignored?
>
> I'm never really sure, and of course it doesn't help that we have both
> '.gitignore' and '.git/info/exclude' files and conditions like:
>
> > +             if (excluded &&
> > +                 (dir->flags & DIR_SHOW_IGNORED_TOO) &&
> > +                 (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING)) {
>

Good question; no idea.  You can start digging into is_excluded() and
the pattern list stored in the dir struct and try to trace it back to
see if it's just the combination of ignore rules in .gitignore and
.git/info/exclude and core.excludesFile, or if there is something else
meant here.

Patch

diff --git a/dir.c b/dir.c
index c358158f55..225f0bc082 100644
--- a/dir.c
+++ b/dir.c
@@ -1656,7 +1656,7 @@  static enum exist_status directory_exists_in_index(struct index_state *istate,
 static enum path_treatment treat_directory(struct dir_struct *dir,
 	struct index_state *istate,
 	struct untracked_cache_dir *untracked,
-	const char *dirname, int len, int baselen, int exclude,
+	const char *dirname, int len, int baselen, int excluded,
 	const struct pathspec *pathspec)
 {
 	int nested_repo = 0;
@@ -1679,13 +1679,13 @@  static enum path_treatment treat_directory(struct dir_struct *dir,
 		}
 		if (nested_repo)
 			return ((dir->flags & DIR_SKIP_NESTED_GIT) ? path_none :
-				(exclude ? path_excluded : path_untracked));
+				(excluded ? path_excluded : path_untracked));
 
 		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
 			break;
-		if (exclude &&
-			(dir->flags & DIR_SHOW_IGNORED_TOO) &&
-			(dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING)) {
+		if (excluded &&
+		    (dir->flags & DIR_SHOW_IGNORED_TOO) &&
+		    (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING)) {
 
 			/*
 			 * This is an excluded directory and we are
@@ -1713,7 +1713,7 @@  static enum path_treatment treat_directory(struct dir_struct *dir,
 	/* This is the "show_other_directories" case */
 
 	if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
-		return exclude ? path_excluded : path_untracked;
+		return excluded ? path_excluded : path_untracked;
 
 	untracked = lookup_untracked(dir->untracked, untracked,
 				     dirname + baselen, len - baselen);
@@ -1723,7 +1723,7 @@  static enum path_treatment treat_directory(struct dir_struct *dir,
 	 * the directory contains any files.
 	 */
 	return read_directory_recursive(dir, istate, dirname, len,
-					untracked, 1, exclude, pathspec);
+					untracked, 1, excluded, pathspec);
 }
 
 /*
@@ -1904,7 +1904,7 @@  static enum path_treatment treat_path(struct dir_struct *dir,
 				      int baselen,
 				      const struct pathspec *pathspec)
 {
-	int has_path_in_index, dtype, exclude;
+	int has_path_in_index, dtype, excluded;
 	enum path_treatment path_treatment;
 
 	if (!cdir->d_name)
@@ -1949,13 +1949,13 @@  static enum path_treatment treat_path(struct dir_struct *dir,
 	    (directory_exists_in_index(istate, path->buf, path->len) == index_nonexistent))
 		return path_none;
 
-	exclude = is_excluded(dir, istate, path->buf, &dtype);
+	excluded = is_excluded(dir, istate, path->buf, &dtype);
 
 	/*
 	 * Excluded? If we don't explicitly want to show
 	 * ignored files, ignore it
 	 */
-	if (exclude && !(dir->flags & (DIR_SHOW_IGNORED|DIR_SHOW_IGNORED_TOO)))
+	if (excluded && !(dir->flags & (DIR_SHOW_IGNORED|DIR_SHOW_IGNORED_TOO)))
 		return path_excluded;
 
 	switch (dtype) {
@@ -1965,7 +1965,7 @@  static enum path_treatment treat_path(struct dir_struct *dir,
 		strbuf_addch(path, '/');
 		path_treatment = treat_directory(dir, istate, untracked,
 						 path->buf, path->len,
-						 baselen, exclude, pathspec);
+						 baselen, excluded, pathspec);
 		/*
 		 * If 1) we only want to return directories that
 		 * match an exclude pattern and 2) this directory does
@@ -1974,7 +1974,7 @@  static enum path_treatment treat_path(struct dir_struct *dir,
 		 * recurse into this directory (instead of marking the
 		 * directory itself as an ignored path).
 		 */
-		if (!exclude &&
+		if (!excluded &&
 		    path_treatment == path_excluded &&
 		    (dir->flags & DIR_SHOW_IGNORED_TOO) &&
 		    (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING))
@@ -1982,7 +1982,7 @@  static enum path_treatment treat_path(struct dir_struct *dir,
 		return path_treatment;
 	case DT_REG:
 	case DT_LNK:
-		return exclude ? path_excluded : path_untracked;
+		return excluded ? path_excluded : path_untracked;
 	}
 }