Merge pull request #2328 from libgit2/rb/how-broken-can-ignores-be

Improve checks for ignore containment

src/attr_file.c

@@ -281,7 +281,7 @@ uint32_t git_attr_file__name_hash(const char *name)
 
 int git_attr_file__lookup_one(
 	git_attr_file *file,
-	const git_attr_path *path,
+	git_attr_path *path,
 	const char *attr,
 	const char **value)
 {
@@ -342,14 +342,11 @@ int git_attr_file__load_standalone(git_attr_file **out, const char *path)
 
 bool git_attr_fnmatch__match(
 	git_attr_fnmatch *match,
-	const git_attr_path *path)
+	git_attr_path *path)
 {
 	const char *filename;
 	int flags = 0;
 
-	if ((match->flags & GIT_ATTR_FNMATCH_DIRECTORY) && !path->is_dir)
-		return false;
-
 	if (match->flags & GIT_ATTR_FNMATCH_ICASE)
 		flags |= FNM_CASEFOLD;
 	if (match->flags & GIT_ATTR_FNMATCH_LEADINGDIR)
@@ -365,12 +362,28 @@ bool git_attr_fnmatch__match(
 			flags |= FNM_LEADING_DIR;
 	}
 
+	if ((match->flags & GIT_ATTR_FNMATCH_DIRECTORY) && !path->is_dir) {
+		int matchval;
+
+		/* for attribute checks or root ignore checks, fail match */
+		if (!(match->flags & GIT_ATTR_FNMATCH_IGNORE) ||
+			path->basename == path->path)
+			return false;
+
+		/* for ignore checks, use container of current item for check */
+		path->basename[-1] = '\0';
+		flags |= FNM_LEADING_DIR;
+		matchval = p_fnmatch(match->pattern, path->path, flags);
+		path->basename[-1] = '/';
+		return (matchval != FNM_NOMATCH);
+	}
+
 	return (p_fnmatch(match->pattern, filename, flags) != FNM_NOMATCH);
 }
 
 bool git_attr_rule__match(
 	git_attr_rule *rule,
-	const git_attr_path *path)
+	git_attr_path *path)
 {
 	bool matched = git_attr_fnmatch__match(&rule->match, path);
 

src/attr_file.h

@@ -138,7 +138,7 @@ int git_attr_file__clear_rules(
 
 int git_attr_file__lookup_one(
 	git_attr_file *file,
-	const git_attr_path *path,
+	git_attr_path *path,
 	const char *attr,
 	const char **value);
 
@@ -162,13 +162,13 @@ extern int git_attr_fnmatch__parse(
 
 extern bool git_attr_fnmatch__match(
 	git_attr_fnmatch *rule,
-	const git_attr_path *path);
+	git_attr_path *path);
 
 extern void git_attr_rule__free(git_attr_rule *rule);
 
 extern bool git_attr_rule__match(
 	git_attr_rule *rule,
-	const git_attr_path *path);
+	git_attr_path *path);
 
 extern git_attr_assignment *git_attr_rule__lookup_assignment(
 	git_attr_rule *rule, const char *name);

src/diff.c

@@ -633,7 +633,6 @@ typedef struct {
 	git_iterator *new_iter;
 	const git_index_entry *oitem;
 	const git_index_entry *nitem;
-	git_buf ignore_prefix;
 } diff_in_progress;
 
 #define MODE_BITS_MASK 0000777
@@ -844,24 +843,13 @@ static int handle_unmatched_new_item(
 	/* check if this is a prefix of the other side */
 	contains_oitem = entry_is_prefixed(diff, info->oitem, nitem);
 
-	/* check if this is contained in an ignored parent directory */
-	if (git_buf_len(&info->ignore_prefix)) {
-		if (diff->pfxcomp(nitem->path, git_buf_cstr(&info->ignore_prefix)) == 0)
+	/* update delta_type if this item is ignored */
+	if (git_iterator_current_is_ignored(info->new_iter))
 		delta_type = GIT_DELTA_IGNORED;
-		else
-			git_buf_clear(&info->ignore_prefix);
-	}
 
 	if (nitem->mode == GIT_FILEMODE_TREE) {
 		bool recurse_into_dir = contains_oitem;
 
-		/* if not already inside an ignored dir, check if this is ignored */
-		if (delta_type != GIT_DELTA_IGNORED &&
-			git_iterator_current_is_ignored(info->new_iter)) {
-			delta_type = GIT_DELTA_IGNORED;
-			git_buf_sets(&info->ignore_prefix, nitem->path);
-		}
-
 		/* check if user requests recursion into this type of dir */
 		recurse_into_dir = contains_oitem ||
 			(delta_type == GIT_DELTA_UNTRACKED &&
@@ -938,27 +926,12 @@ static int handle_unmatched_new_item(
 		}
 	}
 
-	/* In core git, the next two checks are effectively reversed --
-	 * i.e. when an file contained in an ignored directory is explicitly
-	 * ignored, it shows up as an ignored file in the diff list, even though
-	 * other untracked files in the same directory are skipped completely.
-	 *
-	 * To me, this seems odd.  If the directory is ignored and the file is
-	 * untracked, we should skip it consistently, regardless of whether it
-	 * happens to match a pattern in the ignore file.
-	 *
-	 * To match the core git behavior, reverse the following two if checks
-	 * so that individual file ignores are checked before container
-	 * directory exclusions are used to skip the file.
-	 */
 	else if (delta_type == GIT_DELTA_IGNORED &&
-		DIFF_FLAG_ISNT_SET(diff, GIT_DIFF_RECURSE_IGNORED_DIRS))
+		DIFF_FLAG_ISNT_SET(diff, GIT_DIFF_RECURSE_IGNORED_DIRS) &&
+		git_iterator_current_tree_is_ignored(info->new_iter))
 		/* item contained in ignored directory, so skip over it */
 		return git_iterator_advance(&info->nitem, info->new_iter);
 
-	else if (git_iterator_current_is_ignored(info->new_iter))
-		delta_type = GIT_DELTA_IGNORED;
-
 	else if (info->new_iter->type != GIT_ITERATOR_TYPE_WORKDIR)
 		delta_type = GIT_DELTA_ADDED;
 
@@ -1068,7 +1041,6 @@ int git_diff__from_iterators(
 	info.repo = repo;
 	info.old_iter = old_iter;
 	info.new_iter = new_iter;
-	git_buf_init(&info.ignore_prefix, 0);
 
 	/* make iterators have matching icase behavior */
 	if (DIFF_FLAG_IS_SET(diff, GIT_DIFF_IGNORE_CASE)) {
@@ -1123,8 +1095,6 @@ cleanup:
 	else
 		git_diff_free(diff);
 
-	git_buf_free(&info.ignore_prefix);
-
 	return error;
 }
 

src/ignore.c

@@ -248,14 +248,15 @@ void git_ignore__free(git_ignores *ignores)
 }
 
 static bool ignore_lookup_in_rules(
-	git_attr_file *file, git_attr_path *path, int *ignored)
+	int *ignored, git_attr_file *file, git_attr_path *path)
 {
 	size_t j;
 	git_attr_fnmatch *match;
 
 	git_vector_rforeach(&file->rules, j, match) {
 		if (git_attr_fnmatch__match(match, path)) {
-			*ignored = ((match->flags & GIT_ATTR_FNMATCH_NEGATIVE) == 0);
+			*ignored = ((match->flags & GIT_ATTR_FNMATCH_NEGATIVE) == 0) ?
+				GIT_IGNORE_TRUE : GIT_IGNORE_FALSE;
 			return true;
 		}
 	}
@@ -264,34 +265,34 @@ static bool ignore_lookup_in_rules(
 }
 
 int git_ignore__lookup(
-	git_ignores *ignores, const char *pathname, int *ignored)
+	int *out, git_ignores *ignores, const char *pathname)
 {
 	unsigned int i;
 	git_attr_file *file;
 	git_attr_path path;
 
+	*out = GIT_IGNORE_NOTFOUND;
+
 	if (git_attr_path__init(
 		&path, pathname, git_repository_workdir(ignores->repo)) < 0)
 		return -1;
 
 	/* first process builtins - success means path was found */
-	if (ignore_lookup_in_rules(ignores->ign_internal, &path, ignored))
+	if (ignore_lookup_in_rules(out, ignores->ign_internal, &path))
 		goto cleanup;
 
 	/* next process files in the path */
 	git_vector_foreach(&ignores->ign_path, i, file) {
-		if (ignore_lookup_in_rules(file, &path, ignored))
+		if (ignore_lookup_in_rules(out, file, &path))
 			goto cleanup;
 	}
 
 	/* last process global ignores */
 	git_vector_foreach(&ignores->ign_global, i, file) {
-		if (ignore_lookup_in_rules(file, &path, ignored))
+		if (ignore_lookup_in_rules(out, file, &path))
 			goto cleanup;
 	}
 
-	*ignored = 0;
-
 cleanup:
 	git_attr_path__free(&path);
 	return 0;
@@ -335,8 +336,6 @@ int git_ignore_path_is_ignored(
 	int error;
 	const char *workdir;
 	git_attr_path path;
-	char *tail, *end;
-	bool full_is_dir;
 	git_ignores ignores;
 	unsigned int i;
 	git_attr_file *file;
@@ -345,55 +344,42 @@ int git_ignore_path_is_ignored(
 
 	workdir = repo ? git_repository_workdir(repo) : NULL;
 
-	if ((error = git_attr_path__init(&path, pathname, workdir)) < 0)
-		return error;
-
-	tail = path.path;
-	end  = &path.full.ptr[path.full.size];
-	full_is_dir = path.is_dir;
-
-	while (1) {
-		/* advance to next component of path */
-		path.basename = tail;
-
-		while (tail < end && *tail != '/') tail++;
-		*tail = '\0';
+	memset(&path, 0, sizeof(path));
+	memset(&ignores, 0, sizeof(ignores));
 
-		path.full.size = (tail - path.full.ptr);
-		path.is_dir = (tail == end) ? full_is_dir : true;
-
-		/* initialize ignores the first time through */
-		if (path.basename == path.path &&
+	if ((error = git_attr_path__init(&path, pathname, workdir)) < 0 ||
 		(error = git_ignore__for_path(repo, path.path, &ignores)) < 0)
-			break;
+		goto cleanup;
 
+	while (1) {
 		/* first process builtins - success means path was found */
-		if (ignore_lookup_in_rules(ignores.ign_internal, &path, ignored))
+		if (ignore_lookup_in_rules(ignored, ignores.ign_internal, &path))
 			goto cleanup;
 
 		/* next process files in the path */
 		git_vector_foreach(&ignores.ign_path, i, file) {
-			if (ignore_lookup_in_rules(file, &path, ignored))
+			if (ignore_lookup_in_rules(ignored, file, &path))
 				goto cleanup;
 		}
 
 		/* last process global ignores */
 		git_vector_foreach(&ignores.ign_global, i, file) {
-			if (ignore_lookup_in_rules(file, &path, ignored))
+			if (ignore_lookup_in_rules(ignored, file, &path))
 				goto cleanup;
 		}
 
-		/* if we found no rules before reaching the end, we're done */
-		if (tail == end)
+		/* move up one directory */
+		if (path.basename == path.path)
 			break;
-
-		/* now add this directory to list of ignores */
-		if ((error = git_ignore__push_dir(&ignores, path.path)) < 0)
+		path.basename[-1] = '\0';
+		while (path.basename > path.path && *path.basename != '/')
+			path.basename--;
+		if (path.basename > path.path)
+			path.basename++;
+		path.is_dir = 1;
+
+		if ((error = git_ignore__pop_dir(&ignores)) < 0)
 			break;
-
-		/* reinstate divider in path */
-		*tail = '/';
-		while (*tail == '/') tail++;
 	}
 
 	*ignored = 0;
@@ -404,7 +390,6 @@ cleanup:
 	return error;
 }
 
-
 int git_ignore__check_pathspec_for_exact_ignores(
 	git_repository *repo,
 	git_vector *vspec,

src/ignore.h

@@ -42,7 +42,14 @@ extern int git_ignore__pop_dir(git_ignores *ign);
 
 extern void git_ignore__free(git_ignores *ign);
 
-extern int git_ignore__lookup(git_ignores *ign, const char *path, int *ignored);
+enum {
+	GIT_IGNORE_UNCHECKED = -2,
+	GIT_IGNORE_NOTFOUND = -1,
+	GIT_IGNORE_FALSE = 0,
+	GIT_IGNORE_TRUE = 1,
+};
+
+extern int git_ignore__lookup(int *out, git_ignores *ign, const char *path);
 
 /* command line Git sometimes generates an error message if given a
  * pathspec that contains an exact match to an ignored file (provided

src/iterator.c

@@ -897,6 +897,7 @@ struct fs_iterator_frame {
 	fs_iterator_frame *next;
 	git_vector entries;
 	size_t index;
+	int is_ignored;
 };
 
 typedef struct fs_iterator fs_iterator;
@@ -1290,16 +1291,28 @@ GIT_INLINE(bool) workdir_path_is_dotgit(const git_buf *path)
 
 static int workdir_iterator__enter_dir(fs_iterator *fi)
 {
+	workdir_iterator *wi = (workdir_iterator *)fi;
 	fs_iterator_frame *ff = fi->stack;
 	size_t pos;
 	git_path_with_stat *entry;
 	bool found_submodules = false;
 
-	/* only push new ignores if this is not top level directory */
+	/* check if this directory is ignored */
+	if (git_ignore__lookup(
+			&ff->is_ignored, &wi->ignores, fi->path.ptr + fi->root_len) < 0) {
+		giterr_clear();
+		ff->is_ignored = GIT_IGNORE_NOTFOUND;
+	}
+
+	/* if this is not the top level directory... */
 	if (ff->next != NULL) {
-		workdir_iterator *wi = (workdir_iterator *)fi;
 		ssize_t slash_pos = git_buf_rfind_next(&fi->path, '/');
 
+		/* inherit ignored from parent if no rule specified */
+		if (ff->is_ignored <= GIT_IGNORE_NOTFOUND)
+			ff->is_ignored = ff->next->is_ignored;
+
+		/* push new ignores for files in this directory */
 		(void)git_ignore__push_dir(&wi->ignores, &fi->path.ptr[slash_pos + 1]);
 	}
 
@@ -1342,7 +1355,7 @@ static int workdir_iterator__update_entry(fs_iterator *fi)
 		return GIT_ENOTFOUND;
 
 	/* reset is_ignored since we haven't checked yet */
-	wi->is_ignored = -1;
+	wi->is_ignored = GIT_IGNORE_UNCHECKED;
 
 	return 0;
 }
@@ -1487,6 +1500,19 @@ int git_iterator_current_parent_tree(
 	return 0;
 }
 
+static void workdir_iterator_update_is_ignored(workdir_iterator *wi)
+{
+	if (git_ignore__lookup(
+			&wi->is_ignored, &wi->ignores, wi->fi.entry.path) < 0) {
+		giterr_clear();
+		wi->is_ignored = GIT_IGNORE_NOTFOUND;
+	}
+
+	/* use ignore from containing frame stack */
+	if (wi->is_ignored <= GIT_IGNORE_NOTFOUND)
+		wi->is_ignored = wi->fi.stack->is_ignored;
+}
+
 bool git_iterator_current_is_ignored(git_iterator *iter)
 {
 	workdir_iterator *wi = (workdir_iterator *)iter;
@@ -1494,14 +1520,22 @@ bool git_iterator_current_is_ignored(git_iterator *iter)
 	if (iter->type != GIT_ITERATOR_TYPE_WORKDIR)
 		return false;
 
-	if (wi->is_ignored != -1)
-		return (bool)(wi->is_ignored != 0);
+	if (wi->is_ignored != GIT_IGNORE_UNCHECKED)
+		return (bool)(wi->is_ignored == GIT_IGNORE_TRUE);
 
-	if (git_ignore__lookup(
-			&wi->ignores, wi->fi.entry.path, &wi->is_ignored) < 0)
-		wi->is_ignored = true;
+	workdir_iterator_update_is_ignored(wi);
 
-	return (bool)wi->is_ignored;
+	return (bool)(wi->is_ignored == GIT_IGNORE_TRUE);
+}
+
+bool git_iterator_current_tree_is_ignored(git_iterator *iter)
+{
+	workdir_iterator *wi = (workdir_iterator *)iter;
+
+	if (iter->type != GIT_ITERATOR_TYPE_WORKDIR)
+		return false;
+
+	return (bool)(wi->fi.stack->is_ignored == GIT_IGNORE_TRUE);
 }
 
 int git_iterator_cmp(git_iterator *iter, const char *path_prefix)
@@ -1549,10 +1583,8 @@ int git_iterator_advance_over_with_status(
 		return error;
 
 	if (!S_ISDIR(entry->mode)) {
-		if (git_ignore__lookup(
-				&wi->ignores, wi->fi.entry.path, &wi->is_ignored) < 0)
-			wi->is_ignored = true;
-		if (wi->is_ignored)
+		workdir_iterator_update_is_ignored(wi);
+		if (wi->is_ignored == GIT_IGNORE_TRUE)
 			*status = GIT_ITERATOR_STATUS_IGNORED;
 		return git_iterator_advance(entryptr, iter);
 	}
@@ -1564,14 +1596,12 @@ int git_iterator_advance_over_with_status(
 
 	/* scan inside directory looking for a non-ignored item */
 	while (entry && !iter->prefixcomp(entry->path, base)) {
-		if (git_ignore__lookup(
-				&wi->ignores, wi->fi.entry.path, &wi->is_ignored) < 0)
-			wi->is_ignored = true;
+		workdir_iterator_update_is_ignored(wi);
 
 		/* if we found an explicitly ignored item, then update from
 		 * EMPTY to IGNORED
 		 */
-		if (wi->is_ignored)
+		if (wi->is_ignored == GIT_IGNORE_TRUE)
 			*status = GIT_ITERATOR_STATUS_IGNORED;
 		else if (S_ISDIR(entry->mode)) {
 			error = git_iterator_advance_into(&entry, iter);
@@ -1580,7 +1610,7 @@ int git_iterator_advance_over_with_status(
 				continue;
 			else if (error == GIT_ENOTFOUND) {
 				error = 0;
-				wi->is_ignored = true; /* mark empty directories as ignored */
+				wi->is_ignored = GIT_IGNORE_TRUE; /* mark empty dirs ignored */
 			} else
 				break; /* real error, stop here */
 		} else {

src/iterator.h

@@ -245,6 +245,8 @@ extern int git_iterator_current_parent_tree(
 
 extern bool git_iterator_current_is_ignored(git_iterator *iter);
 
+extern bool git_iterator_current_tree_is_ignored(git_iterator *iter);
+
 extern int git_iterator_cmp(
 	git_iterator *iter, const char *path_prefix);
 

tests/diff/iterator.c

@@ -647,7 +647,7 @@ static void workdir_iterator_test(
 
 void test_diff_iterator__workdir_0(void)
 {
-	workdir_iterator_test("attr", NULL, NULL, 27, 1, NULL, "ign");
+	workdir_iterator_test("attr", NULL, NULL, 23, 5, NULL, "ign");
 }
 
 static const char *status_paths[] = {

tests/status/ignore.c

@@ -681,3 +681,110 @@ void test_status_ignore__issue_1766_negated_ignores(void)
 	}
 }
 
+static void add_one_to_index(const char *file)
+{
+	git_index *index;
+	cl_git_pass(git_repository_index(&index, g_repo));
+	cl_git_pass(git_index_add_bypath(index, file));
+	git_index_free(index);
+}
+
+/* Some further broken scenarios that have been reported */
+void test_status_ignore__more_breakage(void)
+{
+	static const char *test_files[] = {
+		"empty_standard_repo/d1/pfx-d2/d3/d4/d5/tracked",
+		"empty_standard_repo/d1/pfx-d2/d3/d4/d5/untracked",
+		"empty_standard_repo/d1/pfx-d2/d3/d4/untracked",
+		NULL
+	};
+
+	make_test_data("empty_standard_repo", test_files);
+	cl_git_mkfile(
+		"empty_standard_repo/.gitignore",
+		"/d1/pfx-*\n"
+		"!/d1/pfx-d2/\n"
+		"/d1/pfx-d2/*\n"
+		"!/d1/pfx-d2/d3/\n"
+		"/d1/pfx-d2/d3/*\n"
+		"!/d1/pfx-d2/d3/d4/\n");
+	add_one_to_index("d1/pfx-d2/d3/d4/d5/tracked");
+
+	{
+		git_status_options opts = GIT_STATUS_OPTIONS_INIT;
+		status_entry_counts counts;
+		static const char *files[] = {
+			".gitignore",
+			"d1/pfx-d2/d3/d4/d5/tracked",
+			"d1/pfx-d2/d3/d4/d5/untracked",
+			"d1/pfx-d2/d3/d4/untracked",
+		};
+		static const unsigned int statuses[] = {
+			GIT_STATUS_WT_NEW,
+			GIT_STATUS_INDEX_NEW,
+			GIT_STATUS_WT_NEW,
+			GIT_STATUS_WT_NEW,
+		};
+
+		memset(&counts, 0x0, sizeof(status_entry_counts));
+		counts.expected_entry_count = 4;
+		counts.expected_paths = files;
+		counts.expected_statuses = statuses;
+		opts.flags = GIT_STATUS_OPT_DEFAULTS |
+			GIT_STATUS_OPT_INCLUDE_IGNORED |
+			GIT_STATUS_OPT_RECURSE_IGNORED_DIRS;
+		cl_git_pass(git_status_foreach_ext(
+			g_repo, &opts, cb_status__normal, &counts));
+
+		cl_assert_equal_i(counts.expected_entry_count, counts.entry_count);
+		cl_assert_equal_i(0, counts.wrong_status_flags_count);
+		cl_assert_equal_i(0, counts.wrong_sorted_path);
+	}
+
+	refute_is_ignored("d1/pfx-d2/d3/d4/d5/tracked");
+	refute_is_ignored("d1/pfx-d2/d3/d4/d5/untracked");
+	refute_is_ignored("d1/pfx-d2/d3/d4/untracked");
+}
+
+void test_status_ignore__negative_ignores_inside_ignores(void)
+{
+	static const char *test_files[] = {
+		"empty_standard_repo/top/mid/btm/tracked",
+		"empty_standard_repo/top/mid/btm/untracked",
+		NULL
+	};
+
+	make_test_data("empty_standard_repo", test_files);
+	cl_git_mkfile(
+		"empty_standard_repo/.gitignore",
+		"top\n!top/mid/btm\n");
+	add_one_to_index("top/mid/btm/tracked");
+
+	{
+		git_status_options opts = GIT_STATUS_OPTIONS_INIT;
+		status_entry_counts counts;
+		static const char *files[] = {
+			".gitignore", "top/mid/btm/tracked", "top/mid/btm/untracked",
+		};
+		static const unsigned int statuses[] = {
+			GIT_STATUS_WT_NEW, GIT_STATUS_INDEX_NEW, GIT_STATUS_WT_NEW,
+		};
+
+		memset(&counts, 0x0, sizeof(status_entry_counts));
+		counts.expected_entry_count = 3;
+		counts.expected_paths = files;
+		counts.expected_statuses = statuses;
+		opts.flags = GIT_STATUS_OPT_DEFAULTS |
+			GIT_STATUS_OPT_INCLUDE_IGNORED |
+			GIT_STATUS_OPT_RECURSE_IGNORED_DIRS;
+		cl_git_pass(git_status_foreach_ext(
+			g_repo, &opts, cb_status__normal, &counts));
+
+		cl_assert_equal_i(counts.expected_entry_count, counts.entry_count);
+		cl_assert_equal_i(0, counts.wrong_status_flags_count);
+		cl_assert_equal_i(0, counts.wrong_sorted_path);
+	}
+
+	refute_is_ignored("top/mid/btm/tracked");
+	refute_is_ignored("top/mid/btm/untracked");
+}