Merge pull request #3724 from ethomson/submodule_start_supports_silly_slashes

iterator/diff: allow trailing `/` on start/end paths to match submodules

3 files changed, 85 insertions, 16 deletions

diff --git a/src/iterator.c b/src/iterator.c
index 60b33cc70..ec44aac4c 100644
--- a/src/iterator.c
+++ b/src/iterator.c
@@ -169,7 +169,8 @@ static void iterator_clear(git_iterator *iter)
 	iter->flags &= ~GIT_ITERATOR_FIRST_ACCESS;
 }
 
-GIT_INLINE(bool) iterator_has_started(git_iterator *iter, const char *path)
+GIT_INLINE(bool) iterator_has_started(
+	git_iterator *iter, const char *path, bool is_submodule)
 {
 	size_t path_len;
 
@@ -181,19 +182,32 @@ GIT_INLINE(bool) iterator_has_started(git_iterator *iter, const char *path)
 	 */
 	iter->started = (iter->prefixcomp(path, iter->start) >= 0);
 
+	if (iter->started)
+		return true;
+
+	path_len = strlen(path);
+
+	/* if, however, we are a submodule, then we support `start` being
+	 * suffixed with a `/` for crazy legacy reasons.  match `submod`
+	 * with a start path of `submod/`.
+	 */
+	if (is_submodule && iter->start_len && path_len == iter->start_len - 1 &&
+		iter->start[iter->start_len-1] == '/')
+		return true;
+
 	/* if, however, our current path is a directory, and our starting path
 	 * is _beneath_ that directory, then recurse into the directory (even
 	 * though we have not yet "started")
 	 */
-	if (!iter->started &&
-		(path_len = strlen(path)) > 0 && path[path_len-1] == '/' &&
+	if (path_len > 0 && path[path_len-1] == '/' &&
 		iter->strncomp(path, iter->start, path_len) == 0)
 		return true;
 
-	return iter->started;
+	return false;
 }
 
-GIT_INLINE(bool) iterator_has_ended(git_iterator *iter, const char *path)
+GIT_INLINE(bool) iterator_has_ended(
+	git_iterator *iter, const char *path, bool is_submodule)
 {
 	if (iter->end == NULL)
 		return false;
@@ -779,11 +793,11 @@ static int tree_iterator_advance(const git_index_entry **out, git_iterator *i)
 			break;
 
 		/* if this path is before our start, advance over this entry */
-		if (!iterator_has_started(&iter->base, iter->entry_path.ptr))
+		if (!iterator_has_started(&iter->base, iter->entry_path.ptr, false))
 			continue;
 
 		/* if this path is after our end, stop */
-		if (iterator_has_ended(&iter->base, iter->entry_path.ptr)) {
+		if (iterator_has_ended(&iter->base, iter->entry_path.ptr, false)) {
 			error = GIT_ITEROVER;
 			break;
 		}
@@ -1400,7 +1414,7 @@ static int filesystem_iterator_frame_push(
 		}
 
 		/* Ensure that the pathlist entry lines up with what we expected */
-		if (dir_expected && !S_ISDIR(statbuf.st_mode))
+		else if (dir_expected)
 			continue;
 
 		entry = filesystem_iterator_entry_init(new_frame,
@@ -1995,6 +2009,7 @@ static int index_iterator_advance(
 {
 	index_iterator *iter = (index_iterator *)i;
 	const git_index_entry *entry = NULL;
+	bool is_submodule;
 	int error = 0;
 
 	iter->base.flags |= GIT_ITERATOR_FIRST_ACCESS;
@@ -2012,13 +2027,14 @@ static int index_iterator_advance(
 		}
 
 		entry = iter->entries.contents[iter->next_idx];
+		is_submodule = S_ISGITLINK(entry->mode);
 
-		if (!iterator_has_started(&iter->base, entry->path)) {
+		if (!iterator_has_started(&iter->base, entry->path, is_submodule)) {
 			iter->next_idx++;
 			continue;
 		}
 
-		if (iterator_has_ended(&iter->base, entry->path)) {
+		if (iterator_has_ended(&iter->base, entry->path, is_submodule)) {
 			error = GIT_ITEROVER;
 			break;
 		}
diff --git a/tests/diff/submodules.c b/tests/diff/submodules.c
index 08682cd4b..eebfef3a2 100644
--- a/tests/diff/submodules.c
+++ b/tests/diff/submodules.c
@@ -493,3 +493,50 @@ void test_diff_submodules__skips_empty_includes_used(void)
 	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNTRACKED]);
 	git_diff_free(diff);
 }
+
+static void ensure_submodules_found(
+	git_repository *repo,
+	const char **paths,
+	size_t cnt)
+{
+	git_diff *diff = NULL;
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	const git_diff_delta *delta;
+	size_t i, pathlen;
+
+	opts.pathspec.strings = (char **)paths;
+	opts.pathspec.count = cnt;
+
+	git_diff_index_to_workdir(&diff, repo, NULL, &opts);
+
+	cl_assert_equal_i(cnt, git_diff_num_deltas(diff));
+
+	for (i = 0; i < cnt; i++) {
+		delta = git_diff_get_delta(diff, i);
+
+		/* ensure that the given path is returned w/o trailing slashes. */
+		pathlen = strlen(opts.pathspec.strings[i]);
+
+		while (pathlen && opts.pathspec.strings[i][pathlen - 1] == '/')
+			pathlen--;
+
+		cl_assert_equal_strn(opts.pathspec.strings[i], delta->new_file.path, pathlen);
+	}
+
+	git_diff_free(diff);
+}
+
+void test_diff_submodules__can_be_identified_by_trailing_slash_in_pathspec(void)
+{
+	const char *one_path_without_slash[] = { "sm_changed_head" };
+	const char *one_path_with_slash[] = { "sm_changed_head/" };
+	const char *many_paths_without_slashes[] = { "sm_changed_head", "sm_changed_index" };
+	const char *many_paths_with_slashes[] = { "sm_changed_head/", "sm_changed_index/" };
+
+	g_repo = setup_fixture_submod2();
+
+	ensure_submodules_found(g_repo, one_path_without_slash, ARRAY_SIZE(one_path_without_slash));
+	ensure_submodules_found(g_repo, one_path_with_slash, ARRAY_SIZE(one_path_with_slash));
+	ensure_submodules_found(g_repo, many_paths_without_slashes, ARRAY_SIZE(many_paths_without_slashes));
+	ensure_submodules_found(g_repo, many_paths_with_slashes, ARRAY_SIZE(many_paths_with_slashes));
+}
diff --git a/tests/iterator/workdir.c b/tests/iterator/workdir.c
index fc7771c20..c8f795a0d 100644
--- a/tests/iterator/workdir.c
+++ b/tests/iterator/workdir.c
@@ -1197,8 +1197,11 @@ void test_iterator_workdir__bounded_submodules(void)
 		git_iterator_free(i);
 	}
 
-	/* Test that a submodule never matches when suffixed with a '/' */
+	/* Test that a submodule still matches when suffixed with a '/' */
 	{
+		const char *expected[] = { "sm_changed_head" };
+		size_t expected_len = 1;
+
 		git_vector_clear(&filelist);
 		cl_git_pass(git_vector_insert(&filelist, "sm_changed_head/"));
 
@@ -1207,7 +1210,7 @@ void test_iterator_workdir__bounded_submodules(void)
 		i_opts.flags = GIT_ITERATOR_DONT_IGNORE_CASE;
 
 		cl_git_pass(git_iterator_for_workdir(&i, g_repo, index, head, &i_opts));
-		cl_git_fail_with(GIT_ITEROVER, git_iterator_advance(NULL, i));
+		expect_iterator_items(i, expected_len, expected, expected_len, expected);
 		git_iterator_free(i);
 	}
 
@@ -1227,16 +1230,19 @@ void test_iterator_workdir__bounded_submodules(void)
 		git_iterator_free(i);
 	}
 
-	/* Test that start and end do not allow '/' suffixes of submodules */
+	/* Test that start and end allow '/' suffixes of submodules */
 	{
-		i_opts.start = "sm_changed_head/";
-		i_opts.end = "sm_changed_head/";
+		const char *expected[] = { "sm_changed_head", "sm_changed_index" };
+		size_t expected_len = 2;
+
+		i_opts.start = "sm_changed_head";
+		i_opts.end = "sm_changed_index";
 		i_opts.pathlist.strings = NULL;
 		i_opts.pathlist.count = 0;
 		i_opts.flags = GIT_ITERATOR_DONT_IGNORE_CASE;
 
 		cl_git_pass(git_iterator_for_workdir(&i, g_repo, index, head, &i_opts));
-		cl_git_fail_with(GIT_ITEROVER, git_iterator_advance(NULL, i));
+		expect_iterator_items(i, expected_len, expected, expected_len, expected);
 		git_iterator_free(i);
 	}