From ba8b8c040744edb9bc832a67646e0126636753bb Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 7 Aug 2013 09:17:20 -0700 Subject: [PATCH 1/2] Improve building ignore file lists The routines to push and pop ignore files while traversing a directory had some issues. In particular, setting up the initial list would sometimes push an ignore file before it ought to be applied if the starting path was a directory containing an ignore file. Also, the pop function was not always matching the right part of the path and would fail to pop ignores from the list in some cases. This adds some tests that exercise a particular problematic case and then fixes the problems that I could find related to this. At some point, I'd like to isolate this ignore rule management code and rewrite it, but that's a larger project and right now, I'll opt to just try to fix the broken behaviors. --- src/ignore.c | 30 ++++++++----- src/ignore.h | 5 ++- src/iterator.c | 2 +- src/path.c | 2 +- tests-clar/status/ignore.c | 87 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 112 insertions(+), 14 deletions(-) diff --git a/src/ignore.c b/src/ignore.c index 3c23158c2..ca63f0bd9 100644 --- a/src/ignore.c +++ b/src/ignore.c @@ -159,17 +159,26 @@ int git_ignore__push_dir(git_ignores *ign, const char *dir) { if (git_buf_joinpath(&ign->dir, ign->dir.ptr, dir) < 0) return -1; - else - return push_ignore_file( - ign->repo, ign, &ign->ign_path, ign->dir.ptr, GIT_IGNORE_FILE); + + return push_ignore_file( + ign->repo, ign, &ign->ign_path, ign->dir.ptr, GIT_IGNORE_FILE); } int git_ignore__pop_dir(git_ignores *ign) { if (ign->ign_path.length > 0) { git_attr_file *file = git_vector_last(&ign->ign_path); - if (git__suffixcmp(ign->dir.ptr, file->key + 2) == 0) + size_t keylen = strlen(file->key); + + while (keylen && file->key[keylen] != '/') + keylen--; + keylen -= 1; /* because we will skip "0#" prefix */ + + if (ign->dir.size > keylen && + !memcmp(ign->dir.ptr + ign->dir.size - keylen, + file->key + 2, keylen)) git_vector_pop(&ign->ign_path); + git_buf_rtruncate_at_char(&ign->dir, '/'); } return 0; @@ -298,12 +307,9 @@ int git_ignore_path_is_ignored( path.full.size = (tail - path.full.ptr); path.is_dir = (tail == end) ? full_is_dir : true; - /* update ignores for new path fragment */ - if (path.basename == path.path) - error = git_ignore__for_path(repo, path.path, &ignores); - else - error = git_ignore__push_dir(&ignores, path.basename); - if (error < 0) + /* initialize ignores the first time through */ + if (path.basename == path.path && + (error = git_ignore__for_path(repo, path.path, &ignores)) < 0) break; /* first process builtins - success means path was found */ @@ -327,6 +333,10 @@ int git_ignore_path_is_ignored( if (tail == end) break; + /* now add this directory to list of ignores */ + if ((error = git_ignore__push_dir(&ignores, path.path)) < 0) + break; + /* reinstate divider in path */ *tail = '/'; while (*tail == '/') tail++; diff --git a/src/ignore.h b/src/ignore.h index cc114b001..851c824bf 100644 --- a/src/ignore.h +++ b/src/ignore.h @@ -24,14 +24,15 @@ */ typedef struct { git_repository *repo; - git_buf dir; + git_buf dir; /* current directory reflected in ign_path */ git_attr_file *ign_internal; git_vector ign_path; git_vector ign_global; int ignore_case; } git_ignores; -extern int git_ignore__for_path(git_repository *repo, const char *path, git_ignores *ign); +extern int git_ignore__for_path( + git_repository *repo, const char *path, git_ignores *ign); extern int git_ignore__push_dir(git_ignores *ign, const char *dir); diff --git a/src/iterator.c b/src/iterator.c index 5917f63fd..bdc98d22b 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -1350,7 +1350,7 @@ int git_iterator_for_workdir_ext( wi->fi.update_entry_cb = workdir_iterator__update_entry; if ((error = iterator__update_ignore_case((git_iterator *)wi, flags)) < 0 || - (error = git_ignore__for_path(repo, "", &wi->ignores)) < 0) + (error = git_ignore__for_path(repo, ".gitignore", &wi->ignores)) < 0) { git_iterator_free((git_iterator *)wi); return error; diff --git a/src/path.c b/src/path.c index 6437979d5..b81675b49 100644 --- a/src/path.c +++ b/src/path.c @@ -603,7 +603,7 @@ int git_path_find_dir(git_buf *dir, const char *path, const char *base) } /* call dirname if this is not a directory */ - if (!error && git_path_isdir(dir->ptr) == false) + if (!error) /* && git_path_isdir(dir->ptr) == false) */ error = git_path_dirname_r(dir, dir->ptr); if (!error) diff --git a/tests-clar/status/ignore.c b/tests-clar/status/ignore.c index 1b41fed1a..acdc8fb58 100644 --- a/tests-clar/status/ignore.c +++ b/tests-clar/status/ignore.c @@ -493,3 +493,90 @@ void test_status_ignore__filenames_with_special_prefixes_do_not_interfere_with_s git_buf_free(&file); } } + +void test_status_ignore__issue_1766_negated_ignores(void) +{ + int ignored = 0; + unsigned int status; + + g_repo = cl_git_sandbox_init("empty_standard_repo"); + + cl_git_pass(git_futils_mkdir_r( + "empty_standard_repo/a", NULL, 0775)); + cl_git_mkfile( + "empty_standard_repo/a/.gitignore", "*\n!.gitignore\n"); + cl_git_mkfile( + "empty_standard_repo/a/ignoreme", "I should be ignored\n"); + + cl_git_pass(git_status_should_ignore(&ignored, g_repo, "a/.gitignore")); + cl_assert(!ignored); + + cl_git_pass(git_status_should_ignore(&ignored, g_repo, "a/ignoreme")); + cl_assert(ignored); + + cl_git_pass(git_futils_mkdir_r( + "empty_standard_repo/b", NULL, 0775)); + cl_git_mkfile( + "empty_standard_repo/b/.gitignore", "*\n!.gitignore\n"); + cl_git_mkfile( + "empty_standard_repo/b/ignoreme", "I should be ignored\n"); + + cl_git_pass(git_status_should_ignore(&ignored, g_repo, "b/.gitignore")); + cl_assert(!ignored); + + cl_git_pass(git_status_should_ignore(&ignored, g_repo, "b/ignoreme")); + cl_assert(ignored); + + /* shouldn't have changed results from first couple either */ + + cl_git_pass(git_status_should_ignore(&ignored, g_repo, "a/.gitignore")); + cl_assert(!ignored); + cl_git_pass(git_status_should_ignore(&ignored, g_repo, "a/ignoreme")); + cl_assert(ignored); + + /* status should find the two ignore files and nothing else */ + + cl_git_pass(git_status_file(&status, g_repo, "a/.gitignore")); + cl_assert_equal_i(GIT_STATUS_WT_NEW, (int)status); + + cl_git_pass(git_status_file(&status, g_repo, "a/ignoreme")); + cl_assert_equal_i(GIT_STATUS_IGNORED, (int)status); + + cl_git_pass(git_status_file(&status, g_repo, "b/.gitignore")); + cl_assert_equal_i(GIT_STATUS_WT_NEW, (int)status); + + cl_git_pass(git_status_file(&status, g_repo, "b/ignoreme")); + cl_assert_equal_i(GIT_STATUS_IGNORED, (int)status); + + { + git_status_options opts = GIT_STATUS_OPTIONS_INIT; + status_entry_counts counts; + static const char *paths[] = { + "a/.gitignore", + "a/ignoreme", + "b/.gitignore", + "b/ignoreme", + }; + static const unsigned int statuses[] = { + GIT_STATUS_WT_NEW, + GIT_STATUS_IGNORED, + GIT_STATUS_WT_NEW, + GIT_STATUS_IGNORED, + }; + + memset(&counts, 0x0, sizeof(status_entry_counts)); + counts.expected_entry_count = 4; + counts.expected_paths = paths; + counts.expected_statuses = statuses; + + opts.flags = GIT_STATUS_OPT_DEFAULTS; + + 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); + } +} + From 3bc3ed80f476bcef24a508cadd45d4c341ef60c7 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 9 Aug 2013 10:06:23 -0700 Subject: [PATCH 2/2] Improve and comment git_ignore__pop_dir This just cleans up the improved logic for popping ignore dirs and documents why the complex behavior is needed. --- src/ignore.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/ignore.c b/src/ignore.c index ca63f0bd9..0c35d0431 100644 --- a/src/ignore.c +++ b/src/ignore.c @@ -168,15 +168,25 @@ int git_ignore__pop_dir(git_ignores *ign) { if (ign->ign_path.length > 0) { git_attr_file *file = git_vector_last(&ign->ign_path); - size_t keylen = strlen(file->key); + const char *start, *end, *scan; + size_t keylen; - while (keylen && file->key[keylen] != '/') - keylen--; - keylen -= 1; /* because we will skip "0#" prefix */ + /* - ign->dir looks something like "a/b" (or "a/b/c/d") + * - file->key looks something like "0#a/b/.gitignore + * + * We are popping the last directory off ign->dir. We also want to + * remove the file from the vector if the directory part of the key + * matches the ign->dir path. We need to test if the "a/b" part of + * the file key matches the path we are about to pop. + */ - if (ign->dir.size > keylen && - !memcmp(ign->dir.ptr + ign->dir.size - keylen, - file->key + 2, keylen)) + for (start = end = scan = &file->key[2]; *scan; ++scan) + if (*scan == '/') + end = scan; /* point 'end' to last '/' in key */ + keylen = (end - start) + 1; + + if (ign->dir.size >= keylen && + !memcmp(ign->dir.ptr + ign->dir.size - keylen, start, keylen)) git_vector_pop(&ign->ign_path); git_buf_rtruncate_at_char(&ign->dir, '/');