From 6a0956e504328f6584af971e840c202ecb21b5b6 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 18 Apr 2014 10:32:35 -0700 Subject: [PATCH 1/5] Pop ignore only if whole relative path matches When traversing the directory structure, the iterator pushes and pops ignore files using a vector. Some directories don't have ignore files, so it uses a path comparison to decide when it is right to actually pop the last ignore file. This was only comparing directory suffixes, though, so a subdirectory with the same name as a parent could result in the parent's .gitignore being popped off the list ignores too early. This changes the logic to compare the entire relative path of the ignore file. --- src/ignore.c | 13 ++++-- src/ignore.h | 1 + src/path.c | 2 +- tests/status/ignore.c | 86 +++++++++++++++++++++++++++-------- tests/status/status_helpers.c | 13 ++---- 5 files changed, 80 insertions(+), 35 deletions(-) diff --git a/src/ignore.c b/src/ignore.c index deae204f8..b08ff2200 100644 --- a/src/ignore.c +++ b/src/ignore.c @@ -123,7 +123,7 @@ int git_ignore__for_path( int error = 0; const char *workdir = git_repository_workdir(repo); - assert(ignores); + assert(ignores && path); memset(ignores, 0, sizeof(*ignores)); ignores->repo = repo; @@ -140,10 +140,13 @@ int git_ignore__for_path( if (workdir && git_path_root(path) < 0) error = git_path_find_dir(&ignores->dir, path, workdir); else - error = git_buf_sets(&ignores->dir, path); + error = git_buf_joinpath(&ignores->dir, path, ""); if (error < 0) goto cleanup; + if (workdir && !git__prefixcmp(ignores->dir.ptr, workdir)) + ignores->dir_root = strlen(workdir); + /* set up internals */ if ((error = get_internal_ignores(&ignores->ign_internal, repo)) < 0) goto cleanup; @@ -204,10 +207,10 @@ int git_ignore__pop_dir(git_ignores *ign) if ((end = strrchr(start, '/')) != NULL) { size_t dirlen = (end - start) + 1; + const char *relpath = ign->dir.ptr + ign->dir_root; + size_t pathlen = ign->dir.size - ign->dir_root; - if (ign->dir.size >= dirlen && - !memcmp(ign->dir.ptr + ign->dir.size - dirlen, start, dirlen)) - { + if (pathlen == dirlen && !memcmp(relpath, start, dirlen)) { git_vector_pop(&ign->ign_path); git_attr_file__free(file); } diff --git a/src/ignore.h b/src/ignore.h index 46172c72f..ff9369000 100644 --- a/src/ignore.h +++ b/src/ignore.h @@ -28,6 +28,7 @@ typedef struct { git_attr_file *ign_internal; git_vector ign_path; git_vector ign_global; + size_t dir_root; /* offset in dir to repo root */ int ignore_case; int depth; } git_ignores; diff --git a/src/path.c b/src/path.c index 7cad28d45..a990b005f 100644 --- a/src/path.c +++ b/src/path.c @@ -624,7 +624,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) */ - error = git_path_dirname_r(dir, dir->ptr); + error = (git_path_dirname_r(dir, dir->ptr) < 0) ? -1 : 0; if (!error) error = git_path_to_dir(dir); diff --git a/tests/status/ignore.c b/tests/status/ignore.c index 052a8eae8..eb171deb6 100644 --- a/tests/status/ignore.c +++ b/tests/status/ignore.c @@ -230,32 +230,34 @@ void test_status_ignore__subdirectories(void) cl_assert(ignored); } -static void make_test_data(void) +static void make_test_data(const char *reponame, const char **files) { - static const char *files[] = { - "empty_standard_repo/dir/a/ignore_me", - "empty_standard_repo/dir/b/ignore_me", - "empty_standard_repo/dir/ignore_me", - "empty_standard_repo/ignore_also/file", - "empty_standard_repo/ignore_me", - "empty_standard_repo/test/ignore_me/file", - "empty_standard_repo/test/ignore_me/file2", - "empty_standard_repo/test/ignore_me/and_me/file", - NULL - }; - static const char *repo = "empty_standard_repo"; const char **scan; - size_t repolen = strlen(repo) + 1; + size_t repolen = strlen(reponame) + 1; - g_repo = cl_git_sandbox_init(repo); + g_repo = cl_git_sandbox_init(reponame); for (scan = files; *scan != NULL; ++scan) { cl_git_pass(git_futils_mkdir( - *scan + repolen, repo, 0777, GIT_MKDIR_PATH | GIT_MKDIR_SKIP_LAST)); + *scan + repolen, reponame, + 0777, GIT_MKDIR_PATH | GIT_MKDIR_SKIP_LAST)); cl_git_mkfile(*scan, "contents"); } } +static const char *test_repo_1 = "empty_standard_repo"; +static const char *test_files_1[] = { + "empty_standard_repo/dir/a/ignore_me", + "empty_standard_repo/dir/b/ignore_me", + "empty_standard_repo/dir/ignore_me", + "empty_standard_repo/ignore_also/file", + "empty_standard_repo/ignore_me", + "empty_standard_repo/test/ignore_me/file", + "empty_standard_repo/test/ignore_me/file2", + "empty_standard_repo/test/ignore_me/and_me/file", + NULL +}; + void test_status_ignore__subdirectories_recursion(void) { /* Let's try again with recursing into ignored dirs turned on */ @@ -292,7 +294,7 @@ void test_status_ignore__subdirectories_recursion(void) GIT_STATUS_IGNORED, GIT_STATUS_IGNORED, GIT_STATUS_IGNORED, }; - make_test_data(); + make_test_data(test_repo_1, test_files_1); cl_git_rewritefile("empty_standard_repo/.gitignore", "ignore_me\n/ignore_also\n"); memset(&counts, 0x0, sizeof(status_entry_counts)); @@ -347,7 +349,7 @@ void test_status_ignore__subdirectories_not_at_root(void) GIT_STATUS_WT_NEW, GIT_STATUS_IGNORED, GIT_STATUS_WT_NEW, GIT_STATUS_WT_NEW, }; - make_test_data(); + make_test_data(test_repo_1, test_files_1); cl_git_rewritefile("empty_standard_repo/dir/.gitignore", "ignore_me\n/ignore_also\n"); cl_git_rewritefile("empty_standard_repo/test/.gitignore", "and_me\n"); @@ -389,7 +391,7 @@ void test_status_ignore__leading_slash_ignores(void) GIT_STATUS_WT_NEW, GIT_STATUS_WT_NEW, GIT_STATUS_WT_NEW, GIT_STATUS_WT_NEW, }; - make_test_data(); + make_test_data(test_repo_1, test_files_1); cl_fake_home(&home); cl_git_mkfile("home/.gitignore", "/ignore_me\n"); @@ -422,6 +424,52 @@ void test_status_ignore__leading_slash_ignores(void) cl_fake_home_cleanup(&home); } +void test_status_ignore__contained_dir_with_matching_name(void) +{ + static const char *test_files[] = { + "empty_standard_repo/subdir_match/aaa/subdir_match/file", + "empty_standard_repo/subdir_match/zzz_ignoreme", + NULL + }; + static const char *expected_paths[] = { + "subdir_match/.gitignore", + "subdir_match/aaa/subdir_match/file", + "subdir_match/zzz_ignoreme", + }; + static const unsigned int expected_statuses[] = { + GIT_STATUS_WT_NEW, GIT_STATUS_WT_NEW, GIT_STATUS_IGNORED + }; + int ignored; + git_status_options opts = GIT_STATUS_OPTIONS_INIT; + status_entry_counts counts; + + make_test_data("empty_standard_repo", test_files); + cl_git_mkfile( + "empty_standard_repo/subdir_match/.gitignore", "*_ignoreme\n"); + + cl_git_pass(git_status_should_ignore( + &ignored, g_repo, "subdir_match/aaa/subdir_match/file")); + cl_assert(!ignored); + + cl_git_pass(git_status_should_ignore( + &ignored, g_repo, "subdir_match/zzz_ignoreme")); + cl_assert(ignored); + + memset(&counts, 0x0, sizeof(status_entry_counts)); + counts.expected_entry_count = 3; + counts.expected_paths = expected_paths; + counts.expected_statuses = expected_statuses; + + opts.flags = GIT_STATUS_OPT_DEFAULTS | 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); +} + void test_status_ignore__adding_internal_ignores(void) { int ignored; diff --git a/tests/status/status_helpers.c b/tests/status/status_helpers.c index 902b65c4f..088279252 100644 --- a/tests/status/status_helpers.c +++ b/tests/status/status_helpers.c @@ -9,20 +9,13 @@ int cb_status__normal( if (counts->debug) cb_status__print(path, status_flags, NULL); - if (counts->entry_count >= counts->expected_entry_count) { + if (counts->entry_count >= counts->expected_entry_count) counts->wrong_status_flags_count++; - goto exit; - } - - if (strcmp(path, counts->expected_paths[counts->entry_count])) { + else if (strcmp(path, counts->expected_paths[counts->entry_count])) counts->wrong_sorted_path++; - goto exit; - } - - if (status_flags != counts->expected_statuses[counts->entry_count]) + else if (status_flags != counts->expected_statuses[counts->entry_count]) counts->wrong_status_flags_count++; -exit: counts->entry_count++; return 0; } From 50e46d6018ede64e3e4b177baa4ad8156d928fbd Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 18 Apr 2014 10:58:01 -0700 Subject: [PATCH 2/5] Cleanup tests with helper functions --- tests/attr/ignore.c | 2 +- tests/status/ignore.c | 229 +++++++++++++++--------------------------- 2 files changed, 80 insertions(+), 151 deletions(-) diff --git a/tests/attr/ignore.c b/tests/attr/ignore.c index a83c5bd74..68875194d 100644 --- a/tests/attr/ignore.c +++ b/tests/attr/ignore.c @@ -16,7 +16,7 @@ void test_attr_ignore__cleanup(void) g_repo = NULL; } -void assert_is_ignored_( +static void assert_is_ignored_( bool expected, const char *filepath, const char *file, int line) { int is_ignored = 0; diff --git a/tests/status/ignore.c b/tests/status/ignore.c index eb171deb6..15c85e262 100644 --- a/tests/status/ignore.c +++ b/tests/status/ignore.c @@ -16,6 +16,23 @@ void test_status_ignore__cleanup(void) cl_git_sandbox_cleanup(); } +static void assert_ignored_( + bool expected, const char *filepath, const char *file, int line) +{ + int is_ignored = 0; + cl_git_pass_( + git_status_should_ignore(&is_ignored, g_repo, filepath), file, line); + clar__assert( + (expected != 0) == (is_ignored != 0), + file, line, "expected != is_ignored", filepath, 1); +} +#define assert_ignored(expected, filepath) \ + assert_ignored_(expected, filepath, __FILE__, __LINE__) +#define assert_is_ignored(filepath) \ + assert_ignored_(true, filepath, __FILE__, __LINE__) +#define refute_is_ignored(filepath) \ + assert_ignored_(false, filepath, __FILE__, __LINE__) + void test_status_ignore__0(void) { struct { @@ -47,11 +64,8 @@ void test_status_ignore__0(void) g_repo = cl_git_sandbox_init("attr"); - for (one_test = test_cases; one_test->path != NULL; one_test++) { - int ignored; - cl_git_pass(git_status_should_ignore(&ignored, g_repo, one_test->path)); - cl_assert_(ignored == one_test->expected, one_test->path); - } + for (one_test = test_cases; one_test->path != NULL; one_test++) + assert_ignored(one_test->expected, one_test->path); /* confirm that ignore files were cached */ cl_assert(git_attr_cache__is_cached( @@ -63,37 +77,22 @@ void test_status_ignore__0(void) void test_status_ignore__1(void) { - int ignored; - g_repo = cl_git_sandbox_init("attr"); cl_git_rewritefile("attr/.gitignore", "/*.txt\n/dir/\n"); git_attr_cache_flush(g_repo); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "root_test4.txt")); - cl_assert(ignored); - - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "sub/subdir_test2.txt")); - cl_assert(!ignored); - - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "dir")); - cl_assert(ignored); - - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "dir/")); - cl_assert(ignored); - - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "sub/dir")); - cl_assert(!ignored); - - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "sub/dir/")); - cl_assert(!ignored); + assert_is_ignored("root_test4.txt"); + refute_is_ignored("sub/subdir_test2.txt"); + assert_is_ignored("dir"); + assert_is_ignored("dir/"); + refute_is_ignored("sub/dir"); + refute_is_ignored("sub/dir/"); } - void test_status_ignore__empty_repo_with_gitignore_rewrite(void) { status_entry_single st; - int ignored; g_repo = cl_git_sandbox_init("empty_standard_repo"); @@ -108,8 +107,7 @@ void test_status_ignore__empty_repo_with_gitignore_rewrite(void) cl_git_pass(git_status_file(&st.status, g_repo, "look-ma.txt")); cl_assert(st.status == GIT_STATUS_WT_NEW); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "look-ma.txt")); - cl_assert(!ignored); + refute_is_ignored("look-ma.txt"); cl_git_rewritefile("empty_standard_repo/.gitignore", "*.nomatch\n"); @@ -121,8 +119,7 @@ void test_status_ignore__empty_repo_with_gitignore_rewrite(void) cl_git_pass(git_status_file(&st.status, g_repo, "look-ma.txt")); cl_assert(st.status == GIT_STATUS_WT_NEW); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "look-ma.txt")); - cl_assert(!ignored); + refute_is_ignored("look-ma.txt"); cl_git_rewritefile("empty_standard_repo/.gitignore", "*.txt\n"); @@ -134,8 +131,7 @@ void test_status_ignore__empty_repo_with_gitignore_rewrite(void) cl_git_pass(git_status_file(&st.status, g_repo, "look-ma.txt")); cl_assert(st.status == GIT_STATUS_IGNORED); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "look-ma.txt")); - cl_assert(ignored); + assert_is_ignored("look-ma.txt"); } void test_status_ignore__ignore_pattern_contains_space(void) @@ -181,7 +177,6 @@ void test_status_ignore__ignore_pattern_ignorecase(void) void test_status_ignore__subdirectories(void) { status_entry_single st; - int ignored; g_repo = cl_git_sandbox_init("empty_standard_repo"); @@ -198,8 +193,7 @@ void test_status_ignore__subdirectories(void) cl_git_pass(git_status_file(&st.status, g_repo, "ignore_me")); cl_assert(st.status == GIT_STATUS_IGNORED); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "ignore_me")); - cl_assert(ignored); + assert_is_ignored("ignore_me"); /* I've changed libgit2 so that the behavior here now differs from * core git but seems to make more sense. In core git, the following @@ -225,9 +219,7 @@ void test_status_ignore__subdirectories(void) cl_git_pass(git_status_file(&st.status, g_repo, "test/ignore_me/file")); cl_assert(st.status == GIT_STATUS_IGNORED); - cl_git_pass( - git_status_should_ignore(&ignored, g_repo, "test/ignore_me/file")); - cl_assert(ignored); + assert_is_ignored("test/ignore_me/file"); } static void make_test_data(const char *reponame, const char **files) @@ -439,7 +431,6 @@ void test_status_ignore__contained_dir_with_matching_name(void) static const unsigned int expected_statuses[] = { GIT_STATUS_WT_NEW, GIT_STATUS_WT_NEW, GIT_STATUS_IGNORED }; - int ignored; git_status_options opts = GIT_STATUS_OPTIONS_INIT; status_entry_counts counts; @@ -447,13 +438,8 @@ void test_status_ignore__contained_dir_with_matching_name(void) cl_git_mkfile( "empty_standard_repo/subdir_match/.gitignore", "*_ignoreme\n"); - cl_git_pass(git_status_should_ignore( - &ignored, g_repo, "subdir_match/aaa/subdir_match/file")); - cl_assert(!ignored); - - cl_git_pass(git_status_should_ignore( - &ignored, g_repo, "subdir_match/zzz_ignoreme")); - cl_assert(ignored); + refute_is_ignored("subdir_match/aaa/subdir_match/file"); + assert_is_ignored("subdir_match/zzz_ignoreme"); memset(&counts, 0x0, sizeof(status_entry_counts)); counts.expected_entry_count = 3; @@ -472,149 +458,102 @@ void test_status_ignore__contained_dir_with_matching_name(void) void test_status_ignore__adding_internal_ignores(void) { - int ignored; - g_repo = cl_git_sandbox_init("empty_standard_repo"); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "one.txt")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "two.bar")); - cl_assert(!ignored); + refute_is_ignored("one.txt"); + refute_is_ignored("two.bar"); cl_git_pass(git_ignore_add_rule(g_repo, "*.nomatch\n")); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "one.txt")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "two.bar")); - cl_assert(!ignored); + refute_is_ignored("one.txt"); + refute_is_ignored("two.bar"); cl_git_pass(git_ignore_add_rule(g_repo, "*.txt\n")); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "one.txt")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "two.bar")); - cl_assert(!ignored); + assert_is_ignored("one.txt"); + refute_is_ignored("two.bar"); cl_git_pass(git_ignore_add_rule(g_repo, "*.bar\n")); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "one.txt")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "two.bar")); - cl_assert(ignored); + assert_is_ignored("one.txt"); + assert_is_ignored("two.bar"); cl_git_pass(git_ignore_clear_internal_rules(g_repo)); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "one.txt")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "two.bar")); - cl_assert(!ignored); + refute_is_ignored("one.txt"); + refute_is_ignored("two.bar"); cl_git_pass(git_ignore_add_rule( g_repo, "multiple\n*.rules\n# comment line\n*.bar\n")); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "one.txt")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "two.bar")); - cl_assert(ignored); + refute_is_ignored("one.txt"); + assert_is_ignored("two.bar"); } void test_status_ignore__add_internal_as_first_thing(void) { - int ignored; const char *add_me = "\n#################\n## Eclipse\n#################\n\n*.pydevproject\n.project\n.metadata\nbin/\ntmp/\n*.tmp\n\n"; g_repo = cl_git_sandbox_init("empty_standard_repo"); cl_git_pass(git_ignore_add_rule(g_repo, add_me)); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "one.tmp")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "two.bar")); - cl_assert(!ignored); + assert_is_ignored("one.tmp"); + refute_is_ignored("two.bar"); } void test_status_ignore__internal_ignores_inside_deep_paths(void) { - int ignored; const char *add_me = "Debug\nthis/is/deep\npatterned*/dir\n"; g_repo = cl_git_sandbox_init("empty_standard_repo"); cl_git_pass(git_ignore_add_rule(g_repo, add_me)); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "Debug")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "and/Debug")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "really/Debug/this/file")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "Debug/what/I/say")); - cl_assert(ignored); + assert_is_ignored("Debug"); + assert_is_ignored("and/Debug"); + assert_is_ignored("really/Debug/this/file"); + assert_is_ignored("Debug/what/I/say"); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "and/NoDebug")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "NoDebug/this")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "please/NoDebug/this")); - cl_assert(!ignored); + refute_is_ignored("and/NoDebug"); + refute_is_ignored("NoDebug/this"); + refute_is_ignored("please/NoDebug/this"); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "this/is/deep")); - cl_assert(ignored); + assert_is_ignored("this/is/deep"); /* pattern containing slash gets FNM_PATHNAME so all slashes must match */ - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "and/this/is/deep")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "this/is/deep/too")); - cl_assert(ignored); + refute_is_ignored("and/this/is/deep"); + assert_is_ignored("this/is/deep/too"); /* pattern containing slash gets FNM_PATHNAME so all slashes must match */ - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "but/this/is/deep/and/ignored")); - cl_assert(!ignored); + refute_is_ignored("but/this/is/deep/and/ignored"); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "this/is/not/deep")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "is/this/not/as/deep")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "this/is/deepish")); - cl_assert(!ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "xthis/is/deep")); - cl_assert(!ignored); + refute_is_ignored("this/is/not/deep"); + refute_is_ignored("is/this/not/as/deep"); + refute_is_ignored("this/is/deepish"); + refute_is_ignored("xthis/is/deep"); } void test_status_ignore__automatically_ignore_bad_files(void) { - int ignored; - g_repo = cl_git_sandbox_init("empty_standard_repo"); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, ".git")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "this/file/.")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "path/../funky")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "path/whatever.c")); - cl_assert(!ignored); + assert_is_ignored(".git"); + assert_is_ignored("this/file/."); + assert_is_ignored("path/../funky"); + refute_is_ignored("path/whatever.c"); cl_git_pass(git_ignore_add_rule(g_repo, "*.c\n")); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, ".git")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "this/file/.")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "path/../funky")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "path/whatever.c")); - cl_assert(ignored); + assert_is_ignored(".git"); + assert_is_ignored("this/file/."); + assert_is_ignored("path/../funky"); + assert_is_ignored("path/whatever.c"); cl_git_pass(git_ignore_clear_internal_rules(g_repo)); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, ".git")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "this/file/.")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "path/../funky")); - cl_assert(ignored); - cl_git_pass(git_status_should_ignore(&ignored, g_repo, "path/whatever.c")); - cl_assert(!ignored); + assert_is_ignored(".git"); + assert_is_ignored("this/file/."); + assert_is_ignored("path/../funky"); + refute_is_ignored("path/whatever.c"); } void test_status_ignore__filenames_with_special_prefixes_do_not_interfere_with_status_retrieval(void) @@ -653,7 +592,6 @@ void test_status_ignore__filenames_with_special_prefixes_do_not_interfere_with_s void test_status_ignore__issue_1766_negated_ignores(void) { - int ignored = 0; unsigned int status; g_repo = cl_git_sandbox_init("empty_standard_repo"); @@ -665,11 +603,8 @@ void test_status_ignore__issue_1766_negated_ignores(void) 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); + refute_is_ignored("a/.gitignore"); + assert_is_ignored("a/ignoreme"); cl_git_pass(git_futils_mkdir_r( "empty_standard_repo/b", NULL, 0775)); @@ -678,18 +613,12 @@ void test_status_ignore__issue_1766_negated_ignores(void) 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); + refute_is_ignored("b/.gitignore"); + assert_is_ignored("b/ignoreme"); /* 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); + refute_is_ignored("a/.gitignore"); + assert_is_ignored("a/ignoreme"); /* status should find the two ignore files and nothing else */ From e3a2a04ceff1d3657629fd6a7245d9a9fc53f24b Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 18 Apr 2014 14:29:58 -0700 Subject: [PATCH 3/5] Preload attribute files that may contain macros There was a latent bug where files that use macro definitions could be parsed before the macro definitions were loaded. Because of attribute file caching, preloading files that are going to be used doesn't add a significant amount of overhead, so let's always preload any files that could contain macros before we assemble the actual vector of files to scan for attributes. --- src/attr.c | 74 +++++++++++++++++++++++++++++++-- src/attr_file.c | 4 +- tests/attr/repo.c | 101 +++++++++++++++++++++++++++++----------------- 3 files changed, 135 insertions(+), 44 deletions(-) diff --git a/src/attr.c b/src/attr.c index 622874348..6b9a3d614 100644 --- a/src/attr.c +++ b/src/attr.c @@ -217,6 +217,74 @@ cleanup: return error; } +static int preload_attr_file( + git_repository *repo, + git_attr_file_source source, + const char *base, + const char *file) +{ + int error; + git_attr_file *preload = NULL; + + if (!file) + return 0; + if (!(error = git_attr_cache__get( + &preload, repo, source, base, file, git_attr_file__parse_buffer))) + git_attr_file__free(preload); + + return error; +} + +static int attr_setup(git_repository *repo) +{ + int error = 0; + const char *workdir = git_repository_workdir(repo); + git_index *idx = NULL; + git_buf sys = GIT_BUF_INIT; + + if ((error = git_attr_cache__init(repo)) < 0) + return error; + + /* preload attribute files that could contain macros so the + * definitions will be available for later file parsing + */ + + if (!(error = git_sysdir_find_system_file(&sys, GIT_ATTR_FILE_SYSTEM))) { + error = preload_attr_file( + repo, GIT_ATTR_FILE__FROM_FILE, NULL, sys.ptr); + git_buf_free(&sys); + } + if (error < 0) { + if (error == GIT_ENOTFOUND) { + giterr_clear(); + error = 0; + } else + return error; + } + + if ((error = preload_attr_file( + repo, GIT_ATTR_FILE__FROM_FILE, + NULL, git_repository_attr_cache(repo)->cfg_attr_file)) < 0) + return error; + + if ((error = preload_attr_file( + repo, GIT_ATTR_FILE__FROM_FILE, + git_repository_path(repo), GIT_ATTR_FILE_INREPO)) < 0) + return error; + + if (workdir != NULL && + (error = preload_attr_file( + repo, GIT_ATTR_FILE__FROM_FILE, workdir, GIT_ATTR_FILE)) < 0) + return error; + + if ((error = git_repository_index__weakptr(&idx, repo)) < 0 || + (error = preload_attr_file( + repo, GIT_ATTR_FILE__FROM_INDEX, NULL, GIT_ATTR_FILE)) < 0) + return error; + + return error; +} + int git_attr_add_macro( git_repository *repo, const char *name, @@ -226,8 +294,8 @@ int git_attr_add_macro( git_attr_rule *macro = NULL; git_pool *pool; - if (git_attr_cache__init(repo) < 0) - return -1; + if ((error = attr_setup(repo)) < 0) + return error; macro = git__calloc(1, sizeof(git_attr_rule)); GITERR_CHECK_ALLOC(macro); @@ -348,7 +416,7 @@ static int collect_attr_files( const char *workdir = git_repository_workdir(repo); attr_walk_up_info info = { NULL }; - if ((error = git_attr_cache__init(repo)) < 0) + if ((error = attr_setup(repo)) < 0) return error; /* Resolve path in a non-bare repo */ diff --git a/src/attr_file.c b/src/attr_file.c index 65bbf78e8..8a8d86a2d 100644 --- a/src/attr_file.c +++ b/src/attr_file.c @@ -248,9 +248,7 @@ int git_attr_file__parse_buffer( repo, &attrs->pool, &rule->assigns, &scan))) { if (rule->match.flags & GIT_ATTR_FNMATCH_MACRO) - /* should generate error/warning if this is coming from any - * file other than .gitattributes at repo root. - */ + /* TODO: warning if macro found in file below repo root */ error = git_attr_cache__insert_macro(repo, rule); else error = git_vector_insert(&attrs->rules, rule); diff --git a/tests/attr/repo.c b/tests/attr/repo.c index 71dc7a5b5..9aab7ed96 100644 --- a/tests/attr/repo.c +++ b/tests/attr/repo.c @@ -23,49 +23,74 @@ void test_attr_repo__cleanup(void) g_repo = NULL; } +static struct attr_expected get_one_test_cases[] = { + { "root_test1", "repoattr", EXPECT_TRUE, NULL }, + { "root_test1", "rootattr", EXPECT_TRUE, NULL }, + { "root_test1", "missingattr", EXPECT_UNDEFINED, NULL }, + { "root_test1", "subattr", EXPECT_UNDEFINED, NULL }, + { "root_test1", "negattr", EXPECT_UNDEFINED, NULL }, + { "root_test2", "repoattr", EXPECT_TRUE, NULL }, + { "root_test2", "rootattr", EXPECT_FALSE, NULL }, + { "root_test2", "missingattr", EXPECT_UNDEFINED, NULL }, + { "root_test2", "multiattr", EXPECT_FALSE, NULL }, + { "root_test3", "repoattr", EXPECT_TRUE, NULL }, + { "root_test3", "rootattr", EXPECT_UNDEFINED, NULL }, + { "root_test3", "multiattr", EXPECT_STRING, "3" }, + { "root_test3", "multi2", EXPECT_UNDEFINED, NULL }, + { "sub/subdir_test1", "repoattr", EXPECT_TRUE, NULL }, + { "sub/subdir_test1", "rootattr", EXPECT_TRUE, NULL }, + { "sub/subdir_test1", "missingattr", EXPECT_UNDEFINED, NULL }, + { "sub/subdir_test1", "subattr", EXPECT_STRING, "yes" }, + { "sub/subdir_test1", "negattr", EXPECT_FALSE, NULL }, + { "sub/subdir_test1", "another", EXPECT_UNDEFINED, NULL }, + { "sub/subdir_test2.txt", "repoattr", EXPECT_TRUE, NULL }, + { "sub/subdir_test2.txt", "rootattr", EXPECT_TRUE, NULL }, + { "sub/subdir_test2.txt", "missingattr", EXPECT_UNDEFINED, NULL }, + { "sub/subdir_test2.txt", "subattr", EXPECT_STRING, "yes" }, + { "sub/subdir_test2.txt", "negattr", EXPECT_FALSE, NULL }, + { "sub/subdir_test2.txt", "another", EXPECT_STRING, "zero" }, + { "sub/subdir_test2.txt", "reposub", EXPECT_TRUE, NULL }, + { "sub/sub/subdir.txt", "another", EXPECT_STRING, "one" }, + { "sub/sub/subdir.txt", "reposubsub", EXPECT_TRUE, NULL }, + { "sub/sub/subdir.txt", "reposub", EXPECT_UNDEFINED, NULL }, + { "does-not-exist", "foo", EXPECT_STRING, "yes" }, + { "sub/deep/file", "deepdeep", EXPECT_TRUE, NULL }, + { "sub/sub/d/no", "test", EXPECT_STRING, "a/b/d/*" }, + { "sub/sub/d/yes", "test", EXPECT_UNDEFINED, NULL }, +}; + void test_attr_repo__get_one(void) { - struct attr_expected test_cases[] = { - { "root_test1", "repoattr", EXPECT_TRUE, NULL }, - { "root_test1", "rootattr", EXPECT_TRUE, NULL }, - { "root_test1", "missingattr", EXPECT_UNDEFINED, NULL }, - { "root_test1", "subattr", EXPECT_UNDEFINED, NULL }, - { "root_test1", "negattr", EXPECT_UNDEFINED, NULL }, - { "root_test2", "repoattr", EXPECT_TRUE, NULL }, - { "root_test2", "rootattr", EXPECT_FALSE, NULL }, - { "root_test2", "missingattr", EXPECT_UNDEFINED, NULL }, - { "root_test2", "multiattr", EXPECT_FALSE, NULL }, - { "root_test3", "repoattr", EXPECT_TRUE, NULL }, - { "root_test3", "rootattr", EXPECT_UNDEFINED, NULL }, - { "root_test3", "multiattr", EXPECT_STRING, "3" }, - { "root_test3", "multi2", EXPECT_UNDEFINED, NULL }, - { "sub/subdir_test1", "repoattr", EXPECT_TRUE, NULL }, - { "sub/subdir_test1", "rootattr", EXPECT_TRUE, NULL }, - { "sub/subdir_test1", "missingattr", EXPECT_UNDEFINED, NULL }, - { "sub/subdir_test1", "subattr", EXPECT_STRING, "yes" }, - { "sub/subdir_test1", "negattr", EXPECT_FALSE, NULL }, - { "sub/subdir_test1", "another", EXPECT_UNDEFINED, NULL }, - { "sub/subdir_test2.txt", "repoattr", EXPECT_TRUE, NULL }, - { "sub/subdir_test2.txt", "rootattr", EXPECT_TRUE, NULL }, - { "sub/subdir_test2.txt", "missingattr", EXPECT_UNDEFINED, NULL }, - { "sub/subdir_test2.txt", "subattr", EXPECT_STRING, "yes" }, - { "sub/subdir_test2.txt", "negattr", EXPECT_FALSE, NULL }, - { "sub/subdir_test2.txt", "another", EXPECT_STRING, "zero" }, - { "sub/subdir_test2.txt", "reposub", EXPECT_TRUE, NULL }, - { "sub/sub/subdir.txt", "another", EXPECT_STRING, "one" }, - { "sub/sub/subdir.txt", "reposubsub", EXPECT_TRUE, NULL }, - { "sub/sub/subdir.txt", "reposub", EXPECT_UNDEFINED, NULL }, - { "does-not-exist", "foo", EXPECT_STRING, "yes" }, - { "sub/deep/file", "deepdeep", EXPECT_TRUE, NULL }, - { "sub/sub/d/no", "test", EXPECT_STRING, "a/b/d/*" }, - { "sub/sub/d/yes", "test", EXPECT_UNDEFINED, NULL }, - { NULL, NULL, 0, NULL } - }, *scan; + int i; - for (scan = test_cases; scan->path != NULL; scan++) { + for (i = 0; i < (int)ARRAY_SIZE(get_one_test_cases); ++i) { + struct attr_expected *scan = &get_one_test_cases[i]; const char *value; + cl_git_pass(git_attr_get(&value, g_repo, 0, scan->path, scan->attr)); - attr_check_expected(scan->expected, scan->expected_str, scan->attr, value); + attr_check_expected( + scan->expected, scan->expected_str, scan->attr, value); + } + + cl_assert(git_attr_cache__is_cached( + g_repo, GIT_ATTR_FILE__FROM_FILE, ".git/info/attributes")); + cl_assert(git_attr_cache__is_cached( + g_repo, GIT_ATTR_FILE__FROM_FILE, ".gitattributes")); + cl_assert(git_attr_cache__is_cached( + g_repo, GIT_ATTR_FILE__FROM_FILE, "sub/.gitattributes")); +} + +void test_attr_repo__get_one_start_deep(void) +{ + int i; + + for (i = (int)ARRAY_SIZE(get_one_test_cases) - 1; i >= 0; --i) { + struct attr_expected *scan = &get_one_test_cases[i]; + const char *value; + + cl_git_pass(git_attr_get(&value, g_repo, 0, scan->path, scan->attr)); + attr_check_expected( + scan->expected, scan->expected_str, scan->attr, value); } cl_assert(git_attr_cache__is_cached( From 916fcbd61754f74b350ca689e27563cdbded2d30 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 18 Apr 2014 14:42:40 -0700 Subject: [PATCH 4/5] Fix ignore difference from git with trailing /* Ignore patterns that ended with a trailing '/*' were still needing to match against another actual '/' character in the full path. This is not the same behavior as core Git. Instead, we strip a trailing '/*' off of any patterns that were matching and just take it to imply the FNM_LEADING_DIR behavior. --- src/attr_file.c | 9 +++++++++ src/attr_file.h | 1 + tests/status/ignore.c | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/src/attr_file.c b/src/attr_file.c index 8a8d86a2d..57b4da7dd 100644 --- a/src/attr_file.c +++ b/src/attr_file.c @@ -353,6 +353,8 @@ bool git_attr_fnmatch__match( if (match->flags & GIT_ATTR_FNMATCH_ICASE) flags |= FNM_CASEFOLD; + if (match->flags & GIT_ATTR_FNMATCH_LEADINGDIR) + flags |= FNM_LEADING_DIR; if (match->flags & GIT_ATTR_FNMATCH_FULLPATH) { filename = path->path; @@ -543,6 +545,13 @@ int git_attr_fnmatch__parse( if (--slash_count <= 0) spec->flags = spec->flags & ~GIT_ATTR_FNMATCH_FULLPATH; } + if (spec->length >= 2 && + pattern[spec->length - 1] == '*' && + pattern[spec->length - 2] == '/') { + spec->length -= 2; + spec->flags = spec->flags | GIT_ATTR_FNMATCH_LEADINGDIR; + /* leave FULLPATH match on, however */ + } if ((spec->flags & GIT_ATTR_FNMATCH_FULLPATH) != 0 && context != NULL && git_path_root(pattern) < 0) diff --git a/src/attr_file.h b/src/attr_file.h index c906be44d..09afa5bd4 100644 --- a/src/attr_file.h +++ b/src/attr_file.h @@ -30,6 +30,7 @@ #define GIT_ATTR_FNMATCH_MATCH_ALL (1U << 8) #define GIT_ATTR_FNMATCH_ALLOWNEG (1U << 9) #define GIT_ATTR_FNMATCH_ALLOWMACRO (1U << 10) +#define GIT_ATTR_FNMATCH_LEADINGDIR (1U << 11) #define GIT_ATTR_FNMATCH__INCOMING \ (GIT_ATTR_FNMATCH_ALLOWSPACE | \ diff --git a/tests/status/ignore.c b/tests/status/ignore.c index 15c85e262..d88b2eb6b 100644 --- a/tests/status/ignore.c +++ b/tests/status/ignore.c @@ -456,6 +456,24 @@ void test_status_ignore__contained_dir_with_matching_name(void) cl_assert_equal_i(0, counts.wrong_sorted_path); } +void test_status_ignore__trailing_slash_star(void) +{ + static const char *test_files[] = { + "empty_standard_repo/file", + "empty_standard_repo/subdir/file", + "empty_standard_repo/subdir/sub2/sub3/file", + NULL + }; + + make_test_data("empty_standard_repo", test_files); + cl_git_mkfile( + "empty_standard_repo/subdir/.gitignore", "/**/*\n"); + + refute_is_ignored("file"); + assert_is_ignored("subdir/sub2/sub3/file"); + assert_is_ignored("subdir/file"); +} + void test_status_ignore__adding_internal_ignores(void) { g_repo = cl_git_sandbox_init("empty_standard_repo"); From ac16bd0a94e1f7254112c7585b843bdc2d0659c1 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 18 Apr 2014 15:45:59 -0700 Subject: [PATCH 5/5] Minor fixes Only apply LEADING_DIR pattern munging to patterns in ignore and attribute files, not to pathspecs used to select files to operate on. Also, allow internal macro definitions to be evaluated before loading all external ones (important so that external ones can make use of internal `binary` definition). --- src/attr.c | 2 +- src/attr_file.c | 3 ++- src/attr_file.h | 5 +++-- src/pathspec.c | 3 ++- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/attr.c b/src/attr.c index 6b9a3d614..05b0c1b3c 100644 --- a/src/attr.c +++ b/src/attr.c @@ -294,7 +294,7 @@ int git_attr_add_macro( git_attr_rule *macro = NULL; git_pool *pool; - if ((error = attr_setup(repo)) < 0) + if ((error = git_attr_cache__init(repo)) < 0) return error; macro = git__calloc(1, sizeof(git_attr_rule)); diff --git a/src/attr_file.c b/src/attr_file.c index 57b4da7dd..d107b5ab0 100644 --- a/src/attr_file.c +++ b/src/attr_file.c @@ -545,7 +545,8 @@ int git_attr_fnmatch__parse( if (--slash_count <= 0) spec->flags = spec->flags & ~GIT_ATTR_FNMATCH_FULLPATH; } - if (spec->length >= 2 && + if ((spec->flags & GIT_ATTR_FNMATCH_NOLEADINGDIR) == 0 && + spec->length >= 2 && pattern[spec->length - 1] == '*' && pattern[spec->length - 2] == '/') { spec->length -= 2; diff --git a/src/attr_file.h b/src/attr_file.h index 09afa5bd4..e50aec07c 100644 --- a/src/attr_file.h +++ b/src/attr_file.h @@ -31,10 +31,11 @@ #define GIT_ATTR_FNMATCH_ALLOWNEG (1U << 9) #define GIT_ATTR_FNMATCH_ALLOWMACRO (1U << 10) #define GIT_ATTR_FNMATCH_LEADINGDIR (1U << 11) +#define GIT_ATTR_FNMATCH_NOLEADINGDIR (1U << 12) #define GIT_ATTR_FNMATCH__INCOMING \ - (GIT_ATTR_FNMATCH_ALLOWSPACE | \ - GIT_ATTR_FNMATCH_ALLOWNEG | GIT_ATTR_FNMATCH_ALLOWMACRO) + (GIT_ATTR_FNMATCH_ALLOWSPACE | GIT_ATTR_FNMATCH_ALLOWNEG | \ + GIT_ATTR_FNMATCH_ALLOWMACRO | GIT_ATTR_FNMATCH_NOLEADINGDIR) typedef enum { GIT_ATTR_FILE__IN_MEMORY = 0, diff --git a/src/pathspec.c b/src/pathspec.c index 09650de7c..a01d74f07 100644 --- a/src/pathspec.c +++ b/src/pathspec.c @@ -83,7 +83,8 @@ int git_pathspec__vinit( if (!match) return -1; - match->flags = GIT_ATTR_FNMATCH_ALLOWSPACE | GIT_ATTR_FNMATCH_ALLOWNEG; + match->flags = GIT_ATTR_FNMATCH_ALLOWSPACE | + GIT_ATTR_FNMATCH_ALLOWNEG | GIT_ATTR_FNMATCH_NOLEADINGDIR; ret = git_attr_fnmatch__parse(match, strpool, NULL, &pattern); if (ret == GIT_ENOTFOUND) {