[PATCH] dir.c: fix dir re-inclusion rules with "NODIR" and "MUSTBEDIR"

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date: Mar 9, 2016, 11:08 UTC
Message-ID: <1457521693-26141-1-git-send-email-pclouds@gmail.com>
In-Reply-To: <xmqqy49siin2.fsf@gitster.mtv.corp.google.com>

For NODIR case, the patterns look like this

    *          # exclude dir, dir/file1 and dir/file2..
    !dir       # ..except that dir and everything inside is re-included..
    dir/file2  # ..except (again!) that dir/file2 is excluded
               # ..which means dir/file1 stays included

When we stay at topdir and test "dir", everything is hunky dory, current code returns "re-include dir" as expected. When we stay in "dir" and examine "dir/file1", however, match_basename() checks if the base name component, which is "file1", matches "dir" from the second rule.

This is wrong. We contradict ourselves because earlier we decided to re-include dir/file1 (from the second rule) when we were at toplevel. Because the second rule is ignored, we hit the first one and return "excluded" even though "dir/file1" should be re-included.

Side note, it's probably a bad idea to use basename matching on a negative rule (ie. no slashes in the pattern) because what the pattern says is "re-include _any_ directory named 'dir'", not just the directory "dir" at right below this directory.

In the MUSTBEDIR case, the patterns look like this

    *          # exclude dir, dir/file1 and dir/file2..
    !dir/      #  ..except that dir and everything inside is re-included..
    dir/file2  # ..except (again!) that dir/file2 is excluded
               # ..which means dir/file1 stays included

Again, we're ok at the toplevel, then we enter "dir" and test "dir/file1". The MUSTBEDIR code tests if the _full_ path (ie. "dir/file1") is a directory. We want it to test the "dir" part from "dir/file1" instead.

In both cases, we want to test if the pattern matches a parent directory. match_dir_component() is for this purpose.

We do want to be careful not to get back too far. Given the file foo/bar/.gitignore, we should only check as far back as foo/bar because this .gitignore file can never match outside that directory, which is "toplevel" in the above paragraphs, to begin with.

The remaining matching case (neither NODIR nor MUSTBEDIR is set) is already fixed in a60ea8f (dir.c: fix match_pathname() - 2016-02-15)

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 The fix may look like this (I didn't think about the "**" trick in
 wildmatch, which makes things simpler).

 No it's not meant for 2.8.0. I didn't even optimize for the
 no-wildcard case, or add tests. But we can still stare at it in the
 meantime to see if I analyzed anything wrong. I almost thought I was
 wrong to the declare the bug while I was on my ride back home..

 dir.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 69e0be6..5123483 100644
--- a/dir.c
+++ b/dir.c
@@ -992,6 +992,51 @@ static struct exclude *should_descend(const char *pathname, int pathlen,
 }
 
 /*
+ * Given a "NODIR" pattern, check if it matches any directory
+ * component after the x->base part.
+ *
+ * If the pattern is not NODIR (ie. pathname matching) _and_ is
+ * MUSTBE, check if it matches a directory as well.
+ *
+ * Note that "path" and "len" must cover the basename part as well,
+ * looking from last_exclude_matching_from_list(), not just the dirname.
+ */
+static int match_dir_component(const char *path, int len, struct exclude *x)
+{
+	struct strbuf new_pattern = STRBUF_INIT;
+	int ret;
+
+	if (x->flags & EXC_FLAG_NODIR) {
+		if (!x->baselen) {
+			strbuf_addf(&new_pattern, "%s/**", x->pattern);
+			ret = !fnmatch_icase_mem(new_pattern.buf, new_pattern.len,
+						 path, strlen(path),
+						 WM_PATHNAME);
+			strbuf_reset(&new_pattern);
+			if (ret)
+				return ret;
+		}
+
+		strbuf_addf(&new_pattern, "%.*s**/%s/**", x->baselen, x->base, x->pattern);
+		ret = !fnmatch_icase_mem(new_pattern.buf, new_pattern.len,
+					 path, strlen(path),
+					 WM_PATHNAME);
+		strbuf_reset(&new_pattern);
+		return ret;
+	}
+
+	assert(x->flags & EXC_FLAG_MUSTBEDIR);
+	strbuf_addf(&new_pattern, "%.*s%s/**",
+		    x->baselen, x->base,
+		    *x->pattern == '/' ? x->pattern+1 : x->pattern);
+	ret = !fnmatch_icase_mem(new_pattern.buf, new_pattern.len,
+				 path, strlen(path),
+				 WM_PATHNAME);
+	strbuf_release(&new_pattern);
+	return ret;
+}
+
+/*
  * Scan the given exclude list in reverse to see whether pathname
  * should be ignored.  The first match (i.e. the last on the list), if
  * any, determines the fate.  Returns the exclude_list element which
@@ -1033,7 +1078,8 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
 		if (x->flags & EXC_FLAG_MUSTBEDIR) {
 			if (*dtype == DT_UNKNOWN)
 				*dtype = get_dtype(NULL, pathname, pathlen);
-			if (*dtype != DT_DIR)
+			if (*dtype != DT_DIR &&
+			    !match_dir_component(pathname, strlen(pathname), x))
 				continue;
 		}
 
@@ -1041,7 +1087,8 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
 			if (match_basename(basename,
 					   pathlen - (basename - pathname),
 					   exclude, prefix, x->patternlen,
-					   x->flags)) {
+					   x->flags) ||
+			    match_dir_component(pathname, strlen(pathname), x)) {
 				exc = x;
 				break;
 			}

-- 
2.8.0.rc0.208.g69d9f93