Merge pull request #1367 from yorah/fix/pathspecs_behaviour

Correctly return matched pathspec when passing "*" or "."

src/attr_file.c

@@ -8,6 +8,10 @@
 
 static int sort_by_hash_and_name(const void *a_raw, const void *b_raw);
 static void git_attr_rule__clear(git_attr_rule *rule);
+static bool parse_optimized_patterns(
+	git_attr_fnmatch *spec,
+	git_pool *pool,
+	const char *pattern);
 
 int git_attr_file__new(
 	git_attr_file **attrs_ptr,
@@ -296,7 +300,6 @@ void git_attr_path__free(git_attr_path *info)
 	info->basename = NULL;
 }
 
-
 /*
  * From gitattributes(5):
  *
@@ -345,6 +348,9 @@ int git_attr_fnmatch__parse(
 
 	assert(spec && base && *base);
 
+	if (parse_optimized_patterns(spec, pool, *base))
+		return 0;
+
 	spec->flags = (spec->flags & GIT_ATTR_FNMATCH_ALLOWSPACE);
 	allow_space = (spec->flags != 0);
 
@@ -430,6 +436,22 @@ int git_attr_fnmatch__parse(
 	return 0;
 }
 
+static bool parse_optimized_patterns(
+	git_attr_fnmatch *spec,
+	git_pool *pool,
+	const char *pattern)
+{
+	if (!pattern[1] && (pattern[0] == '*' || pattern[0] == '.')) {
+		spec->flags = GIT_ATTR_FNMATCH_MATCH_ALL;
+		spec->pattern = git_pool_strndup(pool, pattern, 1);
+		spec->length = 1;
+
+		return true;
+	}
+
+	return false;
+}
+
 static int sort_by_hash_and_name(const void *a_raw, const void *b_raw)
 {
 	const git_attr_name *a = a_raw;

src/attr_file.h

@@ -27,6 +27,7 @@
 #define GIT_ATTR_FNMATCH_HASWILD	(1U << 5)
 #define GIT_ATTR_FNMATCH_ALLOWSPACE	(1U << 6)
 #define GIT_ATTR_FNMATCH_ICASE		(1U << 7)
+#define GIT_ATTR_FNMATCH_MATCH_ALL	(1U << 8)
 
 extern const char *git_attr__true;
 extern const char *git_attr__false;

src/pathspec.c

@@ -38,18 +38,20 @@ char *git_pathspec_prefix(const git_strarray *pathspec)
 }
 
 /* is there anything in the spec that needs to be filtered on */
-bool git_pathspec_is_interesting(const git_strarray *pathspec)
+bool git_pathspec_is_empty(const git_strarray *pathspec)
 {
-	const char *str;
+	size_t i;
 
-	if (pathspec == NULL || pathspec->count == 0)
-		return false;
-	if (pathspec->count > 1)
+	if (pathspec == NULL)
 		return true;
 
-	str = pathspec->strings[0];
-	if (!str || !str[0] || (!str[1] && (str[0] == '*' || str[0] == '.')))
+	for (i = 0; i < pathspec->count; ++i) {
+		const char *str = pathspec->strings[i];
+
+		if (str && str[0])
 			return false;
+	}
+
 	return true;
 }
 
