Commit f554611a by Russell Belfer

Improve checks for ignore containment

The diff code was using an "ignored_prefix" directory to track if
a parent directory was ignored that contained untracked files
alongside tracked files. Unfortunately, when negative ignore rules
were used for directories inside ignored parents, the wrong rules
were applied to untracked files inside the negatively ignored
child directories.

This commit moves the logic for ignore containment into the workdir
iterator (which is a better place for it), so the ignored-ness of
a directory is contained in the frame stack during traversal.  This
allows a child directory to override with a negative ignore and yet
still restore the ignored state of the parent when we traverse out
of the child.

Along with this, there are some problems with "directory only"
ignore rules on container directories.  Given "a/*" and "!a/b/c/"
(where the second rule is a directory rule but the first rule is
just a generic prefix rule), then the directory only constraint
was having "a/b/c/d/file" match the first rule and not the second.
This was fixed by having ignore directory-only rules test a rule
against the prefix of a file with LEADINGDIR enabled.

Lastly, spot checks for ignores using `git_ignore_path_is_ignored`
were tested from the top directory down to the bottom to deal with
the containment problem, but this is wrong. We have to test bottom
to top so that negative subdirectory rules will be checked before
parent ignore rules.

This does change the behavior of some existing tests, but it seems
only to bring us more in line with core Git, so I think those
changes are acceptable.
parent d2c16e9a
......@@ -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);
......
......@@ -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);
......
......@@ -632,7 +632,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
......@@ -843,24 +842,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 &&
......@@ -937,27 +925,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;
......@@ -1067,7 +1040,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)) {
......@@ -1122,8 +1094,6 @@ cleanup:
else
git_diff_free(diff);
git_buf_free(&info.ignore_prefix);
return error;
}
......
......@@ -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,
......
......@@ -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
......
......@@ -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 {
......
......@@ -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);
......
......@@ -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[] = {
......
......@@ -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");
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment