merge driver: remove `check` callback

Since the `apply` callback can defer, the `check` callback is not
necessary.  Removing the `check` callback further makes the `payload`
unnecessary along with the `cleanup` callback.

include/git2/sys/merge.h

@@ -83,41 +83,7 @@ typedef int (*git_merge_driver_init_fn)(git_merge_driver *self);
 typedef void (*git_merge_driver_shutdown_fn)(git_merge_driver *self);
 
 /**
- * Callback to decide if a given conflict can be resolved with this merge
- * driver.
- *
- * Specified as `driver.check`, this is an optional callback that checks
- * if the given conflict can be resolved with this merge driver.
- *
- * It should return 0 if the merge driver should be applied (i.e. success),
- * `GIT_PASSTHROUGH` if the driver is not available, which is the equivalent
- * of an unregistered or nonexistent merge driver.  In this case, the default
- * (`text`) driver will be run.  This is useful if you register a wildcard
- * merge driver but are not interested in handling the requested file (and
- * should just fallback).  The driver can also return `GIT_EMERGECONFLICT`
- * if the driver is not able to produce a merge result, and the file will
- * remain conflicted.  Any other errors will fail and return to the caller.
- *
- * The `name` will be set to the name of the driver as configured in the
- * attributes.
- *
- * The `src` contains the data about the file to be merged.
- *
- * The `payload` will be a pointer to a reference payload for the driver.
- * This will start as NULL, but `check` can assign to this pointer for
- * later use by the `apply` callback.  Note that the value should be heap
- * allocated (not stack), so that it doesn't go away before the `apply`
- * callback can use it.  If a driver allocates and assigns a value to the
- * `payload`, it will need a `cleanup` callback to free the payload.
- */
-typedef int (*git_merge_driver_check_fn)(
-	git_merge_driver *self,
-	void **payload,
-	const char *name,
-	const git_merge_driver_source *src);
-
-/**
- * Callback to actually perform the merge.
+ * Callback to perform the merge.
  *
  * Specified as `driver.apply`, this is the callback that actually does the
  * merge.  If it can successfully perform a merge, it should populate
@@ -129,32 +95,20 @@ typedef int (*git_merge_driver_check_fn)(
  * and the file will remain conflicted.  Any other errors will fail and
  * return to the caller.
  *
- * The `src` contains the data about the file to be merged.
+ * The `filter_name` contains the name of the filter that was invoked, as
+ * specified by the file's attributes.
  *
- * The `payload` value will refer to any payload that was set by the
- * `check` callback.  It may be read from or written to as needed.
+ * The `src` contains the data about the file to be merged.
  */
 typedef int (*git_merge_driver_apply_fn)(
 	git_merge_driver *self,
-	void **payload,
 	const char **path_out,
 	uint32_t *mode_out,
 	git_buf *merged_out,
+	const char *filter_name,
 	const git_merge_driver_source *src);
 
 /**
- * Callback to clean up after merge has been performed.
- *
- * Specified as `driver.cleanup`, this is an optional callback invoked
- * after the driver has been run.  If the `check` or `apply` callbacks
- * allocated a `payload` to keep per-source merge driver state, use this
- * callback to free that payload and release resources as required.
- */
-typedef void (*git_merge_driver_cleanup_fn)(
-	git_merge_driver *self,
-	void *payload);
-
-/**
  * Merge driver structure used to register custom merge drivers.
  *
  * To associate extra data with a driver, allocate extra data and put the
@@ -172,22 +126,12 @@ struct git_merge_driver {
 	git_merge_driver_shutdown_fn shutdown;
 
 	/**
-	 * Called to determine whether the merge driver should be invoked
-	 * for a given file.  If this function returns `GIT_PASSTHROUGH`
-	 * then the `apply` function will not be invoked and the default
-	 * (`text`) merge driver will instead be run.
-	 */
-	git_merge_driver_check_fn    check;
-
-	/**
 	 * Called to merge the contents of a conflict.  If this function
 	 * returns `GIT_PASSTHROUGH` then the default (`text`) merge driver
 	 * will instead be invoked.  If this function returns
 	 * `GIT_EMERGECONFLICT` then the file will remain conflicted.
+	 */
 	git_merge_driver_apply_fn    apply;