@@ -61,7 +63,7 @@ int git_pathspec_init(
 
 	memset(vspec, 0, sizeof(*vspec));
 
-	if (!git_pathspec_is_interesting(strspec))
+	if (git_pathspec_is_empty(strspec))
 		return 0;
 
 	if (git_vector_init(vspec, strspec->count, NULL) < 0)
@@ -138,7 +140,10 @@ bool git_pathspec_match_path(
 	}
 
 	git_vector_foreach(vspec, i, match) {
-		int result = use_strcmp(match->pattern, path) ? FNM_NOMATCH : 0;
+		int result = (match->flags & GIT_ATTR_FNMATCH_MATCH_ALL) ? 0 : FNM_NOMATCH;
+			
+		if (result == FNM_NOMATCH)
+			result = use_strcmp(match->pattern, path) ? FNM_NOMATCH : 0;
 
 		if (fnmatch_flags >= 0 && result == FNM_NOMATCH)
 			result = p_fnmatch(match->pattern, path, fnmatch_flags);

src/pathspec.h

@@ -16,7 +16,7 @@
 extern char *git_pathspec_prefix(const git_strarray *pathspec);
 
 /* is there anything in the spec that needs to be filtered on */
-extern bool git_pathspec_is_interesting(const git_strarray *pathspec);
+extern bool git_pathspec_is_empty(const git_strarray *pathspec);
 
 /* build a vector of fnmatch patterns to evaluate efficiently */
 extern int git_pathspec_init(

tests-clar/diff/notify.c

+#include "clar_libgit2.h"
+#include "diff_helpers.h"
+
+static git_repository *g_repo = NULL;
+
+void test_diff_notify__initialize(void)
+{
+}
+
+void test_diff_notify__cleanup(void)
+{
+	cl_git_sandbox_cleanup();
+}
+
+static int assert_called_notifications(
+	const git_diff_list *diff_so_far,
+	const git_diff_delta *delta_to_add,
+	const char *matched_pathspec,
+	void *payload)
+{
+	bool found = false;
+	notify_expected *exp = (notify_expected*)payload;
+	notify_expected *e;;
+
+	GIT_UNUSED(diff_so_far);
+
+	for (e = exp; e->path != NULL; e++) {
+		if (strcmp(e->path, delta_to_add->new_file.path))
+			continue;
+
+		cl_assert_equal_s(e->matched_pathspec, matched_pathspec);
+
+		found = true;
+		break;
+	}
+
+	cl_assert(found);
+	return 0;
+}
+
+static void test_notify(
+	char **searched_pathspecs,
+	int pathspecs_count,
+	notify_expected *expected_matched_pathspecs,
+	int expected_diffed_files_count)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff_list *diff = NULL;
+	diff_expects exp;
+
+	g_repo = cl_git_sandbox_init("status");
+
+	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
+	opts.notify_cb = assert_called_notifications;
+	opts.pathspec.strings = searched_pathspecs;
+	opts.pathspec.count   = pathspecs_count;
+
+	opts.notify_payload = expected_matched_pathspecs;
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(expected_diffed_files_count, exp.files);
+
+	git_diff_list_free(diff);
+}
+
+void test_diff_notify__notify_single_pathspec(void)
+{
+	char *searched_pathspecs[] = {
+		"*_deleted",
+	};
+	notify_expected expected_matched_pathspecs[] = {
+		{ "file_deleted", "*_deleted" },
+		{ "staged_changes_file_deleted", "*_deleted" },
+		{ NULL, NULL }
+	};
+
+	test_notify(searched_pathspecs, 1, expected_matched_pathspecs, 2);
+}
+
+void test_diff_notify__notify_multiple_pathspec(void)
+{
+	char *searched_pathspecs[] = {
+		"staged_changes_cant_find_me",
+		"subdir/modified_cant_find_me",
+		"subdir/*",
+		"staged*"
+	};
+	notify_expected expected_matched_pathspecs[] = {
+		{ "staged_changes_file_deleted", "staged*" },
+		{ "staged_changes_modified_file", "staged*" },
+		{ "staged_delete_modified_file", "staged*" },
+		{ "staged_new_file_deleted_file", "staged*" },
+		{ "staged_new_file_modified_file", "staged*" },
+		{ "subdir/deleted_file", "subdir/*" },
+		{ "subdir/modified_file", "subdir/*" },
+		{ "subdir/new_file", "subdir/*" },
+		{ NULL, NULL }
+	};
+
+	test_notify(searched_pathspecs, 4, expected_matched_pathspecs, 8);
+}
+
+void test_diff_notify__notify_catchall_with_empty_pathspecs(void)
+{
+	char *searched_pathspecs[] = {
+		"",
+		""
+	};
+	notify_expected expected_matched_pathspecs[] = {
+		{ "file_deleted", NULL },
+		{ "ignored_file", NULL },
+		{ "modified_file", NULL },
+		{ "new_file", NULL },
+		{ "\xe8\xbf\x99", NULL },
+		{ "staged_changes_file_deleted", NULL },
+		{ "staged_changes_modified_file", NULL },
+		{ "staged_delete_modified_file", NULL },
+		{ "staged_new_file_deleted_file", NULL },
+		{ "staged_new_file_modified_file", NULL },
+		{ "subdir/deleted_file", NULL },
+		{ "subdir/modified_file", NULL },
+		{ "subdir/new_file", NULL },
+		{ NULL, NULL }
+	};
+
+	test_notify(searched_pathspecs, 1, expected_matched_pathspecs, 13);
+}
+
+void test_diff_notify__notify_catchall(void)
+{
+	char *searched_pathspecs[] = {
+		"*",
+	};
+	notify_expected expected_matched_pathspecs[] = {
+		{ "file_deleted", "*" },
+		{ "ignored_file", "*" },
+		{ "modified_file", "*" },
+		{ "new_file", "*" },
+		{ "\xe8\xbf\x99", "*" },
+		{ "staged_changes_file_deleted", "*" },
+		{ "staged_changes_modified_file", "*" },
+		{ "staged_delete_modified_file", "*" },
+		{ "staged_new_file_deleted_file", "*" },
+		{ "staged_new_file_modified_file", "*" },
+		{ "subdir/deleted_file", "*" },
+		{ "subdir/modified_file", "*" },
+		{ "subdir/new_file", "*" },
+		{ NULL, NULL }
+	};
+
+	test_notify(searched_pathspecs, 1, expected_matched_pathspecs, 13);
+}
+
+static int abort_diff(
+	const git_diff_list *diff_so_far,
+	const git_diff_delta *delta_to_add,
+	const char *matched_pathspec,
+	void *payload)
+{
+	GIT_UNUSED(diff_so_far);
+	GIT_UNUSED(delta_to_add);
+	GIT_UNUSED(matched_pathspec);
+	GIT_UNUSED(payload);
+
+	return -42;
+}
+
+void test_diff_notify__notify_cb_can_abort_diff(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff_list *diff = NULL;
+	char *pathspec = NULL;
+
+	g_repo = cl_git_sandbox_init("status");
+
+	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
+	opts.notify_cb = abort_diff;
+	opts.pathspec.strings = &pathspec;
+	opts.pathspec.count   = 1;
+
+	pathspec = "file_deleted";
+	cl_git_fail(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	pathspec = "staged_changes_modified_file";
+	cl_git_fail(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+}
+
+static int filter_all(
+	const git_diff_list *diff_so_far,
+	const git_diff_delta *delta_to_add,
+	const char *matched_pathspec,
+	void *payload)
+{
+	GIT_UNUSED(diff_so_far);
+	GIT_UNUSED(delta_to_add);
+	GIT_UNUSED(matched_pathspec);
+	GIT_UNUSED(payload);
+
+	return 42;
+}
+
+void test_diff_notify__notify_cb_can_be_used_as_filtering_function(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff_list *diff = NULL;
+	char *pathspec = NULL;
+	diff_expects exp;
+
+	g_repo = cl_git_sandbox_init("status");
+
+	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
+	opts.notify_cb = filter_all;
+	opts.pathspec.strings = &pathspec;
+	opts.pathspec.count   = 1;
+
+	pathspec = "*_deleted";
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(0, exp.files);
+
+	git_diff_list_free(diff);
+}

tests-clar/diff/workdir.c

@@ -307,169 +307,6 @@ void test_diff_workdir__to_index_with_pathspec(void)
 	git_diff_list_free(diff);
 }
 
-static int assert_called_notifications(
-	const git_diff_list *diff_so_far,
-	const git_diff_delta *delta_to_add,
-	const char *matched_pathspec,
-	void *payload)
-{
-	bool found = false;
-	notify_expected *exp = (notify_expected*)payload;
-	notify_expected *e;;
-
-	GIT_UNUSED(diff_so_far);
-
-	for (e = exp; e->path != NULL; e++) {
-		if (strcmp(e->path, delta_to_add->new_file.path))
-			continue;
-
-		cl_assert_equal_s(e->matched_pathspec, matched_pathspec);
-
-		found = true;
-		break;
-	}
-
-	cl_assert(found);
-	return 0;
-}
-
-void test_diff_workdir__to_index_notify(void)
-{
-	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
-	git_diff_list *diff = NULL;
-	diff_expects exp;
-
-	char *searched_pathspecs_solo[] = {
-		"*_deleted",
-	};
-	notify_expected expected_matched_pathspecs_solo[] = {
-		{ "file_deleted", "*_deleted" },
-		{ "staged_changes_file_deleted", "*_deleted" },
-		{ NULL, NULL }
-	};
-
-	char *searched_pathspecs_multiple[] = {
-		"staged_changes_cant_find_me",
-		"subdir/modified_cant_find_me",
-		"subdir/*",
-		"staged*"
-	};
-	notify_expected expected_matched_pathspecs_multiple[] = {
-		{ "staged_changes_file_deleted", "staged*" },
-		{ "staged_changes_modified_file", "staged*" },
-		{ "staged_delete_modified_file", "staged*" },
-		{ "staged_new_file_deleted_file", "staged*" },
-		{ "staged_new_file_modified_file", "staged*" },
-		{ "subdir/deleted_file", "subdir/*" },
-		{ "subdir/modified_file", "subdir/*" },
-		{ "subdir/new_file", "subdir/*" },
-		{ NULL, NULL }
-	};
-
-	g_repo = cl_git_sandbox_init("status");
-
-	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
-	opts.notify_cb = assert_called_notifications;
-	opts.pathspec.strings = searched_pathspecs_solo;
-	opts.pathspec.count   = 1;
-
-	opts.notify_payload = &expected_matched_pathspecs_solo;
-	memset(&exp, 0, sizeof(exp));
-
-	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
-	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
-
-	cl_assert_equal_i(2, exp.files);
-
-	git_diff_list_free(diff);
-
-	opts.pathspec.strings = searched_pathspecs_multiple;
-	opts.pathspec.count   = 4;
-	opts.notify_payload = &expected_matched_pathspecs_multiple;
-	memset(&exp, 0, sizeof(exp));
-
-	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
-	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
-
-	cl_assert_equal_i(8, exp.files);
-
-	git_diff_list_free(diff);
-}
-
-static int abort_diff(
-	const git_diff_list *diff_so_far,
-	const git_diff_delta *delta_to_add,
-	const char *matched_pathspec,
-	void *payload)
-{
-	GIT_UNUSED(diff_so_far);
-	GIT_UNUSED(delta_to_add);
-	GIT_UNUSED(matched_pathspec);
-	GIT_UNUSED(payload);
-
-	return -42;
-}
-
-void test_diff_workdir__to_index_notify_can_be_aborted_by_callback(void)
-{
-	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
-	git_diff_list *diff = NULL;
-	char *pathspec = NULL;
-
-	g_repo = cl_git_sandbox_init("status");
-
-	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
-	opts.notify_cb = abort_diff;
-	opts.pathspec.strings = &pathspec;
-	opts.pathspec.count   = 1;
-
-	pathspec = "file_deleted";
-	cl_git_fail(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
-
-	pathspec = "staged_changes_modified_file";
-	cl_git_fail(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
-}
-
-static int filter_all(
-	const git_diff_list *diff_so_far,
-	const git_diff_delta *delta_to_add,
-	const char *matched_pathspec,
-	void *payload)
-{
-	GIT_UNUSED(diff_so_far);
-	GIT_UNUSED(delta_to_add);
-	GIT_UNUSED(matched_pathspec);
-	GIT_UNUSED(payload);
-
-	return 42;
-}
-
-void test_diff_workdir__to_index_notify_can_be_used_as_filtering_function(void)
-{
-	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
-	git_diff_list *diff = NULL;
-	char *pathspec = NULL;
-	diff_expects exp;
-
-	g_repo = cl_git_sandbox_init("status");
-
-	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
-	opts.notify_cb = filter_all;
-	opts.pathspec.strings = &pathspec;
-	opts.pathspec.count   = 1;
-
-	pathspec = "*_deleted";
-	memset(&exp, 0, sizeof(exp));
-
-	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
-	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
-
-	cl_assert_equal_i(0, exp.files);
-
-	git_diff_list_free(diff);
-}
-
-
 void test_diff_workdir__filemode_changes(void)
 {
 	git_diff_list *diff = NULL;