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

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