Merge pull request #4296 from pks-t/pks/pattern-based-gitignore

Fix negative ignore rules with patterns

src/ignore.c

@@ -48,38 +48,42 @@
  */
 static int does_negate_pattern(git_attr_fnmatch *rule, git_attr_fnmatch *neg)
 {
+	int (*cmp)(const char *, const char *, size_t);
 	git_attr_fnmatch *longer, *shorter;
 	char *p;
 
-	if ((rule->flags & GIT_ATTR_FNMATCH_NEGATIVE) == 0
-		&& (neg->flags & GIT_ATTR_FNMATCH_NEGATIVE) != 0) {
-
-		/* If lengths match we need to have an exact match */
-		if (rule->length == neg->length) {
-			return strcmp(rule->pattern, neg->pattern) == 0;
-		} else if (rule->length < neg->length) {
-			shorter = rule;
-			longer = neg;
-		} else {
-			shorter = neg;
-			longer = rule;
-		}
+	if ((rule->flags & GIT_ATTR_FNMATCH_NEGATIVE) != 0
+	    || (neg->flags & GIT_ATTR_FNMATCH_NEGATIVE) == 0)
+		return false;
+
+	if (neg->flags & GIT_ATTR_FNMATCH_ICASE)
+		cmp = git__strncasecmp;
+	else
+		cmp = strncmp;
+
+	/* If lengths match we need to have an exact match */
+	if (rule->length == neg->length) {
+		return cmp(rule->pattern, neg->pattern, rule->length) == 0;
+	} else if (rule->length < neg->length) {
+		shorter = rule;
+		longer = neg;
+	} else {
+		shorter = neg;
+		longer = rule;
+	}
 
-		/* Otherwise, we need to check if the shorter
-		 * rule is a basename only (that is, it contains
-		 * no path separator) and, if so, if it
-		 * matches the tail of the longer rule */
-		p = longer->pattern + longer->length - shorter->length;
+	/* Otherwise, we need to check if the shorter
+	 * rule is a basename only (that is, it contains
+	 * no path separator) and, if so, if it
+	 * matches the tail of the longer rule */
+	p = longer->pattern + longer->length - shorter->length;
 
-		if (p[-1] != '/')
-			return false;
-		if (memchr(shorter->pattern, '/', shorter->length) != NULL)
-			return false;
+	if (p[-1] != '/')
+		return false;
+	if (memchr(shorter->pattern, '/', shorter->length) != NULL)
+		return false;
 
-		return memcmp(p, shorter->pattern, shorter->length) == 0;
-	}
-
-	return false;
+	return cmp(p, shorter->pattern, shorter->length) == 0;
 }
 
 /**
@@ -97,7 +101,7 @@ static int does_negate_pattern(git_attr_fnmatch *rule, git_attr_fnmatch *neg)
  */
 static int does_negate_rule(int *out, git_vector *rules, git_attr_fnmatch *match)
 {
-	int error = 0;
+	int error = 0, fnflags;
 	size_t i;
 	git_attr_fnmatch *rule;
 	char *path;
@@ -105,6 +109,10 @@ static int does_negate_rule(int *out, git_vector *rules, git_attr_fnmatch *match
 
 	*out = 0;
 
+	fnflags = FNM_PATHNAME;
+	if (match->flags & GIT_ATTR_FNMATCH_ICASE)
+		fnflags |= FNM_IGNORECASE;
+
 	/* path of the file relative to the workdir, so we match the rules in subdirs */
 	if (match->containing_dir) {
 		git_buf_puts(&buf, match->containing_dir);
@@ -125,12 +133,12 @@ static int does_negate_rule(int *out, git_vector *rules, git_attr_fnmatch *match
 				continue;
 		}
 
-	/*
-	 * When dealing with a directory, we add '/<star>' so
-	 * p_fnmatch() honours FNM_PATHNAME. Checking for LEADINGDIR
-	 * alone isn't enough as that's also set for nagations, so we
-	 * need to check that NEGATIVE is off.
-	 */
+		/*
+		 * When dealing with a directory, we add '/<star>' so
+		 * p_fnmatch() honours FNM_PATHNAME. Checking for LEADINGDIR
+		 * alone isn't enough as that's also set for nagations, so we
+		 * need to check that NEGATIVE is off.
+		 */
 		git_buf_clear(&buf);
 		if (rule->containing_dir) {
 			git_buf_puts(&buf, rule->containing_dir);
@@ -144,7 +152,7 @@ static int does_negate_rule(int *out, git_vector *rules, git_attr_fnmatch *match
 		if (error < 0)
 			goto out;
 
-		if ((error = p_fnmatch(git_buf_cstr(&buf), path, FNM_PATHNAME)) < 0) {
+		if ((error = p_fnmatch(git_buf_cstr(&buf), path, fnflags)) < 0) {
 			giterr_set(GITERR_INVALID, "error matching pattern");
 			goto out;
 		}
@@ -207,8 +215,14 @@ static int parse_ignore_file(
 
 			scan = git__next_line(scan);
 
-			/* if a negative match doesn't actually do anything, throw it away */
-			if (match->flags & GIT_ATTR_FNMATCH_NEGATIVE)
+			/*
+			 * If a negative match doesn't actually do anything,
+			 * throw it away. As we cannot always verify whether a
+			 * rule containing wildcards negates another rule, we
+			 * do not optimize away these rules, though.
+			 * */
+			if (match->flags & GIT_ATTR_FNMATCH_NEGATIVE
+			    && !(match->flags & GIT_ATTR_FNMATCH_HASWILD))
 				error = does_negate_rule(&valid_rule, &attrs->rules, match);
 
 			if (!error && valid_rule)

tests/attr/ignore.c

@@ -303,3 +303,46 @@ void test_attr_ignore__test(void)
 	assert_is_ignored(true, "dist/foo.o");
 	assert_is_ignored(true, "bin/foo");
 }
+
+void test_attr_ignore__unignore_dir_succeeds(void)
+{
+	cl_git_rewritefile("attr/.gitignore",
+		"*.c\n"
+		"!src/*.c\n");
+	assert_is_ignored(false, "src/foo.c");
+	assert_is_ignored(true, "src/foo/foo.c");
+}
+
+void test_attr_ignore__case_insensitive_unignores_previous_rule(void)
+{
+	git_config *cfg;
+
+	cl_git_rewritefile("attr/.gitignore",
+		"/case\n"
+		"!/Case/\n");
+
+	cl_git_pass(git_repository_config(&cfg, g_repo));
+	cl_git_pass(git_config_set_bool(cfg, "core.ignorecase", true));
+
+	cl_must_pass(p_mkdir("attr/case", 0755));
+	cl_git_mkfile("attr/case/file", "content");
+
+	assert_is_ignored(false, "case/file");
+}
+
+void test_attr_ignore__case_sensitive_unignore_does_nothing(void)
+{
+	git_config *cfg;
+
+	cl_git_rewritefile("attr/.gitignore",
+		"/case\n"
+		"!/Case/\n");
+
+	cl_git_pass(git_repository_config(&cfg, g_repo));
+	cl_git_pass(git_config_set_bool(cfg, "core.ignorecase", false));
+
+	cl_must_pass(p_mkdir("attr/case", 0755));
+	cl_git_mkfile("attr/case/file", "content");
+
+	assert_is_ignored(true, "case/file");
+}

tests/status/ignore.c

@@ -1155,3 +1155,30 @@ void test_status_ignore__subdir_ignore_everything_except_certain_files(void)
 	refute_is_ignored("project/src/foo.c");
 	refute_is_ignored("project/src/foo/foo.c");
 }
+
+void test_status_ignore__deeper(void)
+{
+   int ignored;
+
+    g_repo = cl_git_sandbox_init("empty_standard_repo");
+
+    cl_git_mkfile("empty_standard_repo/.gitignore",
+          "*.data\n"
+          "!dont_ignore/*.data\n");
+
+    cl_git_pass(p_mkdir("empty_standard_repo/dont_ignore", 0777));
+    cl_git_mkfile("empty_standard_repo/foo.data", "");
+    cl_git_mkfile("empty_standard_repo/bar.data", "");
+    cl_git_mkfile("empty_standard_repo/dont_ignore/foo.data", "");
+    cl_git_mkfile("empty_standard_repo/dont_ignore/bar.data", "");
+
+    cl_git_pass(git_ignore_path_is_ignored(&ignored, g_repo, "foo.data"));
+    cl_assert_equal_i(1, ignored);
+    cl_git_pass(git_ignore_path_is_ignored(&ignored, g_repo, "bar.data"));
+    cl_assert_equal_i(1, ignored);
+
+    cl_git_pass(git_ignore_path_is_ignored(&ignored, g_repo, "dont_ignore/foo.data"));
+    cl_assert_equal_i(0, ignored);
+    cl_git_pass(git_ignore_path_is_ignored(&ignored, g_repo, "dont_ignore/bar.data"));
+    cl_assert_equal_i(0, ignored);
+}