Two-step conflict checkout (load / perform)

Move conflict handling into two steps: load the conflicts and
then apply the conflicts.  This is more compatible with the
existing checkout implementation and makes progress reporting
more sane.

src/checkout.c

@@ -29,18 +29,6 @@
 
 /* See docs/checkout-internals.md for more information */
 
-enum {
-	CHECKOUT_ACTION__NONE = 0,
-	CHECKOUT_ACTION__REMOVE = 1,
-	CHECKOUT_ACTION__UPDATE_BLOB = 2,
-	CHECKOUT_ACTION__UPDATE_SUBMODULE = 4,
-	CHECKOUT_ACTION__CONFLICT = 8,
-	CHECKOUT_ACTION__MAX = 8,
-	CHECKOUT_ACTION__DEFER_REMOVE = 16,
-	CHECKOUT_ACTION__REMOVE_AND_UPDATE =
-		(CHECKOUT_ACTION__UPDATE_BLOB | CHECKOUT_ACTION__REMOVE),
-};
-
 static int checkout_notify(
 	checkout_data *data,
 	git_checkout_notify_t why,
@@ -641,6 +629,14 @@ static int checkout_get_actions(
 		goto fail;
 	}
 
+
+	if ((error = git_checkout__get_conflicts(data, workdir, &pathspec)) < 0)
+		goto fail;
+
+	counts[CHECKOUT_ACTION__UPDATE_CONFLICT] = git_vector_length(&data->conflicts);
+
+	/* HERE */
+
 	git_pathspec__vfree(&pathspec);
 	git_pool_clear(&pathpool);
 
@@ -841,7 +837,7 @@ static int checkout_submodule(
 	return checkout_submodule_update_index(data, file);
 }
 
-static void report_progress(
+void git_checkout__report_progress(
 	checkout_data *data,
 	const char *path)
 {
@@ -965,7 +961,7 @@ static int checkout_remove_the_old(
 				return error;
 
 			data->completed_steps++;
-			report_progress(data, delta->old_file.path);
+			git_checkout__report_progress(data, delta->old_file.path);
 
 			if ((actions[i] & CHECKOUT_ACTION__UPDATE_BLOB) == 0 &&
 				(data->strategy & GIT_CHECKOUT_DONT_UPDATE_INDEX) == 0 &&
@@ -982,7 +978,7 @@ static int checkout_remove_the_old(
 			return error;
 
 		data->completed_steps++;
-		report_progress(data, str);
+		git_checkout__report_progress(data, str);
 
 		if ((data->strategy & GIT_CHECKOUT_DONT_UPDATE_INDEX) == 0 &&
 			data->index != NULL)
@@ -1041,7 +1037,7 @@ static int checkout_create_the_new(
 				return error;
 
 			data->completed_steps++;
-			report_progress(data, delta->new_file.path);
+			git_checkout__report_progress(data, delta->new_file.path);
 		}
 	}
 
@@ -1077,7 +1073,7 @@ static int checkout_create_submodules(
 				return error;
 
 			data->completed_steps++;
-			report_progress(data, delta->new_file.path);
+			git_checkout__report_progress(data, delta->new_file.path);
 		}
 	}
 
@@ -1102,6 +1098,9 @@ static int checkout_lookup_head_tree(git_tree **out, git_repository *repo)
 
 static void checkout_data_clear(checkout_data *data)
 {
+	checkout_conflictdata *conflict;
+	size_t i;
+
 	if (data->opts_free_baseline) {
 		git_tree_free(data->opts.baseline);
 		data->opts.baseline = NULL;
@@ -1110,6 +1109,11 @@ static void checkout_data_clear(checkout_data *data)
 	git_vector_free(&data->removes);
 	git_pool_clear(&data->pool);
 
+	git_vector_foreach(&data->conflicts, i, conflict)
+		git__free(conflict);
+
+	git_vector_free(&data->conflicts);
+
 	git__free(data->pfx);
 	data->pfx = NULL;
 
@@ -1226,6 +1230,7 @@ static int checkout_data_init(
 	}
 
 	if ((error = git_vector_init(&data->removes, 0, git__strcmp_cb)) < 0 ||
+		(error = git_vector_init(&data->conflicts, 0, NULL)) < 0 ||
 		(error = git_pool_init(&data->pool, 1, 0)) < 0 ||
 		(error = git_buf_puts(&data->path, data->opts.target_directory)) < 0 ||
 		(error = git_path_to_dir(&data->path)) < 0)
@@ -1296,16 +1301,18 @@ int git_checkout_iterator(
 		goto cleanup;
 
 	/* Loop through diff (and working directory iterator) building a list of
-	 * actions to be taken, plus look for conflicts and send notifications.
+	 * actions to be taken, plus look for conflicts and send notifications,
+	 * then loop through conflicts.
 	 */
 	if ((error = checkout_get_actions(&actions, &counts, &data, workdir)) < 0)
 		goto cleanup;
 
 	data.total_steps = counts[CHECKOUT_ACTION__REMOVE] +
 		counts[CHECKOUT_ACTION__UPDATE_BLOB] +
-		counts[CHECKOUT_ACTION__UPDATE_SUBMODULE];
+		counts[CHECKOUT_ACTION__UPDATE_SUBMODULE] +
+		counts[CHECKOUT_ACTION__UPDATE_CONFLICT];
 
-	report_progress(&data, NULL); /* establish 0 baseline */
+	git_checkout__report_progress(&data, NULL); /* establish 0 baseline */
 
 	/* To deal with some order dependencies, perform remaining checkout
 	 * in three passes: removes, then update blobs, then update submodules.
@@ -1322,10 +1329,11 @@ int git_checkout_iterator(
 		(error = checkout_create_submodules(actions, &data)) < 0)
 		goto cleanup;
 
-	assert(data.completed_steps == data.total_steps);
+	if (counts[CHECKOUT_ACTION__UPDATE_CONFLICT] > 0 &&
+		(error = git_checkout__conflicts(&data)) < 0)
+		goto cleanup;
 
-	/* Write conflict data to disk */
-	error = git_checkout__conflicts(&data);
+	assert(data.completed_steps == data.total_steps);
 
 cleanup:
 	if (error == GIT_EUSER)

src/checkout.h

@@ -22,6 +22,7 @@ typedef struct {
 	git_index *index;
 	git_pool pool;
 	git_vector removes;
+	git_vector conflicts;
 	git_buf path;
 	size_t workdir_len;
 	unsigned int strategy;
@@ -31,6 +32,29 @@ typedef struct {
 	size_t completed_steps;
 } checkout_data;
 
+typedef struct {
+	const git_index_entry *ancestor;
+	const git_index_entry *ours;
+	const git_index_entry *theirs;
+
+	int name_collision:1,
+		directoryfile:1,
+		one_to_two:1;
+} checkout_conflictdata;
+
+enum {
+	CHECKOUT_ACTION__NONE = 0,
+	CHECKOUT_ACTION__REMOVE = 1,
+	CHECKOUT_ACTION__UPDATE_BLOB = 2,
+	CHECKOUT_ACTION__UPDATE_SUBMODULE = 4,
+	CHECKOUT_ACTION__CONFLICT = 8,
+	CHECKOUT_ACTION__UPDATE_CONFLICT = 16,
+	CHECKOUT_ACTION__MAX = 16,
+	CHECKOUT_ACTION__DEFER_REMOVE = 32,
+	CHECKOUT_ACTION__REMOVE_AND_UPDATE =
+		(CHECKOUT_ACTION__UPDATE_BLOB | CHECKOUT_ACTION__REMOVE),
+};
+
 /**
  * Update the working directory to match the target iterator.  The
  * expected baseline value can be passed in via the checkout options
@@ -52,6 +76,11 @@ int git_checkout__write_content(
 	unsigned int mode,
 	struct stat *st);
 
+void git_checkout__report_progress(
+	checkout_data *data,
+	const char *path);
+
+int git_checkout__get_conflicts(checkout_data *data, git_iterator *workdir, git_vector *pathspec);
 int git_checkout__conflicts(checkout_data *data);
 
 #endif

src/checkout_conflicts.c

@@ -11,22 +11,13 @@
 
 #include "vector.h"
 #include "index.h"
+#include "pathspec.h"
 #include "merge_file.h"
 #include "git2/repository.h"
 #include "git2/types.h"
 #include "git2/index.h"
 #include "git2/sys/index.h"
 
-typedef struct {
-	const git_index_entry *ancestor;
-	const git_index_entry *ours;
-	const git_index_entry *theirs;
-
-	int name_collision:1,
-		directoryfile:1,
-		one_to_two:1;
-} checkout_conflictdata;
-
 GIT_INLINE(int) checkout_idxentry_cmp(
 	const git_index_entry *a,
 	const git_index_entry *b)
@@ -68,7 +59,34 @@ int checkout_conflictdata_empty(const git_vector *conflicts, size_t idx)
 	return 1;
 }
 
-static int checkout_conflicts_load(checkout_data *data, git_vector *conflicts)
+GIT_INLINE(bool) conflict_pathspec_match(
+	checkout_data *data,
+	git_iterator *workdir,
+	git_vector *pathspec,
+	const git_index_entry *ancestor,
+	const git_index_entry *ours,
+	const git_index_entry *theirs)
+{
+	/* if the pathspec matches ours *or* theirs, proceed */
+	if (ours && git_pathspec__match(pathspec, ours->path,
+		(data->strategy & GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH) != 0,
+		git_iterator_ignore_case(workdir), NULL, NULL))
+		return true;
+
+	if (theirs && git_pathspec__match(pathspec, theirs->path,
+		(data->strategy & GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH) != 0,
+		git_iterator_ignore_case(workdir), NULL, NULL))
+		return true;
+
+	if (ancestor && git_pathspec__match(pathspec, ancestor->path,
+		(data->strategy & GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH) != 0,
+		git_iterator_ignore_case(workdir), NULL, NULL))
+		return true;
+
+	return false;
+}
+
+static int checkout_conflicts_load(checkout_data *data, git_iterator *workdir, git_vector *pathspec)
 {
 	git_index_conflict_iterator *iterator = NULL;
 	const git_index_entry *ancestor, *ours, *theirs;
@@ -78,11 +96,12 @@ static int checkout_conflicts_load(checkout_data *data, git_vector *conflicts)
 	if ((error = git_index_conflict_iterator_new(&iterator, data->index)) < 0)
 		goto done;
 
-	conflicts->_cmp = checkout_conflictdata_cmp;
+	data->conflicts._cmp = checkout_conflictdata_cmp;
 
 	/* Collect the conflicts */
-	while ((error = git_index_conflict_next(
-		&ancestor, &ours, &theirs, iterator)) == 0) {
+	while ((error = git_index_conflict_next(&ancestor, &ours, &theirs, iterator)) == 0) {
+		if (!conflict_pathspec_match(data, workdir, pathspec, ancestor, ours, theirs))
+			continue;
 
 		conflict = git__calloc(1, sizeof(checkout_conflictdata));
 		GITERR_CHECK_ALLOC(conflict);
@@ -91,7 +110,7 @@ static int checkout_conflicts_load(checkout_data *data, git_vector *conflicts)
 		conflict->ours = ours;
 		conflict->theirs = theirs;
 
-		git_vector_insert(conflicts, conflict);
+		git_vector_insert(&data->conflicts, conflict);
 	}
 
 	if (error == GIT_ITEROVER)
@@ -122,25 +141,25 @@ static int checkout_conflicts_cmp_ancestor(const void *p, const void *c)
 }
 
 static checkout_conflictdata *checkout_conflicts_search_ancestor(
-	git_vector *conflicts,
+	checkout_data *data,
 	const char *path)
 {
 	size_t pos;
 
-	if (git_vector_bsearch2(&pos, conflicts, checkout_conflicts_cmp_ancestor, path) < 0)
+	if (git_vector_bsearch2(&pos, &data->conflicts, checkout_conflicts_cmp_ancestor, path) < 0)
 		return NULL;
 
-	return git_vector_get(conflicts, pos);
+	return git_vector_get(&data->conflicts, pos);
 }
 
 static checkout_conflictdata *checkout_conflicts_search_branch(
-	git_vector *conflicts,
+	checkout_data *data,
 	const char *path)
 {
 	checkout_conflictdata *conflict;
 	size_t i;
 
-	git_vector_foreach(conflicts, i, conflict) {
+	git_vector_foreach(&data->conflicts, i, conflict) {
 		int cmp = -1;
 
 		if (conflict->ancestor)
@@ -162,7 +181,7 @@ static int checkout_conflicts_load_byname_entry(
 	checkout_conflictdata **ancestor_out,
 	checkout_conflictdata **ours_out,
 	checkout_conflictdata **theirs_out,
-	git_vector *conflicts,
+	checkout_data *data,
 	const git_index_name_entry *name_entry)
 {
 	checkout_conflictdata *ancestor, *ours = NULL, *theirs = NULL;
@@ -184,7 +203,7 @@ static int checkout_conflicts_load_byname_entry(
 		goto done;
 	}
 
-	if ((ancestor = checkout_conflicts_search_ancestor(conflicts,
+	if ((ancestor = checkout_conflicts_search_ancestor(data,
 		name_entry->ancestor)) == NULL) {
 		giterr_set(GITERR_INDEX,
 			"A NAME entry referenced ancestor entry '%s' which does not exist in the main index",
@@ -196,7 +215,7 @@ static int checkout_conflicts_load_byname_entry(
 	if (name_entry->ours) {
 		if (strcmp(name_entry->ancestor, name_entry->ours) == 0)
 			ours = ancestor;
-		else if ((ours = checkout_conflicts_search_branch(conflicts, name_entry->ours)) == NULL ||
+		else if ((ours = checkout_conflicts_search_branch(data, name_entry->ours)) == NULL ||
 			ours->ours == NULL) {
 			giterr_set(GITERR_INDEX,
 				"A NAME entry referenced our entry '%s' which does not exist in the main index",
@@ -211,7 +230,7 @@ static int checkout_conflicts_load_byname_entry(
 			theirs = ancestor;
 		else if (name_entry->ours && strcmp(name_entry->ours, name_entry->theirs) == 0)
 			theirs = ours;
-		else if ((theirs = checkout_conflicts_search_branch(conflicts, name_entry->theirs)) == NULL ||
+		else if ((theirs = checkout_conflicts_search_branch(data, name_entry->theirs)) == NULL ||
 			theirs->theirs == NULL) {
 			giterr_set(GITERR_INDEX,
 				"A NAME entry referenced their entry '%s' which does not exist in the main index",
@@ -230,8 +249,7 @@ done:
 }
 
 static int checkout_conflicts_coalesce_renames(
-	checkout_data *data,
-	git_vector *conflicts)
+	checkout_data *data)
 {
 	const git_index_name_entry *name_entry;
 	checkout_conflictdata *ancestor_conflict, *our_conflict, *their_conflict;
@@ -239,15 +257,14 @@ static int checkout_conflicts_coalesce_renames(
 	int error = 0;
 
 	/* Juggle entries based on renames */
-	for (i = 0, names = git_index_name_entrycount(data->index);
-		i < names;
-		i++) {
+	names = git_index_name_entrycount(data->index);
 	
+	for (i = 0; i < names; i++) {
 		name_entry = git_index_name_get_byindex(data->index, i);
 
 		if ((error = checkout_conflicts_load_byname_entry(
 			&ancestor_conflict, &our_conflict, &their_conflict,
-			conflicts, name_entry)) < 0)
+			data, name_entry)) < 0)
 			goto done;
 
 		if (our_conflict && our_conflict != ancestor_conflict) {
@@ -277,7 +294,7 @@ static int checkout_conflicts_coalesce_renames(
 			ancestor_conflict->one_to_two = 1;
 	}
 
-	git_vector_remove_matching(conflicts, checkout_conflictdata_empty);
+	git_vector_remove_matching(&data->conflicts, checkout_conflictdata_empty);
 
 done:
 	return error;
@@ -307,8 +324,7 @@ GIT_INLINE(void) path_equal_or_prefixed(
 }
 
 static int checkout_conflicts_mark_directoryfile(
-	checkout_data *data,
-	git_vector *conflicts)
+	checkout_data *data)
 {
 	checkout_conflictdata *conflict;
 	const git_index_entry *entry;
@@ -320,7 +336,7 @@ static int checkout_conflicts_mark_directoryfile(
 	len = git_index_entrycount(data->index);
 
 	/* Find d/f conflicts */
-	git_vector_foreach(conflicts, i, conflict) {
+	git_vector_foreach(&data->conflicts, i, conflict) {
 		if ((conflict->ours && conflict->theirs) ||
 			(!conflict->ours && !conflict->theirs))
 			continue;
@@ -560,22 +576,30 @@ done:
 	return error;
 }
 
-int git_checkout__conflicts(checkout_data *data)
+int git_checkout__get_conflicts(checkout_data *data, git_iterator *workdir, git_vector *pathspec)
 {
-	git_vector conflicts = GIT_VECTOR_INIT;
-	checkout_conflictdata *conflict;
-	size_t i;
 	int error = 0;
 
 	if (data->strategy & GIT_CHECKOUT_SKIP_UNMERGED)
 		return 0;
 
-	if ((error = checkout_conflicts_load(data, &conflicts)) < 0 ||
-		(error = checkout_conflicts_coalesce_renames(data, &conflicts)) < 0 ||
-		(error = checkout_conflicts_mark_directoryfile(data, &conflicts)) < 0)
+	if ((error = checkout_conflicts_load(data, workdir, pathspec)) < 0 ||
+		(error = checkout_conflicts_coalesce_renames(data)) < 0 ||
+		(error = checkout_conflicts_mark_directoryfile(data)) < 0)
 		goto done;
 
-	git_vector_foreach(&conflicts, i, conflict) {
+done:
+	return error;
+}
+
+int git_checkout__conflicts(checkout_data *data)
+{
+	git_vector conflicts = GIT_VECTOR_INIT;
+	checkout_conflictdata *conflict;
+	size_t i;
+	int error = 0;
+
+	git_vector_foreach(&data->conflicts, i, conflict) {
 		/* Both deleted: nothing to do */
 		if (conflict->ours == NULL && conflict->theirs == NULL)
 			error = 0;
@@ -621,13 +645,15 @@ int git_checkout__conflicts(checkout_data *data)
 
 		else
 			error = checkout_write_merge(data, conflict);
-	}
 
-done:
-	git_vector_foreach(&conflicts, i, conflict)
-		git__free(conflict);
+		if (error)
+			break;
 
-	git_vector_free(&conflicts);
+		data->completed_steps++;
+		git_checkout__report_progress(data,
+			conflict->ours ? conflict->ours->path :
+			(conflict->theirs ? conflict->theirs->path : conflict->ancestor->path));
+	}
 
 	return error;
 }

tests-clar/checkout/conflict.c

@@ -1022,3 +1022,103 @@ void test_checkout_conflict__update_only(void)
 	cl_assert(!git_path_exists("merge-resolve/directory_file-one~ours"));
 	cl_assert(!git_path_exists("merge-resolve/directory_file-two~theirs"));
 }
+
+void test_checkout_conflict__path_filters(void)
+{
+	git_checkout_opts opts = GIT_CHECKOUT_OPTS_INIT;
+	char *paths[] = { "conflicting-1.txt", "conflicting-3.txt" };
+	git_strarray patharray = {0};
+
+	struct checkout_index_entry checkout_index_entries[] = {
+		{ 0100644, CONFLICTING_ANCESTOR_OID, 1, "conflicting-1.txt" },
+		{ 0100644, CONFLICTING_OURS_OID, 2, "conflicting-1.txt" },
+		{ 0100644, CONFLICTING_THEIRS_OID, 3, "conflicting-1.txt" },
+
+		{ 0100644, CONFLICTING_ANCESTOR_OID, 1, "conflicting-2.txt" },
+		{ 0100644, CONFLICTING_OURS_OID, 2, "conflicting-2.txt" },
+		{ 0100644, CONFLICTING_THEIRS_OID, 3, "conflicting-2.txt" },
+
+		{ 0100644, AUTOMERGEABLE_ANCESTOR_OID, 1, "conflicting-3.txt" },
+		{ 0100644, AUTOMERGEABLE_OURS_OID, 2, "conflicting-3.txt" },
+		{ 0100644, AUTOMERGEABLE_THEIRS_OID, 3, "conflicting-3.txt" },
+
+		{ 0100644, AUTOMERGEABLE_ANCESTOR_OID, 1, "conflicting-4.txt" },
+		{ 0100644, AUTOMERGEABLE_OURS_OID, 2, "conflicting-4.txt" },
+		{ 0100644, AUTOMERGEABLE_THEIRS_OID, 3, "conflicting-4.txt" },
+	};
+
+	patharray.count = 2;
+	patharray.strings = paths;
+
+	opts.paths = patharray;
+
+	create_index(checkout_index_entries, 12);
+	git_index_write(g_index);
+
+	cl_git_pass(git_checkout_index(g_repo, g_index, &opts));
+
+	ensure_workdir_contents("conflicting-1.txt", CONFLICTING_DIFF3_FILE);
+	cl_assert(!git_path_exists("merge-resolve/conflicting-2.txt"));
+	ensure_workdir_contents("conflicting-3.txt", AUTOMERGEABLE_MERGED_FILE);
+	cl_assert(!git_path_exists("merge-resolve/conflicting-4.txt"));
+}
+
+static void collect_progress(
+	const char *path,
+	size_t completed_steps,
+	size_t total_steps,
+	void *payload)
+{
+	git_vector *paths = payload;
+
+	if (path == NULL)
+		return;
+
+	git_vector_insert(paths, strdup(path));
+}
+
+void test_checkout_conflict__report_progress(void)
+{
+	git_checkout_opts opts = GIT_CHECKOUT_OPTS_INIT;
+	git_vector paths = GIT_VECTOR_INIT;
+	char *path;
+	size_t i;
+
+	struct checkout_index_entry checkout_index_entries[] = {
+		{ 0100644, CONFLICTING_ANCESTOR_OID, 1, "conflicting-1.txt" },
+		{ 0100644, CONFLICTING_OURS_OID, 2, "conflicting-1.txt" },
+		{ 0100644, CONFLICTING_THEIRS_OID, 3, "conflicting-1.txt" },
+
+		{ 0100644, CONFLICTING_ANCESTOR_OID, 1, "conflicting-2.txt" },
+		{ 0100644, CONFLICTING_OURS_OID, 2, "conflicting-2.txt" },
+		{ 0100644, CONFLICTING_THEIRS_OID, 3, "conflicting-2.txt" },
+
+		{ 0100644, AUTOMERGEABLE_ANCESTOR_OID, 1, "conflicting-3.txt" },
+		{ 0100644, AUTOMERGEABLE_OURS_OID, 2, "conflicting-3.txt" },
+		{ 0100644, AUTOMERGEABLE_THEIRS_OID, 3, "conflicting-3.txt" },
+
+		{ 0100644, AUTOMERGEABLE_ANCESTOR_OID, 1, "conflicting-4.txt" },
+		{ 0100644, AUTOMERGEABLE_OURS_OID, 2, "conflicting-4.txt" },
+		{ 0100644, AUTOMERGEABLE_THEIRS_OID, 3, "conflicting-4.txt" },
+	};
+
+	opts.progress_cb = collect_progress;
+	opts.progress_payload = &paths;
+
+
+	create_index(checkout_index_entries, 12);
+	git_index_write(g_index);
+
+	cl_git_pass(git_checkout_index(g_repo, g_index, &opts));
+
+	cl_assert_equal_i(4, git_vector_length(&paths));
+	cl_assert_equal_s("conflicting-1.txt", git_vector_get(&paths, 0));
+	cl_assert_equal_s("conflicting-2.txt", git_vector_get(&paths, 1));
+	cl_assert_equal_s("conflicting-3.txt", git_vector_get(&paths, 2));
+	cl_assert_equal_s("conflicting-4.txt", git_vector_get(&paths, 3));
+
+	git_vector_foreach(&paths, i, path)
+		git__free(path);
+
+	git_vector_free(&paths);
+}