-
-	/** Called when the system is done filtering for a file. */
-	git_merge_driver_cleanup_fn  cleanup;
 };
 
 #define GIT_MERGE_DRIVER_VERSION 1

src/merge.c

@@ -838,10 +838,10 @@ static bool merge_conflict_can_resolve_contents(
 
 static int merge_conflict_invoke_driver(
 	git_index_entry **out,
+	const char *name,
 	git_merge_driver *driver,
-	void *data,
 	git_merge_diff_list *diff_list,
-	git_merge_driver_source *source)
+	git_merge_driver_source *src)
 {
 	git_index_entry *result;
 	git_buf buf = GIT_BUF_INIT;
@@ -853,10 +853,8 @@ static int merge_conflict_invoke_driver(
 
 	*out = NULL;
 
-	if ((error = driver->apply(driver, &data, &path, &mode, &buf, source)) < 0)
-		goto done;
-
-	if ((error = git_repository_odb(&odb, source->repo)) < 0 ||
+	if ((error = driver->apply(driver, &path, &mode, &buf, name, src)) < 0 ||
+		(error = git_repository_odb(&odb, src->repo)) < 0 ||
 		(error = git_odb_write(&oid, odb, buf.ptr, buf.size, GIT_OBJ_BLOB)) < 0)
 		goto done;
 
@@ -873,9 +871,6 @@ static int merge_conflict_invoke_driver(
 	*out = result;
 
 done:
-	if (driver->cleanup)
-		driver->cleanup(driver, data);
-
 	git_buf_free(&buf);
 	git_odb_free(odb);
 
@@ -892,9 +887,10 @@ static int merge_conflict_resolve_contents(
 	git_merge_driver_source source = {0};
 	git_merge_file_result result = {0};
 	git_merge_driver *driver;
+	git_merge_driver__builtin builtin = {{0}};
 	git_index_entry *merge_result;
 	git_odb *odb = NULL;
-	void *data;
+	const char *name;
 	int error = 0;
 
 	assert(resolved && diff_list && conflict);
@@ -916,23 +912,26 @@ static int merge_conflict_resolve_contents(
 
 	if (file_opts->favor != GIT_MERGE_FILE_FAVOR_NORMAL) {
 		/* if the user requested a particular type of resolution (via the
-		 * favor flag) then let that override the gitattributes.
+		 * favor flag) then let that override the gitattributes and use
+		 * the builtin driver.
 		 */
-		driver = &git_merge_driver__normal;
-		data = (void **)&file_opts->favor;
+		name = "text";
+		builtin.base.apply = git_merge_driver__builtin_apply;
+		builtin.favor = file_opts->favor;
+
+		driver = &builtin.base;
 	} else {
 		/* find the merge driver for this file */
-		if ((error = git_merge_driver_for_source(&driver, &data, &source)) < 0)
+		if ((error = git_merge_driver_for_source(&name, &driver, &source)) < 0)
 			goto done;
 	}
 
-	error = merge_conflict_invoke_driver(&merge_result,
-		driver, data, diff_list, &source);
+	error = merge_conflict_invoke_driver(&merge_result, name, driver,
+		diff_list, &source);
 
 	if (error == GIT_PASSTHROUGH) {
-		data = NULL;
-		error = merge_conflict_invoke_driver(&merge_result,
-			&git_merge_driver__text, data, diff_list, &source);
+		error = merge_conflict_invoke_driver(&merge_result, "text",
+			&git_merge_driver__text.base, diff_list, &source);
 	}
 
 	if (error < 0) {

src/merge.h

@@ -145,12 +145,6 @@ int git_merge_diff_list__find_renames(git_repository *repo, git_merge_diff_list
 
 void git_merge_diff_list__free(git_merge_diff_list *diff_list);
 
-/* Merge driver configuration */
-int git_merge_driver_for_source(
-	git_merge_driver **driver_out,
-	void **data_out,
-	const git_merge_driver_source *src);
-
 /* Merge metadata setup */
 
 int git_merge__setup(

src/merge_driver.c

@@ -30,32 +30,29 @@ typedef struct {
 
 static struct merge_driver_registry merge_driver_registry;
 
-static git_merge_file_favor_t merge_favor_normal = GIT_MERGE_FILE_FAVOR_NORMAL;
-static git_merge_file_favor_t merge_favor_union = GIT_MERGE_FILE_FAVOR_UNION;
-
 static void git_merge_driver_global_shutdown(void);
 
 
-static int merge_driver_apply(
+int git_merge_driver__builtin_apply(
 	git_merge_driver *self,
-	void **payload,
 	const char **path_out,
 	uint32_t *mode_out,
 	git_buf *merged_out,
+	const char *filter_name,
 	const git_merge_driver_source *src)
 {
+	git_merge_driver__builtin *driver = (git_merge_driver__builtin *)self;
 	git_merge_file_options file_opts = GIT_MERGE_FILE_OPTIONS_INIT;
-	git_merge_file_favor_t *favor = (git_merge_file_favor_t *) *payload;
 	git_merge_file_result result = {0};
 	int error;
 
-	GIT_UNUSED(self);
+	GIT_UNUSED(filter_name);
 
 	if (src->file_opts)
 		memcpy(&file_opts, src->file_opts, sizeof(git_merge_file_options));
 
-	if (favor)
-		file_opts.favor = *favor;
+	if (driver->favor)
+		file_opts.favor = driver->favor;
 
 	if ((error = git_merge_file_from_index(&result, src->repo,
 		src->ancestor, src->ours, src->theirs, &file_opts)) < 0)
@@ -87,47 +84,19 @@ done:
 	return error;
 }
 
-static int merge_driver_text_check(
-	git_merge_driver *self,
-	void **payload,
-	const char *name,
-	const git_merge_driver_source *src)
-{
-	GIT_UNUSED(self);
-	GIT_UNUSED(name);
-	GIT_UNUSED(src);
-
-	*payload = &merge_favor_normal;
-	return 0;
-}
-
-static int merge_driver_union_check(
-	git_merge_driver *self,
-	void **payload,
-	const char *name,
-	const git_merge_driver_source *src)
-{
-	GIT_UNUSED(self);
-	GIT_UNUSED(name);
-	GIT_UNUSED(src);
-
-	*payload = &merge_favor_union;
-	return 0;
-}
-
 static int merge_driver_binary_apply(
 	git_merge_driver *self,
-	void **payload,
 	const char **path_out,
 	uint32_t *mode_out,
 	git_buf *merged_out,
+	const char *filter_name,
 	const git_merge_driver_source *src)
 {
 	GIT_UNUSED(self);
-	GIT_UNUSED(payload);
 	GIT_UNUSED(path_out);
 	GIT_UNUSED(mode_out);
 	GIT_UNUSED(merged_out);
+	GIT_UNUSED(filter_name);
 	GIT_UNUSED(src);
 
 	return GIT_EMERGECONFLICT;
@@ -149,35 +118,30 @@ static int merge_driver_entry_search(const void *a, const void *b)
 	return strcmp(name_a, entry_b->name);
 }
 
-git_merge_driver git_merge_driver__normal = {
-	GIT_MERGE_DRIVER_VERSION,
-	NULL,
-	NULL,
-	NULL,
-	merge_driver_apply
-};
-
-git_merge_driver git_merge_driver__text = {
-	GIT_MERGE_DRIVER_VERSION,
-	NULL,
-	NULL,
-	merge_driver_text_check,
-	merge_driver_apply
+git_merge_driver__builtin git_merge_driver__text = {
+	{
+		GIT_MERGE_DRIVER_VERSION,
+		NULL,
+		NULL,
+		git_merge_driver__builtin_apply,
+	},
+	GIT_MERGE_FILE_FAVOR_NORMAL
 };
 
-git_merge_driver git_merge_driver__union = {
-	GIT_MERGE_DRIVER_VERSION,
-	NULL,
-	NULL,
-	merge_driver_union_check,
-	merge_driver_apply
+git_merge_driver__builtin git_merge_driver__union = {
+	{
+		GIT_MERGE_DRIVER_VERSION,
+		NULL,
+		NULL,
+		git_merge_driver__builtin_apply,
+	},
+	GIT_MERGE_FILE_FAVOR_UNION
 };
 
 git_merge_driver git_merge_driver__binary = {
 	GIT_MERGE_DRIVER_VERSION,
 	NULL,
 	NULL,
-	NULL,
 	merge_driver_binary_apply
 };
 
@@ -209,9 +173,9 @@ int git_merge_driver_global_init(void)
 		goto done;
 
 	if ((error = merge_driver_registry_insert(
-			merge_driver_name__text, &git_merge_driver__text)) < 0 ||
+			merge_driver_name__text, &git_merge_driver__text.base)) < 0 ||
 		(error = merge_driver_registry_insert(
-			merge_driver_name__union, &git_merge_driver__union)) < 0 ||
+			merge_driver_name__union, &git_merge_driver__union.base)) < 0 ||
 		(error = merge_driver_registry_insert(
 			merge_driver_name__binary, &git_merge_driver__binary)) < 0)
 
@@ -333,7 +297,7 @@ git_merge_driver *git_merge_driver_lookup(const char *name)
 	 * to take a lock and look it up in the vector.
 	 */
 	if (name == merge_driver_name__text)
-		return &git_merge_driver__text;
+		return &git_merge_driver__text.base;
 	else if (name == merge_driver_name__binary)
 		return &git_merge_driver__binary;
 
@@ -409,13 +373,11 @@ GIT_INLINE(git_merge_driver *) merge_driver_lookup_with_wildcard(
 }
 
 int git_merge_driver_for_source(
+	const char **name_out,
 	git_merge_driver **driver_out,
-	void **data_out,
 	const git_merge_driver_source *src)
 {
 	const char *path, *driver_name;
-	git_merge_driver *driver;
-	void *data = NULL;
 	int error = 0;
 
 	path = git_merge_file__best_path(
@@ -427,31 +389,8 @@ int git_merge_driver_for_source(
 			&driver_name, src->repo, path, src->default_driver)) < 0)
 		return error;
 
-	driver = merge_driver_lookup_with_wildcard(driver_name);
-
-	if (driver && driver->check) {
-		error = driver->check(driver, &data, driver_name, src);
-
-		if (error == GIT_PASSTHROUGH)
-			driver = &git_merge_driver__text;
-		else if (error == GIT_EMERGECONFLICT)
-			driver = &git_merge_driver__binary;
-		else
-			goto done;
-	}
-
-	error = 0;
-	data = NULL;
-
-	if (driver->check)
-		error = driver->check(driver, &data, driver_name, src);
-
-	/* the text and binary drivers must succeed their check */
-	assert(error == 0);
-
-done:
-	*driver_out = driver;
-	*data_out = data;
+	*name_out = driver_name;
+	*driver_out = merge_driver_lookup_with_wildcard(driver_name);
 	return error;
 }
 

src/merge_driver.h

@@ -21,6 +21,11 @@ struct git_merge_driver_source {
 	const git_index_entry *theirs;
 };
 
+typedef struct git_merge_driver__builtin {
+	git_merge_driver base;
+	git_merge_file_favor_t favor;
+} git_merge_driver__builtin;
+
 extern int git_merge_driver_global_init(void);
 
 extern int git_merge_driver_for_path(
@@ -29,14 +34,25 @@ extern int git_merge_driver_for_path(
 	git_repository *repo,
 	const char *path);
 
-/* Basic (normal) merge driver, takes favor type as the payload argument */
-extern git_merge_driver git_merge_driver__normal;
+/* Merge driver configuration */
+extern int git_merge_driver_for_source(
+	const char **name_out,
+	git_merge_driver **driver_out,
+	const git_merge_driver_source *src);
+
+extern int git_merge_driver__builtin_apply(
+	git_merge_driver *self,
+	const char **path_out,
+	uint32_t *mode_out,
+	git_buf *merged_out,
+	const char *filter_name,
+	const git_merge_driver_source *src);
 
 /* Merge driver for text files, performs a standard three-way merge */
-extern git_merge_driver git_merge_driver__text;
+extern git_merge_driver__builtin git_merge_driver__text;
 
 /* Merge driver for union-style merging */
-extern git_merge_driver git_merge_driver__union;
+extern git_merge_driver__builtin git_merge_driver__union;
 
 /* Merge driver for unmergeable (binary) files: always produces conflicts */
 extern git_merge_driver git_merge_driver__binary;

tests/merge/driver.c

@@ -63,27 +63,12 @@ static void test_driver_shutdown(git_merge_driver *s)
 	self->shutdown = 1;
 }
 
-static int test_driver_check(
-	git_merge_driver *s,
-	void **payload,
-	const char *name,
-	const git_merge_driver_source *src)
-{
-	GIT_UNUSED(s);
-	GIT_UNUSED(src);
-
-	*payload = git__strdup(name);
-	GITERR_CHECK_ALLOC(*payload);
-
-	return 0;
-}
-
 static int test_driver_apply(
 	git_merge_driver *s,
-	void **payload,
 	const char **path_out,
 	uint32_t *mode_out,
 	git_buf *merged_out,
+	const char *filter_name,
 	const git_merge_driver_source *src)
 {
 	GIT_UNUSED(s);
@@ -93,25 +78,15 @@ static int test_driver_apply(
 	*mode_out = GIT_FILEMODE_BLOB;
 
 	return git_buf_printf(merged_out, "This is the `%s` driver.\n",
-		(char *)*payload);
+		filter_name);
 }
 
-static void test_driver_cleanup(git_merge_driver *s, void *payload)
-{
-	GIT_UNUSED(s);
-
-	git__free(payload);
-}
-
-
 static struct test_merge_driver test_driver_custom = {
 	{
 		GIT_MERGE_DRIVER_VERSION,
 		test_driver_init,
 		test_driver_shutdown,
-		test_driver_check,
 		test_driver_apply,
-		test_driver_cleanup
 	},
 	0,
 	0,
@@ -122,9 +97,7 @@ static struct test_merge_driver test_driver_wildcard = {
 		GIT_MERGE_DRIVER_VERSION,
 		test_driver_init,
 		test_driver_shutdown,
-		test_driver_check,
 		test_driver_apply,
-		test_driver_cleanup
 	},
 	0,
 	0,
@@ -219,62 +192,19 @@ void test_merge_driver__shutdown_is_called(void)
 	test_drivers_register();
 }
 
-static int defer_driver_check(
-	git_merge_driver *s,
-	void **payload,
-	const char *name,
-	const git_merge_driver_source *src)
-{
-	GIT_UNUSED(s);
-	GIT_UNUSED(payload);
-	GIT_UNUSED(name);
-	GIT_UNUSED(src);
-
-	return GIT_PASSTHROUGH;
-}
-
-static struct test_merge_driver test_driver_defer_check = {
-	{
-		GIT_MERGE_DRIVER_VERSION,
-		test_driver_init,
-		test_driver_shutdown,
-		defer_driver_check,
-		test_driver_apply,
-		test_driver_cleanup
-	},
-	0,
-	0,
-};
-
-void test_merge_driver__check_can_defer(void)
-{
-	const git_index_entry *idx;
-
-	cl_git_pass(git_merge_driver_register("defer",
-		&test_driver_defer_check.base));
-
-    set_gitattributes_to("defer");
-    merge_branch();
-
-	cl_assert((idx = git_index_get_bypath(repo_index, "automergeable.txt", 0)));
-	cl_assert_equal_oid(&automergeable_id, &idx->id);
-
-	git_merge_driver_unregister("defer");
-}
-
 static int defer_driver_apply(
 	git_merge_driver *s,
-	void **payload,
 	const char **path_out,
 	uint32_t *mode_out,
 	git_buf *merged_out,
+	const char *filter_name,
 	const git_merge_driver_source *src)
 {
 	GIT_UNUSED(s);
-	GIT_UNUSED(payload);
 	GIT_UNUSED(path_out);
 	GIT_UNUSED(mode_out);
 	GIT_UNUSED(merged_out);
+	GIT_UNUSED(filter_name);
 	GIT_UNUSED(src);
 
 	return GIT_PASSTHROUGH;
@@ -285,9 +215,7 @@ static struct test_merge_driver test_driver_defer_apply = {
 		GIT_MERGE_DRIVER_VERSION,
 		test_driver_init,
 		test_driver_shutdown,
-		test_driver_check,
 		defer_driver_apply,
-		test_driver_cleanup
 	},
 	0,
 	0,
@@ -309,62 +237,19 @@ void test_merge_driver__apply_can_defer(void)
 	git_merge_driver_unregister("defer");
 }
 
-static int conflict_driver_check(
-	git_merge_driver *s,
-	void **payload,
-	const char *name,
-	const git_merge_driver_source *src)
-{
-	GIT_UNUSED(s);
-	GIT_UNUSED(payload);
-	GIT_UNUSED(name);
-	GIT_UNUSED(src);
-
-	return GIT_EMERGECONFLICT;
-}
-
-static struct test_merge_driver test_driver_conflict_check = {
-	{
-		GIT_MERGE_DRIVER_VERSION,
-		test_driver_init,
-		test_driver_shutdown,
-		conflict_driver_check,
-		test_driver_apply,
-		test_driver_cleanup
-	},
-	0,
-	0,
-};
-
-void test_merge_driver__check_can_conflict(void)
-{
-	const git_index_entry *ancestor, *ours, *theirs;
-
-	cl_git_pass(git_merge_driver_register("conflict",
-		&test_driver_conflict_check.base));
-
-    set_gitattributes_to("conflict");
-    merge_branch();
-
-	cl_git_pass(git_index_conflict_get(&ancestor, &ours, &theirs,
-		repo_index, "automergeable.txt"));
-
-	git_merge_driver_unregister("conflict");
-}
-
 static int conflict_driver_apply(
 	git_merge_driver *s,
-	void **payload,
 	const char **path_out,
 	uint32_t *mode_out,
 	git_buf *merged_out,
+	const char *filter_name,
 	const git_merge_driver_source *src)
 {
 	GIT_UNUSED(s);
-	GIT_UNUSED(payload);
 	GIT_UNUSED(path_out);
 	GIT_UNUSED(mode_out);
 	GIT_UNUSED(merged_out);
+	GIT_UNUSED(filter_name);
 	GIT_UNUSED(src);
 
 	return GIT_EMERGECONFLICT;
@@ -375,9 +260,7 @@ static struct test_merge_driver test_driver_conflict_apply = {
 		GIT_MERGE_DRIVER_VERSION,
 		test_driver_init,
 		test_driver_shutdown,
-		test_driver_check,
 		conflict_driver_apply,
-		test_driver_cleanup
 	},
 	0,
 	0,
@@ -447,7 +330,7 @@ void test_merge_driver__mergedefault_deferring_falls_back_to_text(void)
 	const git_index_entry *idx;
 
 	cl_git_pass(git_merge_driver_register("defer",
-		&test_driver_defer_check.base));
+		&test_driver_defer_apply.base));
 
 	cl_repo_set_string(repo, "merge.default", "defer");
 	merge_branch();