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

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date: Mar 17, 2016, 12:54 UTC
Message-ID: <1458219254-16343-1-git-send-email-pclouds@gmail.com>
In-Reply-To: <1458218744-15810-2-git-send-email-pclouds@gmail.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.

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, the second decision on "dir/file1" is wrong and contradicts with the first one on "dir". This is a perfect use case for "sticky path list" added earlier to solve a different (but quite similar) problem. So, when we have the right decision the first time, we mark the path sticky to override the coming wrong decision.

The reason to do this, instead of actually fixing the code to make the right second decision in the first place, is because it's soooooo complicated. There are many combinations to take care of, many optimizations involved to keep the cost on normal/common case (and what's being described here is NOT normal) down to minimum. On top of that, exclude code is already complicated as it is. It's best to not turn the code upside down. Not until this approach is proved insufficient.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 This is NOT for 2.8.0! Posted here to give you some context on the
 problem mentioned in the first patch.

 If you actually read the link in 1/2, you'll notice this patch is
 completely different. "soooooo complicated" is not an exaggeration. I
 found some problem with that old patch, which ended up with a
 combination explosion of cases I would have to cover separately,
 essentially the same thing I encountered in my first try before that
 patch. I finally admitted I could not fit all that in my brain
 anymore.

 dir.c                              |  5 ++++
 t/t3007-ls-files-other-negative.sh | 51 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/dir.c b/dir.c
index 2028094..a704e8a 100644
--- a/dir.c
+++ b/dir.c
@@ -1090,6 +1090,11 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
 					 x->pattern, x->srcpos);
 			return NULL;
 		}
+	} else if (exc->flags & EXC_FLAG_NEGATIVE) {
+		if (*dtype == DT_UNKNOWN)
+			*dtype = get_dtype(NULL, pathname, pathlen);
+		if (*dtype == DT_DIR)
+			add_sticky(exc, pathname, pathlen);
 	}
 
 	trace_printf_key(&trace_exclude, "exclude: %.*s vs %s at line %d => %s%s\n",
diff --git a/t/t3007-ls-files-other-negative.sh b/t/t3007-ls-files-other-negative.sh
index 0797b86..c8f39dd 100755
--- a/t/t3007-ls-files-other-negative.sh
+++ b/t/t3007-ls-files-other-negative.sh
@@ -150,4 +150,55 @@ test_expect_success 'match, literal pathname, nested negatives' '
 	test_cmp tmp/expected tmp/actual
 '
 
+test_expect_success 're-include case, NODIR' '
+	git init re-include-nodir &&
+	(
+		cd re-include-nodir &&
+		mkdir dir &&
+		touch dir/file1 dir/file2 &&
+		cat >.gitignore <<-\EOF &&
+		*
+		!dir
+		dir/file2
+		EOF
+		git ls-files -o --exclude-standard >../actual &&
+		echo dir/file1 >../expected &&
+		test_cmp ../expected ../actual
+	)
+'
+
+test_expect_success 're-include case, MUSTBEDIR with NODIR' '
+	git init re-include-mustbedir &&
+	(
+		cd re-include-mustbedir &&
+		mkdir dir &&
+		touch dir/file1 dir/file2 &&
+		cat >.gitignore <<-\EOF &&
+		*
+		!dir/
+		dir/file2
+		EOF
+		git ls-files -o --exclude-standard >../actual &&
+		echo dir/file1 >../expected &&
+		test_cmp ../expected ../actual
+	)
+'
+
+test_expect_success 're-include case, MUSTBEDIR without NODIR' '
+	git init re-include-pathname &&
+	(
+		cd re-include-pathname &&
+		mkdir -p dir1/dir2 &&
+		touch dir1/dir2/file1 dir1/dir2/file2 &&
+		cat >.gitignore <<-\EOF &&
+		*
+		!dir1/dir2/
+		dir1/dir2/file2
+		EOF
+		git ls-files -o --exclude-standard >../actual &&
+		echo dir1/dir2/file1 >../expected &&
+		test_cmp ../expected ../actual
+	)
+'
+
 test_done

-- 
2.8.0.rc0.210.gd302cd2