From cfeef7ce2ca7747a19d3caffa49cd1fda8af5730 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 19 Nov 2012 13:40:08 -0800 Subject: [PATCH 1/3] Minor optimization to tree entry validity check This checks for a leading '.' before looking for the invalid tree entry names. Even on pretty high levels of optimization, this seems to make a measurable improvement. I accidentally used && in the check initially instead of || and while debugging ended up improving the error reporting of issues with adding tree entries. I thought I'd leave those changes, too. --- src/tree.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/tree.c b/src/tree.c index 6f9838880..fdc4a07be 100644 --- a/src/tree.c +++ b/src/tree.c @@ -26,9 +26,12 @@ static bool valid_filemode(const int filemode) static int valid_entry_name(const char *filename) { - return *filename != '\0' && strchr(filename, '/') == NULL && - strcmp(filename, "..") != 0 && strcmp(filename, ".") != 0 && - strcmp(filename, ".git") != 0; + return *filename != '\0' && + strchr(filename, '/') == NULL && + (*filename != '.' || + (strcmp(filename, ".") != 0 && + strcmp(filename, "..") != 0 && + strcmp(filename, DOT_GIT) != 0)); } static int entry_sort_cmp(const void *a, const void *b) @@ -294,9 +297,12 @@ unsigned int git_tree_entrycount(const git_tree *tree) return (unsigned int)tree->entries.length; } -static int tree_error(const char *str) +static int tree_error(const char *str, const char *path) { - giterr_set(GITERR_TREE, "%s", str); + if (path) + giterr_set(GITERR_TREE, "%s - %s", str, path); + else + giterr_set(GITERR_TREE, "%s", str); return -1; } @@ -311,13 +317,13 @@ static int tree_parse_buffer(git_tree *tree, const char *buffer, const char *buf if (git__strtol32(&attr, buffer, &buffer, 8) < 0 || !buffer || !valid_filemode(attr)) - return tree_error("Failed to parse tree. Can't parse filemode"); + return tree_error("Failed to parse tree. Can't parse filemode", NULL); if (*buffer++ != ' ') - return tree_error("Failed to parse tree. Object is corrupted"); + return tree_error("Failed to parse tree. Object is corrupted", NULL); if (memchr(buffer, 0, buffer_end - buffer) == NULL) - return tree_error("Failed to parse tree. Object is corrupted"); + return tree_error("Failed to parse tree. Object is corrupted", NULL); /** Allocate the entry and store it in the entries vector */ { @@ -375,7 +381,7 @@ static int append_entry( git_tree_entry *entry; if (!valid_entry_name(filename)) - return tree_error("Failed to insert entry. Invalid name for a tree entry"); + return tree_error("Failed to insert entry. Invalid name for a tree entry", filename); entry = alloc_entry(filename); GITERR_CHECK_ALLOC(entry); @@ -452,7 +458,8 @@ static int write_tree( /* Write out the subtree */ written = write_tree(&sub_oid, repo, index, subdir, i); if (written < 0) { - tree_error("Failed to write subtree"); + tree_error("Failed to write subtree", subdir); + git__free(subdir); goto on_error; } else { i = written - 1; /* -1 because of the loop increment */ @@ -470,18 +477,15 @@ static int write_tree( } else { last_comp = subdir; } + error = append_entry(bld, last_comp, &sub_oid, S_IFDIR); git__free(subdir); - if (error < 0) { - tree_error("Failed to insert dir"); + if (error < 0) goto on_error; - } } else { error = append_entry(bld, filename, &entry->oid, entry->mode); - if (error < 0) { - tree_error("Failed to insert file"); + if (error < 0) goto on_error; - } } } @@ -585,12 +589,12 @@ int git_treebuilder_insert( assert(bld && id && filename); if (!valid_filemode(filemode)) - return tree_error("Failed to insert entry. Invalid filemode"); + return tree_error("Failed to insert entry. Invalid filemode for file", filename); filemode = normalize_filemode(filemode); if (!valid_entry_name(filename)) - return tree_error("Failed to insert entry. Invalid name for a tree entry"); + return tree_error("Failed to insert entry. Invalid name for a tree entry", filename); pos = tree_key_search(&bld->entries, filename, strlen(filename)); @@ -646,7 +650,7 @@ int git_treebuilder_remove(git_treebuilder *bld, const char *filename) git_tree_entry *remove_ptr = treebuilder_get(bld, filename); if (remove_ptr == NULL || remove_ptr->removed) - return tree_error("Failed to remove entry. File isn't in the tree"); + return tree_error("Failed to remove entry. File isn't in the tree", filename); remove_ptr->removed = 1; return 0; From 02df42ddbf87820011aa4ccba9adfde52670e5e2 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 19 Nov 2012 16:33:30 -0800 Subject: [PATCH 2/3] Set up default internal ignores This adds "." ".." and ".git" to the internal ignores list by default - asking about paths with these files will always say that they are ignored. --- include/git2/ignore.h | 7 ++++--- src/ignore.c | 34 +++++++++++++++++++++------------- tests-clar/status/ignore.c | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 16 deletions(-) diff --git a/include/git2/ignore.h b/include/git2/ignore.h index e18615edd..592c96e65 100644 --- a/include/git2/ignore.h +++ b/include/git2/ignore.h @@ -41,9 +41,10 @@ GIT_EXTERN(int) git_ignore_add_rule( /** * Clear ignore rules that were explicitly added. * - * Clears the internal ignore rules that have been set up. This will not - * turn off the rules in .gitignore files that actually exist in the - * filesystem. + * Resets to the default internal ignore rules. This will not turn off + * rules in .gitignore files that actually exist in the filesystem. + * + * The default internal ignores ignore ".", ".." and ".git" entries. * * @param repo The repository to remove ignore rules from. * @return 0 on success diff --git a/src/ignore.c b/src/ignore.c index 6a377e60d..5edc5b65b 100644 --- a/src/ignore.c +++ b/src/ignore.c @@ -7,6 +7,8 @@ #define GIT_IGNORE_FILE_INREPO "info/exclude" #define GIT_IGNORE_FILE ".gitignore" +#define GIT_IGNORE_DEFAULT_RULES ".\n..\n.git\n" + static int parse_ignore_file( git_repository *repo, void *parsedata, const char *buffer, git_attr_file *ignores) { @@ -88,6 +90,19 @@ static int push_one_ignore(void *ref, git_buf *path) return push_ignore_file(ign->repo, ign, &ign->ign_path, path->ptr, GIT_IGNORE_FILE); } +static int get_internal_ignores(git_attr_file **ign, git_repository *repo) +{ + int error; + + if (!(error = git_attr_cache__init(repo))) + error = git_attr_cache__internal_file(repo, GIT_IGNORE_INTERNAL, ign); + + if (!error && !(*ign)->rules.length) + error = parse_ignore_file(repo, NULL, GIT_IGNORE_DEFAULT_RULES, *ign); + + return error; +} + int git_ignore__for_path( git_repository *repo, const char *path, @@ -129,8 +144,7 @@ int git_ignore__for_path( goto cleanup; /* set up internals */ - error = git_attr_cache__internal_file( - repo, GIT_IGNORE_INTERNAL, &ignores->ign_internal); + error = get_internal_ignores(&ignores->ign_internal, repo); if (error < 0) goto cleanup; @@ -239,16 +253,6 @@ cleanup: return 0; } -static int get_internal_ignores(git_attr_file **ign, git_repository *repo) -{ - int error; - - if (!(error = git_attr_cache__init(repo))) - error = git_attr_cache__internal_file(repo, GIT_IGNORE_INTERNAL, ign); - - return error; -} - int git_ignore_add_rule( git_repository *repo, const char *rules) @@ -268,9 +272,13 @@ int git_ignore_clear_internal_rules( int error; git_attr_file *ign_internal; - if (!(error = get_internal_ignores(&ign_internal, repo))) + if (!(error = get_internal_ignores(&ign_internal, repo))) { git_attr_file__clear_rules(ign_internal); + return parse_ignore_file( + repo, NULL, GIT_IGNORE_DEFAULT_RULES, ign_internal); + } + return error; } diff --git a/tests-clar/status/ignore.c b/tests-clar/status/ignore.c index ddd0b3d6e..27f9d85b9 100644 --- a/tests-clar/status/ignore.c +++ b/tests-clar/status/ignore.c @@ -341,3 +341,41 @@ void test_status_ignore__internal_ignores_inside_deep_paths(void) cl_git_pass(git_status_should_ignore(&ignored, g_repo, "xthis/is/deep")); cl_assert(!ignored); } + +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); + + 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); + + 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); +} From d46b0a04c7733e430a50a9d2df195bec87d5dae6 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 19 Nov 2012 16:34:44 -0800 Subject: [PATCH 3/3] Improve iterator ignoring .git file The workdir iterator has always tried to ignore .git files, but it turns out there were some bugs. This makes it more robust at ignoring .git files. This also makes iterators always check ".git" case insensitively regardless of the properties of the system. This will make libgit2 skip ".GIT" and the like. This is different from core git, but on systems with case insensitive but case preserving file systems, allowing ".GIT" to be added is problematic. --- src/iterator.c | 27 +++++++++++++++--- tests-clar/diff/iterator.c | 56 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/iterator.c b/src/iterator.c index ee83a4fda..75985355f 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -10,6 +10,7 @@ #include "ignore.h" #include "buffer.h" #include "git2/submodule.h" +#include #define ITERATOR_BASE_INIT(P,NAME_LC,NAME_UC) do { \ (P) = git__calloc(1, sizeof(NAME_LC ## _iterator)); \ @@ -465,6 +466,23 @@ static int git_path_with_stat_cmp_icase(const void *a, const void *b) return strcasecmp(path_with_stat_a->path, path_with_stat_b->path); } +GIT_INLINE(bool) path_is_dotgit(const git_path_with_stat *ps) +{ + if (!ps) + return false; + else { + const char *path = ps->path; + size_t len = ps->path_len; + + return len >= 4 && + tolower(path[len - 1]) == 't' && + tolower(path[len - 2]) == 'i' && + tolower(path[len - 3]) == 'g' && + path[len - 4] == '.' && + (len == 4 || path[len - 5] == '/'); + } +} + static workdir_iterator_frame *workdir_iterator__alloc_frame(workdir_iterator *wi) { workdir_iterator_frame *wf = git__calloc(1, sizeof(workdir_iterator_frame)); @@ -531,6 +549,9 @@ static int workdir_iterator__expand_dir(workdir_iterator *wi) CASESELECT(wi->base.ignore_case, workdir_iterator__entry_cmp_icase, workdir_iterator__entry_cmp_case), wf->start); + if (path_is_dotgit(git_vector_get(&wf->entries, wf->index))) + wf->index++; + wf->next = wi->stack; wi->stack = wf; @@ -574,8 +595,7 @@ static int workdir_iterator__advance( next = git_vector_get(&wf->entries, ++wf->index); if (next != NULL) { /* match git's behavior of ignoring anything named ".git" */ - if (STRCMP_CASESELECT(wi->base.ignore_case, next->path, DOT_GIT "/") == 0 || - STRCMP_CASESELECT(wi->base.ignore_case, next->path, DOT_GIT) == 0) + if (path_is_dotgit(next)) continue; /* else found a good entry */ break; @@ -658,8 +678,7 @@ static int workdir_iterator__update_entry(workdir_iterator *wi) wi->entry.path = ps->path; /* skip over .git entries */ - if (STRCMP_CASESELECT(wi->base.ignore_case, ps->path, DOT_GIT "/") == 0 || - STRCMP_CASESELECT(wi->base.ignore_case, ps->path, DOT_GIT) == 0) + if (path_is_dotgit(ps)) return workdir_iterator__advance((git_iterator *)wi, NULL); wi->is_ignored = -1; diff --git a/tests-clar/diff/iterator.c b/tests-clar/diff/iterator.c index 368903200..1d8396099 100644 --- a/tests-clar/diff/iterator.c +++ b/tests-clar/diff/iterator.c @@ -668,3 +668,59 @@ void test_diff_iterator__workdir_1_ranged_empty_2(void) "status", NULL, "aaaa_empty_before", 0, 0, NULL, NULL); } + +void test_diff_iterator__workdir_builtin_ignores(void) +{ + git_repository *repo = cl_git_sandbox_init("attr"); + git_iterator *i; + const git_index_entry *entry; + int idx; + static struct { + const char *path; + bool ignored; + } expected[] = { + { "dir/", true }, + { "file", false }, + { "ign", true }, + { "macro_bad", false }, + { "macro_test", false }, + { "root_test1", false }, + { "root_test2", false }, + { "root_test3", false }, + { "root_test4.txt", false }, + { "sub/", false }, + { "sub/.gitattributes", false }, + { "sub/abc", false }, + { "sub/dir/", true }, + { "sub/file", false }, + { "sub/ign/", true }, + { "sub/sub/", false }, + { "sub/sub/.gitattributes", false }, + { "sub/sub/dir", false }, /* file is not actually a dir */ + { "sub/sub/file", false }, + { NULL, false } + }; + + cl_git_pass(p_mkdir("attr/sub/sub/.git", 0777)); + cl_git_mkfile("attr/sub/.git", "whatever"); + + cl_git_pass( + git_iterator_for_workdir_range(&i, repo, "dir", "sub/sub/file")); + cl_git_pass(git_iterator_current(i, &entry)); + + for (idx = 0; entry != NULL; ++idx) { + int ignored = git_iterator_current_is_ignored(i); + + cl_assert_equal_s(expected[idx].path, entry->path); + cl_assert_(ignored == expected[idx].ignored, expected[idx].path); + + if (!ignored && S_ISDIR(entry->mode)) + cl_git_pass(git_iterator_advance_into_directory(i, &entry)); + else + cl_git_pass(git_iterator_advance(i, &entry)); + } + + cl_assert(expected[idx].path == NULL); + + git_iterator_free(i); +}