Merge pull request #1423 from arrbee/submodule-status-errors

Three submodule status bug fixes

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;

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) {

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);

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';
+			}
 		}
 	}
 

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));
 	}
 

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 */
 

tests-clar/resources/submod2/not/.gitted/notempty

@@ -0,0 +1 @@
+fooled you

tests-clar/resources/submod2/not/README.txt

@@ -0,0 +1 @@
+what am I really

tests-clar/resources/submod2/not_submodule/.gitted/COMMIT_EDITMSG

@@ -1 +0,0 @@
-Initial commit

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);

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)

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));
+}