From 65025cb8934a289460bc64f82c27027c68a85be6 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 18 Mar 2013 17:24:13 -0700 Subject: [PATCH] Three submodule status bug fixes 1. Fix sort order problem with submodules where "mod" was sorting after "mod-plus" because they were being sorted as "mod/" and "mod-plus/". This involved pushing the "contains a .git entry" test significantly lower in the stack. 2. Reinstate behavior that a directory which contains a .git entry will be treated as a submodule during iteration even if it is not yet added to the .gitmodules. 3. Now that any directory containing .git is reported as submodule, we have to be more careful checking for GIT_EEXISTS when we do a submodule lookup, because that is the error code that is returned by git_submodule_lookup when you try to look up a directory containing .git that has no record in gitmodules or the index. --- src/diff.c | 10 +- src/diff_output.c | 5 + src/iterator.c | 4 +- src/path.c | 15 ++- tests-clar/diff/iterator.c | 12 ++- tests-clar/diff/workdir.c | 14 +-- .../.gitted/HEAD | 0 .../.gitted/config | 0 .../.gitted/description | 0 .../.gitted/index | Bin .../.gitted/info/exclude | 0 .../.gitted/logs/HEAD | 0 .../.gitted/logs/refs/heads/master | 0 .../68/e92c611b80ee1ed8f38314ff9577f0d15b2444 | Bin .../71/ff9927d7c8a5639e062c38a7d35c433c424627 | Bin .../f0/1d56b18efd353ef2bb93a4585d590a0847195e | Bin .../.gitted/refs/heads/master | 0 .../README.txt | 0 .../resources/submod2/not/.gitted/notempty | 1 + tests-clar/resources/submod2/not/README.txt | 1 + .../not_submodule/.gitted/COMMIT_EDITMSG | 1 - tests-clar/submodule/lookup.c | 4 +- tests-clar/submodule/modify.c | 2 +- tests-clar/submodule/status.c | 88 +++++++++++++++++- 24 files changed, 130 insertions(+), 27 deletions(-) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/HEAD (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/config (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/description (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/index (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/info/exclude (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/logs/HEAD (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/logs/refs/heads/master (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/objects/68/e92c611b80ee1ed8f38314ff9577f0d15b2444 (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/objects/71/ff9927d7c8a5639e062c38a7d35c433c424627 (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/objects/f0/1d56b18efd353ef2bb93a4585d590a0847195e (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/.gitted/refs/heads/master (100%) rename tests-clar/resources/submod2/{not_submodule => not-submodule}/README.txt (100%) create mode 100644 tests-clar/resources/submod2/not/.gitted/notempty create mode 100644 tests-clar/resources/submod2/not/README.txt delete mode 100644 tests-clar/resources/submod2/not_submodule/.gitted/COMMIT_EDITMSG diff --git a/src/diff.c b/src/diff.c index fc37d139d..fb69f8920 100644 --- a/src/diff.c +++ b/src/diff.c @@ -512,13 +512,17 @@ static int maybe_modified( status = GIT_DELTA_UNMODIFIED; else if (S_ISGITLINK(nmode)) { + int err; git_submodule *sub; if ((diff->opts.flags & GIT_DIFF_IGNORE_SUBMODULES) != 0) status = GIT_DELTA_UNMODIFIED; - else if (git_submodule_lookup(&sub, diff->repo, nitem->path) < 0) - return -1; - else if (git_submodule_ignore(sub) == GIT_SUBMODULE_IGNORE_ALL) + else if ((err = git_submodule_lookup(&sub, diff->repo, nitem->path)) < 0) { + if (err == GIT_EEXISTS) + status = GIT_DELTA_UNMODIFIED; + else + return err; + } else if (git_submodule_ignore(sub) == GIT_SUBMODULE_IGNORE_ALL) status = GIT_DELTA_UNMODIFIED; else { unsigned int sm_status = 0; diff --git a/src/diff_output.c b/src/diff_output.c index b938cc06d..fba6129b7 100644 --- a/src/diff_output.c +++ b/src/diff_output.c @@ -299,7 +299,12 @@ static int get_workdir_sm_content( if ((error = git_submodule_lookup(&sm, ctxt->repo, file->path)) < 0 || (error = git_submodule_status(&sm_status, sm)) < 0) + { + /* GIT_EEXISTS means a "submodule" that has not been git added */ + if (error == GIT_EEXISTS) + error = 0; return error; + } /* update OID if we didn't have it previously */ if ((file->flags & GIT_DIFF_FLAG_VALID_OID) == 0) { diff --git a/src/iterator.c b/src/iterator.c index b15bcedd8..805a3c987 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -1150,11 +1150,13 @@ static int workdir_iterator__update_entry(workdir_iterator *wi) return 0; /* detect submodules */ - error = git_submodule_lookup(NULL, wi->base.repo, wi->entry.path); if (error == GIT_ENOTFOUND) giterr_clear(); + if (error == GIT_EEXISTS) /* if contains .git, treat as untracked submod */ + error = 0; + /* if submodule, mark as GITLINK and remove trailing slash */ if (!error) { size_t len = strlen(wi->entry.path); diff --git a/src/path.c b/src/path.c index 5767faeed..6437979d5 100644 --- a/src/path.c +++ b/src/path.c @@ -877,15 +877,22 @@ int git_path_dirload_with_stat( if (cmp_len && strncomp(ps->path, end_stat, cmp_len) > 0) continue; + git_buf_truncate(&full, prefix_len); + if ((error = git_buf_joinpath(&full, full.ptr, ps->path)) < 0 || (error = git_path_lstat(full.ptr, &ps->st)) < 0) break; - git_buf_truncate(&full, prefix_len); - if (S_ISDIR(ps->st.st_mode)) { - ps->path[ps->path_len++] = '/'; - ps->path[ps->path_len] = '\0'; + if ((error = git_buf_joinpath(&full, full.ptr, ".git")) < 0) + break; + + if (p_access(full.ptr, F_OK) == 0) { + ps->st.st_mode = GIT_FILEMODE_COMMIT; + } else { + ps->path[ps->path_len++] = '/'; + ps->path[ps->path_len] = '\0'; + } } } diff --git a/tests-clar/diff/iterator.c b/tests-clar/diff/iterator.c index f1efdfbba..15b10465a 100644 --- a/tests-clar/diff/iterator.c +++ b/tests-clar/diff/iterator.c @@ -720,13 +720,13 @@ void test_diff_iterator__workdir_builtin_ignores(void) { "root_test2", false }, { "root_test3", false }, { "root_test4.txt", false }, - { "sub/", false }, + { "sub", false }, { "sub/.gitattributes", false }, { "sub/abc", false }, { "sub/dir/", true }, { "sub/file", false }, { "sub/ign/", true }, - { "sub/sub/", false }, + { "sub/sub", false }, { "sub/sub/.gitattributes", false }, { "sub/sub/dir", false }, /* file is not actually a dir */ { "sub/sub/file", false }, @@ -746,9 +746,13 @@ void test_diff_iterator__workdir_builtin_ignores(void) cl_assert_equal_s(expected[idx].path, entry->path); cl_assert_(ignored == expected[idx].ignored, expected[idx].path); - if (!ignored && S_ISDIR(entry->mode)) + if (!ignored && + (entry->mode == GIT_FILEMODE_TREE || + entry->mode == GIT_FILEMODE_COMMIT)) + { + /* it is possible to advance "into" a submodule */ cl_git_pass(git_iterator_advance_into(&entry, i)); - else + } else cl_git_pass(git_iterator_advance(&entry, i)); } diff --git a/tests-clar/diff/workdir.c b/tests-clar/diff/workdir.c index 7e8915c4b..983465b29 100644 --- a/tests-clar/diff/workdir.c +++ b/tests-clar/diff/workdir.c @@ -936,7 +936,8 @@ void test_diff_workdir__submodules(void) p_rename("submod2_target/.gitted", "submod2_target/.git"); rewrite_gitmodules(git_repository_workdir(g_repo)); - p_rename("submod2/not_submodule/.gitted", "submod2/not_submodule/.git"); + p_rename("submod2/not-submodule/.gitted", "submod2/not-submodule/.git"); + p_rename("submod2/not/.gitted", "submod2/not/.git"); cl_fixture_cleanup("submod2_target"); @@ -954,21 +955,22 @@ void test_diff_workdir__submodules(void) /* essentially doing: git diff 873585b94bdeabccea991ea5e3ec1a277895b698 */ memset(&exp, 0, sizeof(exp)); + cl_git_pass(git_diff_foreach( diff, diff_file_cb, diff_hunk_cb, diff_line_cb, &exp)); - /* the following differs from "git diff 873585" by one "untracked" file - * because the diff list includes the "not_submodule/" directory which - * is not displayed in the text diff. + /* the following differs from "git diff 873585" by two "untracked" file + * because the diff list includes the "not" and "not-submodule" dirs which + * are not displayed in the text diff. */ - cl_assert_equal_i(10, exp.files); + cl_assert_equal_i(11, exp.files); cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]); cl_assert_equal_i(0, exp.file_status[GIT_DELTA_DELETED]); cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]); cl_assert_equal_i(0, exp.file_status[GIT_DELTA_IGNORED]); - cl_assert_equal_i(9, exp.file_status[GIT_DELTA_UNTRACKED]); + cl_assert_equal_i(10, exp.file_status[GIT_DELTA_UNTRACKED]); /* the following numbers match "git diff 873585" exactly */ diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/HEAD b/tests-clar/resources/submod2/not-submodule/.gitted/HEAD similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/HEAD rename to tests-clar/resources/submod2/not-submodule/.gitted/HEAD diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/config b/tests-clar/resources/submod2/not-submodule/.gitted/config similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/config rename to tests-clar/resources/submod2/not-submodule/.gitted/config diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/description b/tests-clar/resources/submod2/not-submodule/.gitted/description similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/description rename to tests-clar/resources/submod2/not-submodule/.gitted/description diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/index b/tests-clar/resources/submod2/not-submodule/.gitted/index similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/index rename to tests-clar/resources/submod2/not-submodule/.gitted/index diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/info/exclude b/tests-clar/resources/submod2/not-submodule/.gitted/info/exclude similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/info/exclude rename to tests-clar/resources/submod2/not-submodule/.gitted/info/exclude diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/logs/HEAD b/tests-clar/resources/submod2/not-submodule/.gitted/logs/HEAD similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/logs/HEAD rename to tests-clar/resources/submod2/not-submodule/.gitted/logs/HEAD diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/logs/refs/heads/master b/tests-clar/resources/submod2/not-submodule/.gitted/logs/refs/heads/master similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/logs/refs/heads/master rename to tests-clar/resources/submod2/not-submodule/.gitted/logs/refs/heads/master diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/objects/68/e92c611b80ee1ed8f38314ff9577f0d15b2444 b/tests-clar/resources/submod2/not-submodule/.gitted/objects/68/e92c611b80ee1ed8f38314ff9577f0d15b2444 similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/objects/68/e92c611b80ee1ed8f38314ff9577f0d15b2444 rename to tests-clar/resources/submod2/not-submodule/.gitted/objects/68/e92c611b80ee1ed8f38314ff9577f0d15b2444 diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/objects/71/ff9927d7c8a5639e062c38a7d35c433c424627 b/tests-clar/resources/submod2/not-submodule/.gitted/objects/71/ff9927d7c8a5639e062c38a7d35c433c424627 similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/objects/71/ff9927d7c8a5639e062c38a7d35c433c424627 rename to tests-clar/resources/submod2/not-submodule/.gitted/objects/71/ff9927d7c8a5639e062c38a7d35c433c424627 diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/objects/f0/1d56b18efd353ef2bb93a4585d590a0847195e b/tests-clar/resources/submod2/not-submodule/.gitted/objects/f0/1d56b18efd353ef2bb93a4585d590a0847195e similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/objects/f0/1d56b18efd353ef2bb93a4585d590a0847195e rename to tests-clar/resources/submod2/not-submodule/.gitted/objects/f0/1d56b18efd353ef2bb93a4585d590a0847195e diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/refs/heads/master b/tests-clar/resources/submod2/not-submodule/.gitted/refs/heads/master similarity index 100% rename from tests-clar/resources/submod2/not_submodule/.gitted/refs/heads/master rename to tests-clar/resources/submod2/not-submodule/.gitted/refs/heads/master diff --git a/tests-clar/resources/submod2/not_submodule/README.txt b/tests-clar/resources/submod2/not-submodule/README.txt similarity index 100% rename from tests-clar/resources/submod2/not_submodule/README.txt rename to tests-clar/resources/submod2/not-submodule/README.txt diff --git a/tests-clar/resources/submod2/not/.gitted/notempty b/tests-clar/resources/submod2/not/.gitted/notempty new file mode 100644 index 000000000..9b33ac4e4 --- /dev/null +++ b/tests-clar/resources/submod2/not/.gitted/notempty @@ -0,0 +1 @@ +fooled you diff --git a/tests-clar/resources/submod2/not/README.txt b/tests-clar/resources/submod2/not/README.txt new file mode 100644 index 000000000..4f6935b98 --- /dev/null +++ b/tests-clar/resources/submod2/not/README.txt @@ -0,0 +1 @@ +what am I really diff --git a/tests-clar/resources/submod2/not_submodule/.gitted/COMMIT_EDITMSG b/tests-clar/resources/submod2/not_submodule/.gitted/COMMIT_EDITMSG deleted file mode 100644 index 5852f4463..000000000 --- a/tests-clar/resources/submod2/not_submodule/.gitted/COMMIT_EDITMSG +++ /dev/null @@ -1 +0,0 @@ -Initial commit diff --git a/tests-clar/submodule/lookup.c b/tests-clar/submodule/lookup.c index 868b51e55..acf8f6462 100644 --- a/tests-clar/submodule/lookup.c +++ b/tests-clar/submodule/lookup.c @@ -13,7 +13,7 @@ void test_submodule_lookup__initialize(void) /* must create submod2_target before rewrite so prettify will work */ rewrite_gitmodules(git_repository_workdir(g_repo)); - p_rename("submod2/not_submodule/.gitted", "submod2/not_submodule/.git"); + p_rename("submod2/not-submodule/.gitted", "submod2/not-submodule/.git"); } void test_submodule_lookup__cleanup(void) @@ -39,7 +39,7 @@ void test_submodule_lookup__simple_lookup(void) cl_assert(sm); /* lookup git repo subdir that is not added as submodule */ - cl_assert(git_submodule_lookup(&sm, g_repo, "not_submodule") == GIT_EEXISTS); + cl_assert(git_submodule_lookup(&sm, g_repo, "not-submodule") == GIT_EEXISTS); /* lookup existing directory that is not a submodule */ cl_assert(git_submodule_lookup(&sm, g_repo, "just_a_dir") == GIT_ENOTFOUND); diff --git a/tests-clar/submodule/modify.c b/tests-clar/submodule/modify.c index f6d41fdf2..94eb3738a 100644 --- a/tests-clar/submodule/modify.c +++ b/tests-clar/submodule/modify.c @@ -18,7 +18,7 @@ void test_submodule_modify__initialize(void) /* must create submod2_target before rewrite so prettify will work */ rewrite_gitmodules(git_repository_workdir(g_repo)); - p_rename("submod2/not_submodule/.gitted", "submod2/not_submodule/.git"); + p_rename("submod2/not-submodule/.gitted", "submod2/not-submodule/.git"); } void test_submodule_modify__cleanup(void) diff --git a/tests-clar/submodule/status.c b/tests-clar/submodule/status.c index 3fd6960c9..282e82758 100644 --- a/tests-clar/submodule/status.c +++ b/tests-clar/submodule/status.c @@ -3,6 +3,7 @@ #include "path.h" #include "submodule_helpers.h" #include "fileops.h" +#include "iterator.h" static git_repository *g_repo = NULL; @@ -15,7 +16,8 @@ void test_submodule_status__initialize(void) /* must create submod2_target before rewrite so prettify will work */ rewrite_gitmodules(git_repository_workdir(g_repo)); - p_rename("submod2/not_submodule/.gitted", "submod2/not_submodule/.git"); + p_rename("submod2/not-submodule/.gitted", "submod2/not-submodule/.git"); + p_rename("submod2/not/.gitted", "submod2/not/.git"); } void test_submodule_status__cleanup(void) @@ -52,7 +54,12 @@ void test_submodule_status__ignore_none(void) cl_git_pass(git_buf_joinpath(&path, git_repository_workdir(g_repo), "sm_unchanged")); cl_git_pass(git_futils_rmdir_r(git_buf_cstr(&path), NULL, GIT_RMDIR_REMOVE_FILES)); - cl_git_fail(git_submodule_lookup(&sm, g_repo, "not_submodule")); + cl_assert_equal_i(GIT_ENOTFOUND, + git_submodule_lookup(&sm, g_repo, "just_a_dir")); + cl_assert_equal_i(GIT_EEXISTS, + git_submodule_lookup(&sm, g_repo, "not-submodule")); + cl_assert_equal_i(GIT_EEXISTS, + git_submodule_lookup(&sm, g_repo, "not")); cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_changed_index")); cl_git_pass(git_submodule_status(&status, sm)); @@ -138,7 +145,7 @@ void test_submodule_status__ignore_untracked(void) cl_git_pass(git_submodule_foreach(g_repo, set_sm_ignore, &ign)); - cl_git_fail(git_submodule_lookup(&sm, g_repo, "not_submodule")); + cl_git_fail(git_submodule_lookup(&sm, g_repo, "not-submodule")); cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_changed_index")); cl_git_pass(git_submodule_status(&status, sm)); @@ -198,7 +205,12 @@ void test_submodule_status__ignore_dirty(void) cl_git_pass(git_submodule_foreach(g_repo, set_sm_ignore, &ign)); - cl_git_fail(git_submodule_lookup(&sm, g_repo, "not_submodule")); + cl_assert_equal_i(GIT_ENOTFOUND, + git_submodule_lookup(&sm, g_repo, "just_a_dir")); + cl_assert_equal_i(GIT_EEXISTS, + git_submodule_lookup(&sm, g_repo, "not-submodule")); + cl_assert_equal_i(GIT_EEXISTS, + git_submodule_lookup(&sm, g_repo, "not")); cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_changed_index")); cl_git_pass(git_submodule_status(&status, sm)); @@ -258,7 +270,12 @@ void test_submodule_status__ignore_all(void) cl_git_pass(git_submodule_foreach(g_repo, set_sm_ignore, &ign)); - cl_git_fail(git_submodule_lookup(&sm, g_repo, "not_submodule")); + cl_assert_equal_i(GIT_ENOTFOUND, + git_submodule_lookup(&sm, g_repo, "just_a_dir")); + cl_assert_equal_i(GIT_EEXISTS, + git_submodule_lookup(&sm, g_repo, "not-submodule")); + cl_assert_equal_i(GIT_EEXISTS, + git_submodule_lookup(&sm, g_repo, "not")); cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_changed_index")); cl_git_pass(git_submodule_status(&status, sm)); @@ -305,3 +322,64 @@ void test_submodule_status__ignore_all(void) git_buf_free(&path); } + +typedef struct { + size_t counter; + const char **paths; +} submodule_expectations; + +static int confirm_submodule_status( + const char *path, unsigned int status_flags, void *payload) +{ + submodule_expectations *exp = payload; + + while (git__suffixcmp(exp->paths[exp->counter], "/") == 0) + exp->counter++; + + cl_assert_equal_s(exp->paths[exp->counter++], path); + + GIT_UNUSED(status_flags); + + return 0; +} + +void test_submodule_status__iterator(void) +{ + git_iterator *iter; + const git_index_entry *entry; + size_t i; + static const char *expected[] = { + ".gitmodules", + "just_a_dir/", + "just_a_dir/contents", + "just_a_file", + "not", + "not-submodule", + "README.txt", + "sm_added_and_uncommited", + "sm_changed_file", + "sm_changed_head", + "sm_changed_index", + "sm_changed_untracked_file", + "sm_missing_commits", + "sm_unchanged", + NULL + }; + submodule_expectations exp = { 0, expected }; + git_status_options opts = GIT_STATUS_OPTIONS_INIT; + + cl_git_pass(git_iterator_for_workdir(&iter, g_repo, + GIT_ITERATOR_IGNORE_CASE | GIT_ITERATOR_INCLUDE_TREES, NULL, NULL)); + cl_git_pass(git_iterator_current(&entry, iter)); + + for (i = 0; entry; ++i) { + cl_assert_equal_s(expected[i], entry->path); + cl_git_pass(git_iterator_advance(&entry, iter)); + } + + git_iterator_free(iter); + + opts.flags = GIT_STATUS_OPT_INCLUDE_UNTRACKED | GIT_STATUS_OPT_INCLUDE_UNMODIFIED | GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS; + + cl_git_pass(git_status_foreach_ext(g_repo, &opts, confirm_submodule_status, &exp)); +}