threads / patch / 7724

patch, 2 partsCustom low-level merge driver support.

Subject: [PATCH 0/2] Custom low-level merge driver support.

## tl;dr

23 messages between Apr 18, 2007 and Apr 19, 2007. Diffs are folded; open one to read it.

replies: 22people: 6as markdown or json

Junio C Hamano· Apr 18, 2007, 09:21 UTC · lore

This two-patch series concludes my initial attempt for gitattributes for now.

It hooks into the merge-recursive backend to allow custom low-level 3-way merge drivers to be specified path-by-path basis.

To define a custom low-level merge driver, the usual configuration mechanism is used.

	[merge]
		driver = ancient merge %A %O %B

The new configuration item, 'merge.driver', is multi-valued. Each of its values begin with the name of the low-level driver ('ancient' in the above example), followed by its command line. The command line can contain the following patterns to be processed with the usual interpolation mechanism:

	%O	name of the temporary file that has the
		ancestor's version.
	%A	name of the temporary file that has the
		version from the current branch.
	%B	name of the temporary file that has the
		version from the other branch.

The low-level driver is expected to update the file named with %A with the result of the merge, and exit with zero status upon a clean merge. It can exit with non-zero status to signal that the result still has conflicts.

Once you set up a low-level merge driver, you can specify that driver to be used for specific paths, using the attributes mechanism, by using the driver's name as the value of 'merge' attribute. For example, taken together with the above 'ancient' driver configuration and this line in your .git/info/attributes:

	*.txt	merge=ancient

will use the old "RCS merge" external program to merge a file whose extension is ".txt", just like we used to do before v1.5.0.

Junio C Hamano· Apr 18, 2007, 09:21 UTC · re: Junio C Hamano · lore

[PATCH 1/2] Custom low-level merge driver support.

This allows users to specify custom low-level merge driver per path, using the attributes mechanism. Just like you can specify one of built-in "text", "binary", "union" low-level merge drivers by saying:

	*		merge=text
	.gitignore	merge=union
	*.jpg		merge=binary

pick a name of your favorite merge driver, and assign it as the value of the 'merge' attribute.

A custom low-level merge driver is defined via the config mechanism. This patch introduces 'merge.driver', a multi-valued configuration. Its value is the name (i.e. the one you use as the value of 'merge' attribute) followed by a command line specification. The command line can contain %O, %A, and %B to be interpolated with the names of temporary files that hold the common ancestor version, the version from your branch, and the version from the other branch, and the resulting command is spawned.

The low-level merge driver is expected to update the temporary file for your branch (i.e. %A) with the result and exit with status 0 for a clean merge, and non-zero status for a conflicted merge.

A new test in t6026 demonstrates a sample usage.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 merge-recursive.c     |  177 +++++++++++++++++++++++++++++++++++++++++++++----
 t/t6026-merge-attr.sh |   71 +++++++++++++++++++-
 2 files changed, 235 insertions(+), 13 deletions(-)
Show changes to 2 files +235 −13

merge-recursive.c, t/t6026-merge-attr.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 3b34401..8ec18ad 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -15,6 +15,7 @@
 #include "unpack-trees.h"
 #include "path-list.h"
 #include "xdiff-interface.h"
+#include "interpolate.h"
 #include "attr.h"
 
 static int subtree_merge;
@@ -661,12 +662,14 @@ static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
 }
 
 /* Low-level merge functions */
-typedef int (*ll_merge_fn)(mmfile_t *orig,
+typedef int (*ll_merge_fn)(const char *cmd,
+			   mmfile_t *orig,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
 			   mmbuffer_t *result);
 
-static int ll_xdl_merge(mmfile_t *orig,
+static int ll_xdl_merge(const char *cmd__unused,
+			mmfile_t *orig,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
 			mmbuffer_t *result)
@@ -681,7 +684,8 @@ static int ll_xdl_merge(mmfile_t *orig,
 			 result);
 }
 
-static int ll_union_merge(mmfile_t *orig,
+static int ll_union_merge(const char *cmd__unused,
+			  mmfile_t *orig,
 			  mmfile_t *src1, const char *name1,
 			  mmfile_t *src2, const char *name2,
 			  mmbuffer_t *result)
@@ -690,7 +694,8 @@ static int ll_union_merge(mmfile_t *orig,
 	long size;
 	const int marker_size = 7;
 
-	int status = ll_xdl_merge(orig, src1, NULL, src2, NULL, result);
+	int status = ll_xdl_merge(cmd__unused, orig,
+				  src1, NULL, src2, NULL, result);
 	if (status <= 0)
 		return status;
 	size = result->size;
@@ -721,7 +726,8 @@ static int ll_union_merge(mmfile_t *orig,
 	return 0;
 }
 
-static int ll_binary_merge(mmfile_t *orig,
+static int ll_binary_merge(const char *cmd__unused,
+			   mmfile_t *orig,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
 			   mmbuffer_t *result)
@@ -743,24 +749,169 @@ static struct {
 	const char *name;
 	ll_merge_fn fn;
 } ll_merge_fns[] = {
-	{ "text", ll_xdl_merge },
 	{ "binary", ll_binary_merge },
+	{ "text", ll_xdl_merge },
 	{ "union", ll_union_merge },
 	{ NULL, NULL },
 };
 
-static ll_merge_fn find_ll_merge_fn(void *merge_attr)
+static void create_temp(mmfile_t *src, char *path)
 {
+	int fd;
+
+	strcpy(path, ".merge_file_XXXXXX");
+	fd = mkstemp(path);
+	if (fd < 0)
+		die("unable to create temp-file");
+	if (write_in_full(fd, src->ptr, src->size) != src->size)
+		die("unable to write temp-file");
+	close(fd);
+}
+
+static int ll_ext_merge(const char *cmd,
+			mmfile_t *orig,
+			mmfile_t *src1, const char *name1,
+			mmfile_t *src2, const char *name2,
+			mmbuffer_t *result)
+{
+	char temp[3][50];
+	char cmdbuf[2048];
+	struct interp table[] = {
+		{ "%O" },
+		{ "%A" },
+		{ "%B" },
+	};
+	struct child_process child;
+	const char *args[20];
+	int status, fd, i;
+	struct stat st;
+
+	result->ptr = NULL;
+	result->size = 0;
+	create_temp(orig, temp[0]);
+	create_temp(src1, temp[1]);
+	create_temp(src2, temp[2]);
+
+	interp_set_entry(table, 0, temp[0]);
+	interp_set_entry(table, 1, temp[1]);
+	interp_set_entry(table, 2, temp[2]);
+
+	interpolate(cmdbuf, sizeof(cmdbuf), cmd, table, 3);
+
+	memset(&child, 0, sizeof(child));
+	child.argv = args;
+	args[0] = "sh";
+	args[1] = "-c";
+	args[2] = cmdbuf;
+	args[3] = NULL;
+
+	status = run_command(&child);
+	if (status < -ERR_RUN_COMMAND_FORK)
+		; /* failure in run-command */
+	else
+		status = -status;
+	fd = open(temp[1], O_RDONLY);
+	if (fd < 0)
+		goto bad;
+	if (fstat(fd, &st))
+		goto close_bad;
+	result->size = st.st_size;
+	result->ptr = xmalloc(result->size + 1);
+	if (read_in_full(fd, result->ptr, result->size) != result->size) {
+		free(result->ptr);
+		result->ptr = NULL;
+		result->size = 0;
+	}
+ close_bad:
+	close(fd);
+ bad:
+	for (i = 0; i < 3; i++)
+		unlink(temp[i]);
+	return status;
+}
+
+/*
+ * merge.default and merge.driver configuration items
+ */
+static struct user_merge_fn {
+	struct user_merge_fn *next;
+	const char *name;
+	char *cmdline;
+	char b_[1];
+} *ll_user_merge_fns, **ll_user_merge_fns_tail;
+
+static int read_merge_config(const char *var, const char *value)
+{
+	struct user_merge_fn *fn;
+	int blen, nlen;
+
+	if (strcmp(var, "merge.driver"))
+		return 0;
+	if (!value)
+		return error("%s: lacks value", var);
+	/*
+	 * merge.driver is a multi-valued configuration, whose value is
+	 * of form:
+	 *
+	 *	name command-line
+	 *
+	 * The command-line will be interpolated with the following
+	 * tokens and is given to the shell:
+	 *
+	 *    %O - temporary file name for the merge base.
+	 *    %A - temporary file name for our version.
+	 *    %B - temporary file name for the other branches' version.
+	 *
+	 * The external merge driver should write the results in the file
+	 * named by %A, and signal that it has done with exit status 0.
+	 */
+	for (nlen = -1, blen = 0; value[blen]; blen++)
+		if (nlen < 0 && isspace(value[blen]))
+			nlen = blen;
+	if (nlen < 0)
+		return error("%s '%s': lacks command line", var, value);
+	fn = xcalloc(1, sizeof(struct user_merge_fn) + blen + 1);
+	memcpy(fn->b_, value, blen + 1);
+	fn->name = fn->b_;
+	fn->b_[nlen] = 0;
+	fn->cmdline = fn->b_ + nlen + 1;
+	fn->next = *ll_user_merge_fns_tail;
+	*ll_user_merge_fns_tail = fn;
+	return 0;
+}
+
+static void initialize_ll_merge(void)
+{
+	if (ll_user_merge_fns_tail)
+		return;
+	ll_user_merge_fns_tail = &ll_user_merge_fns;
+	git_config(read_merge_config);
+}
+
+static ll_merge_fn find_ll_merge_fn(void *merge_attr, const char **cmdline)
+{
+	struct user_merge_fn *fn;
 	const char *name;
 	int i;
 
-	if (ATTR_TRUE(merge_attr) || ATTR_UNSET(merge_attr))
+	initialize_ll_merge();
+
+	if (ATTR_TRUE(merge_attr))
 		return ll_xdl_merge;
 	else if (ATTR_FALSE(merge_attr))
 		return ll_binary_merge;
+	else if (ATTR_UNSET(merge_attr))
+		return ll_xdl_merge;
+	else
+		name = merge_attr;
+
+	for (fn = ll_user_merge_fns; fn; fn = fn->next) {
+		if (!strcmp(fn->name, name)) {
+			*cmdline = fn->cmdline;
+			return ll_ext_merge;
+		}
+	}
 
-	/* Otherwise merge_attr may name the merge function */
-	name = merge_attr;
 	for (i = 0; ll_merge_fns[i].name; i++)
 		if (!strcmp(ll_merge_fns[i].name, name))
 			return ll_merge_fns[i].fn;
@@ -793,6 +944,7 @@ static int ll_merge(mmbuffer_t *result_buf,
 	int merge_status;
 	void *merge_attr;
 	ll_merge_fn fn;
+	const char *driver = NULL;
 
 	name1 = xstrdup(mkpath("%s:%s", branch1, a->path));
 	name2 = xstrdup(mkpath("%s:%s", branch2, b->path));
@@ -802,9 +954,10 @@ static int ll_merge(mmbuffer_t *result_buf,
 	fill_mm(b->sha1, &src2);
 
 	merge_attr = git_path_check_merge(a->path);
-	fn = find_ll_merge_fn(merge_attr);
+	fn = find_ll_merge_fn(merge_attr, &driver);
 
-	merge_status = fn(&orig, &src1, name1, &src2, name2, result_buf);
+	merge_status = fn(driver, &orig,
+			  &src1, name1, &src2, name2, result_buf);
 
 	free(name1);
 	free(name2);
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 5daa223..1732b60 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -30,8 +30,9 @@ test_expect_success setup '
 		echo Side >>$f && git add $f || break
 	done &&
 	test_tick &&
-	git commit -m Side
+	git commit -m Side &&
 
+	git tag anchor
 '
 
 test_expect_success merge '
@@ -69,4 +70,72 @@ test_expect_success 'check merge result in working tree' '
 
 '
 
+cat >./custom-merge <<\EOF
+#!/bin/sh
+
+orig="$1" ours="$2" theirs="$3" exit="$4"
+(
+	echo "orig is $orig"
+	echo "ours is $ours"
+	echo "theirs is $theirs"
+	echo "=== orig ==="
+	cat "$orig"
+	echo "=== ours ==="
+	cat "$ours"
+	echo "=== theirs ==="
+	cat "$theirs"
+) >"$ours+"
+cat "$ours+" >"$ours"
+rm -f "$ours+"
+exit "$exit"
+EOF
+chmod +x ./custom-merge
+
+test_expect_success 'custom merge backend' '
+
+	echo "* merge=union" >.gitattributes &&
+	echo "text merge=custom" >>.gitattributes &&
+
+	git reset --hard anchor &&
+	git config --replace-all \
+	merge.driver "custom ./custom-merge %O %A %B 0" &&
+
+	git merge master &&
+
+	cmp binary union &&
+	sed -e 1,3d text >check-1 &&
+	o=$(git-unpack-file master^:text) &&
+	a=$(git-unpack-file side^:text) &&
+	b=$(git-unpack-file master:text) &&
+	sh -c "./custom-merge $o $a $b 0" &&
+	sed -e 1,3d $a >check-2 &&
+	cmp check-1 check-2 &&
+	rm -f $o $a $b
+'
+
+test_expect_success 'custom merge backend' '
+
+	git reset --hard anchor &&
+	git config --replace-all \
+	merge.driver "custom ./custom-merge %O %A %B 1" &&
+
+	if git merge master
+	then
+		echo "Eh? should have conflicted"
+		false
+	else
+		echo "Ok, conflicted"
+	fi &&
+
+	cmp binary union &&
+	sed -e 1,3d text >check-1 &&
+	o=$(git-unpack-file master^:text) &&
+	a=$(git-unpack-file anchor:text) &&
+	b=$(git-unpack-file master:text) &&
+	sh -c "./custom-merge $o $a $b 0" &&
+	sed -e 1,3d $a >check-2 &&
+	cmp check-1 check-2 &&
+	rm -f $o $a $b
+'
+
 test_done
-- 
1.5.1.1.901.g25ba
Johannes Sixt· Apr 18, 2007, 10:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Custom low-level merge driver support.

Junio C Hamano wrote:
Show 8 quoted lines
> +       interpolate(cmdbuf, sizeof(cmdbuf), cmd, table, 3);
> +
> +       memset(&child, 0, sizeof(child));
> +       child.argv = args;
> +       args[0] = "sh";
> +       args[1] = "-c";
> +       args[2] = cmdbuf;
> +       args[3] = NULL;

If I read the code correctly, there does not happen any shell quoting anywhere; hence, this shell invocation is dangerous.

-- Hannes
Johannes Schindelin· Apr 18, 2007, 10:55 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] Custom low-level merge driver support.

Hi,
On Wed, 18 Apr 2007, Johannes Sixt wrote:
Show 12 quoted lines
> Junio C Hamano wrote:
> > +       interpolate(cmdbuf, sizeof(cmdbuf), cmd, table, 3);
> > +
> > +       memset(&child, 0, sizeof(child));
> > +       child.argv = args;
> > +       args[0] = "sh";
> > +       args[1] = "-c";
> > +       args[2] = cmdbuf;
> > +       args[3] = NULL;
> 
> If I read the code correctly, there does not happen any shell quoting
> anywhere; hence, this shell invocation is dangerous.

AFAICT the files used are all temporary files named ".merge_file_xxxx" in the current directory, so there should not be a chance to have spaces or other weird characters in the files.

Ciao, Dscho

Junio C Hamano· Apr 18, 2007, 09:21 UTC · re: Junio C Hamano · lore

[PATCH 2/2] Allow the default low-level merge driver to be configured.

When no 'merge' attribute is given to a path, merge-recursive uses the built-in xdl-merge as the low-level merge driver.

A new configuration item 'merge.default' can name a low-level merge driver of user's choice to be used instead.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 merge-recursive.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
Show changes to merge-recursive.c +12 −2
diff --git a/merge-recursive.c b/merge-recursive.c
index 8ec18ad..5983000 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -839,12 +839,18 @@ static struct user_merge_fn {
 	char *cmdline;
 	char b_[1];
 } *ll_user_merge_fns, **ll_user_merge_fns_tail;
+static const char *default_ll_merge;
 
 static int read_merge_config(const char *var, const char *value)
 {
 	struct user_merge_fn *fn;
 	int blen, nlen;
 
+	if (!strcmp(var, "merge.default")) {
+		default_ll_merge = strdup(value);
+		return 0;
+	}
+
 	if (strcmp(var, "merge.driver"))
 		return 0;
 	if (!value)
@@ -900,8 +906,12 @@ static ll_merge_fn find_ll_merge_fn(void *merge_attr, const char **cmdline)
 		return ll_xdl_merge;
 	else if (ATTR_FALSE(merge_attr))
 		return ll_binary_merge;
-	else if (ATTR_UNSET(merge_attr))
-		return ll_xdl_merge;
+	else if (ATTR_UNSET(merge_attr)) {
+		if (!default_ll_merge)
+			return ll_xdl_merge;
+		else
+			name = default_ll_merge;
+	}
 	else
 		name = merge_attr;
 
-- 
1.5.1.1.901.g25ba
Martin Waitz· Apr 18, 2007, 15:34 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

hoi :)
On Wed, Apr 18, 2007 at 12:48:48PM +0200, Johannes Schindelin wrote:
Show 9 quoted lines
> On Wed, 18 Apr 2007, Junio C Hamano wrote:
> 
> > 	[merge]
> > 		driver = ancient merge %A %O %B
> 
> Why not do something like
> 
> 	[merge.driver]
> 		ancient = merge %A %O %B
or
[merge "ancient"]
	driver = merge %A %O %B
-- 
Martin Waitz
Junio C Hamano· Apr 18, 2007, 15:56 UTC · re: Martin Waitz · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

Martin Waitz <tali@admingilde.org> writes:
Show 15 quoted lines
> On Wed, Apr 18, 2007 at 12:48:48PM +0200, Johannes Schindelin wrote:
>> On Wed, 18 Apr 2007, Junio C Hamano wrote:
>> 
>> > 	[merge]
>> > 		driver = ancient merge %A %O %B
>> 
>> Why not do something like
>> 
>> 	[merge.driver]
>> 		ancient = merge %A %O %B
>
> or
>
> [merge "ancient"]
> 	driver = merge %A %O %B
Actually, I've considered the latter.

Johannes's goes against the usual three-level configuration variable naming rules. If you have 'user definable' part in the name, that should be at the second level.

Which yours does.

The only reason I did not do it your way was because we would need to have three lines per driver (one [merge "foo"] section per driver, a "driver" line, and a blank line for readability after that), which at the time I wrote it felt a bit wasteful, and it was late. But I think I like yours much better.

It probably is trivial to change the rule before it hits 'next'.
Martin Waitz· Apr 18, 2007, 16:08 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

hoi :)
On Wed, Apr 18, 2007 at 08:56:25AM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> The only reason I did not do it your way was because we would
> need to have three lines per driver (one [merge "foo"] section
> per driver, a "driver" line, and a blank line for readability
> after that), which at the time I wrote it felt a bit wasteful,
> and it was late.

perhaps we can extend the config syntax to also support an entry on the same line as the category:

	[merge "ancient"] driver=merge...

This would allow an one-line merge driver definition and would play nice with your [attr] macro definition.

Perhaps we should also add the = to macro definitions?
	[attr] binary=nocrlf nodiff merge=binary

Hmm, but then we would get multiple meanings for "=" on the same line, so nevermind.

Or use some whitespace:
	[attr] binary = nocrlf nodiff merge=binary
Does this look better?
-- 
Martin Waitz
Linus Torvalds· Apr 18, 2007, 16:16 UTC · re: Martin Waitz · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

On Wed, 18 Apr 2007, Martin Waitz wrote:
Show 8 quoted lines
>
> > 	[merge.driver]
> > 		ancient = merge %A %O %B
> 
> or
> 
> [merge "ancient"]
> 	driver = merge %A %O %B
Much better. That format also allows you to add extra flags if you want. 
For example, it might be useful to add something like
	[merge "ancient"]
		name = external three-way merge
		driver = merge %A %O %B

so that you could make the merge process actually say what it is doing when it's merging things. Wouldn't it be nice to see

	merging file xyzzy using external three-way merge
	merging file fax.doc using word file merge
	..
when these things trigger?

So even if we do *not* care about the name, this kind of config structure is just more flexible.

		Linus
Junio C Hamano· Apr 18, 2007, 17:10 UTC · re: Linus Torvalds · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 29 quoted lines
> On Wed, 18 Apr 2007, Martin Waitz wrote:
>>
>> > 	[merge.driver]
>> > 		ancient = merge %A %O %B
>> 
>> or
>> 
>> [merge "ancient"]
>> 	driver = merge %A %O %B
>
> Much better. That format also allows you to add extra flags if you want. 
>
> For example, it might be useful to add something like
>
> 	[merge "ancient"]
> 		name = external three-way merge
> 		driver = merge %A %O %B
>
> so that you could make the merge process actually say what it is doing 
> when it's merging things. Wouldn't it be nice to see
>
> 	merging file xyzzy using external three-way merge
> 	merging file fax.doc using word file merge
> 	..
>
> when these things trigger?
>
> So even if we do *not* care about the name, this kind of config structure 
> is just more flexible. 
Yes.

While I agree with all of the above, I am currently fighting with a back-to-drawing-board design problem.

The series made the low-level 3-way merge machinery customizable, which is a progress, but I think merge-recursive needs a hook to affect middle-level merge decision, similar to what git-merge-one-file does.

I wanted to use 'ours' merge for RelNotes, which typically points at the release notes being prepared for the next release from the branch (Documentation/RelNotes-1.5.1.1.txt for 'maint', and Documentation/RelNotes-1.5.2.txt for 'master'). As I never merge 'master' into 'maint', using 'ours' merge for that path is the right thing to do in this case.

But merge-recursive has a built-in middle-level decision that makes a conflicting symlink modification never go through the low-level 3-way merge codepath.

This was originally not a problem, as the low-level 3-way merge used to be always "textual merge" that would never make sense for symbolic links, but once we allow low-level merge driver that could be "pick one of branches", it becomes needless constraints.

For now, I'd stop at pointing this issue in this message, and finish up the configurability of low-level merge driver first and merge that to 'next'.

But I *think* we would end up revising the driver definition to take more than %A %O %B and allow it to do more than the low-level decision.

Perhaps...
	(in .gitattributes)
	RelNotes	merge=ours
	(in .git/config)
        [merge "ours"]
        	name = middle level 'ours' merge driver
                driver = exit 0
                middlelevel

when "merge.$name.middlelevel" boolean is set, it would forbid process_entry() to make the 'git-merge-one-file' policy decision and always call the driver with these in addition to the %O %A %B:

	%OSHA1	object name for ancestor version (or 0{40} for missing)
        %OMODE	mode bits for ancestor version (or 0{6} for missing)

The driver should be able to express the merge cleanliness via exit status, and the resulting (potentially partial) merge result blob via %A as the low-level driver, but in addition to them it needs to be able to say "the merge result is to remove that path". I haven't figured out what that interface should be; we could designate one special exit code to signal that, perhaps "exit 42", but that feels hacky.

In any case, making [merge "driver-name"] section with 'driver' variable leaves the door open for such extension in a backward compatible way by leaving room to add 'middlelevel' boolean like the above outline, so I'd stop worrying about this for now, without doing anything further.

Linus Torvalds· Apr 18, 2007, 18:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

On Wed, 18 Apr 2007, Junio C Hamano wrote:
Show 8 quoted lines
> 
> While I agree with all of the above, I am currently fighting
> with a back-to-drawing-board design problem.
> 
> The series made the low-level 3-way merge machinery
> customizable, which is a progress, but I think merge-recursive
> needs a hook to affect middle-level merge decision, similar to
> what git-merge-one-file does.

Hmm. I think that it's not so much a "hook", as a "recursive merge is a separate merge policy altogether, that just *defaults* to the same final merge policy".

For example, for a three-way merge, it makes sense to do the three-way merge for the "internal" merge too. But it might be that that isn't true for all strategies. For a "blend" merge, blending at the middle level might be the wrong thing to do because it might duplicate the messages, for example.

So maybe the rule could be:
 - for a file with merge strategy "xyzzy", a recursive merge will first 
   look up the merge machinery for "xyzzy-recursive" and if no such merge 
   strategy exists, it will look up merge machinery for "xyzzy".
 - the final merge will always use merge strategy "xyzzy".

So then, assuming I'm right that the "blend" strategy needs soemthing else in the middle (and I'm not sure I'm right at all, this is purely hypothetical ;), you could have something like

	# a recursive merge will always just pick the ORIGINAL
	# version of a file when blending - the blending will
	# be done only on the final merge
	[merge "blend-recursive"]
		name = recursive blend strategy
		driver = cp %O %A
	[merge "blend"]
		name = blend (changelog) strategy
		driver = blend %A %O %B
or something like that?

I _suspect_ that most merge strategies want to use the same strategy for the recursive merge as for the final one, but I haven't really thought it through.

		Linus
Junio C Hamano· Apr 18, 2007, 19:00 UTC · re: Linus Torvalds · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 13 quoted lines
> On Wed, 18 Apr 2007, Junio C Hamano wrote:
>> 
>> While I agree with all of the above, I am currently fighting
>> with a back-to-drawing-board design problem.
>> 
>> The series made the low-level 3-way merge machinery
>> customizable, which is a progress, but I think merge-recursive
>> needs a hook to affect middle-level merge decision, similar to
>> what git-merge-one-file does.
>
> Hmm. I think that it's not so much a "hook", as a "recursive merge is a 
> separate merge policy altogether, that just *defaults* to the same final 
> merge policy".
I suspect I did not state my problem clearly.
> For example, for a three-way merge, it makes sense to do the three-way 
> merge for the "internal" merge too. But it might be that that isn't true 
> for all strategies.

Yes, that is what I alluded to in one of my previous message, but I think the "foo-recursive" idea is a good one. I was planning to give only "3-way" or "pick ancestor" choice to external low-level merge drivers.

But I think that is a separate issue.

The logic I said I wanted to override in the message you are responding to is the big toplevel if () else if () ... chain in process_entry(), and the nested if () else if () ... logic in merge_file(). They roughly implement what git-merge-one-file does.

Currently low-level 3-way merge drivers are invoked only in some cases, exactly where git-merge-one-file used to invoke "RCS merge". The specific case of RelNotes symlink I wanted to override is one of the cases that ll_merge() is not even invoked.

Junio C Hamano· Apr 18, 2007, 19:23 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

Junio C Hamano <junkio@cox.net> writes:
Show 12 quoted lines
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> For example, for a three-way merge, it makes sense to do the three-way 
>> merge for the "internal" merge too. But it might be that that isn't true 
>> for all strategies.
>
> Yes, that is what I alluded to in one of my previous message,
> but I think the "foo-recursive" idea is a good one.  I was
> planning to give only "3-way" or "pick ancestor" choice to
> external low-level merge drivers.
>
> But I think that is a separate issue.

The syntax is a bit different from the one you came up in your MUA.

	# a recursive merge will always just pick the ORIGINAL
	# version of a file when blending - the blending will
	# be done only on the final merge
	[merge "blend"]
		name = blend (changelog) strategy
		driver = blend %A %O %B
		recursive = binary

The variable merge.<<drivername>>.recursive names a different low level merge driver to be used while performing the virtual ancestor merge. In this example, the built-in "binary" merge which picks 'ours' for the final round and 'origin' for the internal merge already does what we want.

-- >8 -- [PATCH] Allow low-level driver to specify different behaviour during internal merge.

This allows [merge "drivername"] to have a variable "recursive" that names a different low-level merge driver to be used when merging common ancestors to come up with a virtual ancestor.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 merge-recursive.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
Show changes to merge-recursive.c +12 −0
diff --git a/merge-recursive.c b/merge-recursive.c
index 4af69d7..43d6117 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -677,6 +677,7 @@ struct ll_merge_driver {
 	const char *name;
 	const char *description;
 	ll_merge_fn fn;
+	const char *recursive;
 	struct ll_merge_driver *next;
 	char *cmdline;
 };
@@ -934,6 +935,13 @@ static int read_merge_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp("recursive", ep)) {
+		if (!value)
+			return error("%s: lacks value", var);
+		fn->recursive = strdup(value);
+		return 0;
+	}
+
 	return 0;
 }
 
@@ -1013,6 +1021,10 @@ static int ll_merge(mmbuffer_t *result_buf,
 	merge_attr = git_path_check_merge(a->path);
 	driver = find_ll_merge_driver(merge_attr);
 
+	if (index_only && driver->recursive) {
+		merge_attr = git_attr(driver->recursive, strlen(driver->recursive));
+		driver = find_ll_merge_driver(merge_attr);
+	}
 	merge_status = driver->fn(driver, a->path,
 				  &orig, &src1, name1, &src2, name2,
 				  result_buf);
-- 
1.5.1.1.905.g4cad0
Linus Torvalds· Apr 18, 2007, 19:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

On Wed, 18 Apr 2007, Junio C Hamano wrote:
> 
> The syntax is a bit different from the one you came up in your
> MUA.
..and I think yours is superior. Make it so.
		Linus
Linus Torvalds· Apr 18, 2007, 19:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

On Wed, 18 Apr 2007, Junio C Hamano wrote:
Show 8 quoted lines
> 
> But I think that is a separate issue.
> 
> The logic I said I wanted to override in the message you are
> responding to is the big toplevel if () else if () ... chain in
> process_entry(), and the nested if () else if () ... logic in
> merge_file().  They roughly implement what git-merge-one-file
> does.
Ahh. 

Ok, in that case, I kind of disagree with you. I think the hardcoded merging rules (ie when two of the SHA1's match etc) are the only sane things to do _regardless_ of any merge strategy.

So yeah, we'll always do one level of merging at the pure SHA1 stage, and pick that without actually doing any file-level merging at all. I don't think that's wrong. Maybe it means that people cannot pick some really strange merge that they want on a file-by-file basis, but I think that's simply how "merge-recursive" is defined.

IOW, I don't think you should be able to turn
	git merge -s recursive
into
	git merge -s ours
by making attributes say something like
	*: merge=ours

because I think the attributes are really about what we do WHEN we hit file-level conflicts, while the "merge strategy" is a much higher-level thing. They are independent.

But maybe there is some real-world and sane usage that shows me wrong. 
		Linus
Junio C Hamano· Apr 18, 2007, 20:13 UTC · re: Linus Torvalds · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 7 quoted lines
> by making attributes say something like
>
> 	*: merge=ours
>
> because I think the attributes are really about what we do WHEN we hit 
> file-level conflicts, while the "merge strategy" is a much higher-level 
> thing. They are independent.

Yes, but in the case I had trouble with, I know what I want to see happen when we DO hit file-level conflict on RelNotes symlink. The ancestor says Documentation/RelNotes-1.5.0.txt, the maint branch says Documentation/RelNotes-1.5.0.1.txt, while the current branch says Documentation/RelNotes-1.5.1.txt. The logic in merge_file() simply says "we would punt on file-level conflicts for symlinks" without giving a chance for low-level drivers to interfere.

Linus Torvalds· Apr 18, 2007, 20:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

On Wed, 18 Apr 2007, Junio C Hamano wrote:
Show 9 quoted lines
> 
> Yes, but in the case I had trouble with, I know what I want to
> see happen when we DO hit file-level conflict on RelNotes
> symlink.  The ancestor says Documentation/RelNotes-1.5.0.txt,
> the maint branch says Documentation/RelNotes-1.5.0.1.txt, while
> the current branch says Documentation/RelNotes-1.5.1.txt.  The
> logic in merge_file() simply says "we would punt on file-level
> conflicts for symlinks" without giving a chance for low-level
> drivers to interfere.

Ahh, ok. So it really is a file-level conflict, it's just that our traditional merger didn't handle them at all, so we said "nobody can do it". Fair enough. That does sound like a misfeature, although I would also claim that expecting merge strategies to handle symlinks is likely to fail horribly.

So maybe each strategy could have "sub-strategies" for other file types.
Ie something like
	[merge "ours"]
		name = pick our own version
		driver = /bin/true
		symlinks = /bin/true

ie we'd use tyhe "driver" name for regular files, and the "symlinks" name for symlinks and if no "symlinks" entry exists, we error it out as a conflict?

		Linus
David Lang· Apr 18, 2007, 20:09 UTC · re: Linus Torvalds · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

On Wed, 18 Apr 2007, Linus Torvalds wrote:
Show 14 quoted lines
> On Wed, 18 Apr 2007, Junio C Hamano wrote:
>>
> So maybe each strategy could have "sub-strategies" for other file types.
>
> Ie something like
>
> 	[merge "ours"]
> 		name = pick our own version
> 		driver = /bin/true
> 		symlinks = /bin/true
>
> ie we'd use tyhe "driver" name for regular files, and the "symlinks" name
> for symlinks and if no "symlinks" entry exists, we error it out as a
> conflict?
this is starting to sound odd here.
we have .gitattributes define file types and what merge to use on those types

then we have this section define different merges to use on different file types.

I think that we would be better off defining a way for the existing type definitions to include the 'it's a symlink' type of info (and then deal with the merges from there) instead of spreading the tyep info among different sections.

I could see other applications careing if the thing being handed to it is a directory, or a named pipe, etc and wanting different rules for them (obviously this wouldn't be relavent for C source, but I think I could see it for other things)

I coudl also see having one external program that can handle the different types of files and/or merges. rather than having a different line for each would it make sense to add another variable that could be handed to the merge program that would tell it what sort of merge to do?

David Lang
Junio C Hamano· Apr 18, 2007, 21:01 UTC · re: David Lang · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

David Lang <david.lang@digitalinsight.com> writes:
Show 7 quoted lines
> On Wed, 18 Apr 2007, Linus Torvalds wrote:
>
> I could also see having one external program that can handle the
> different types of files and/or merges. rather than having a different
> line for each would it make sense to add another variable that could
> be handed to the merge program that would tell it what sort of merge
> to do?
That was the direction I was thinking about.
Martin Waitz· Apr 19, 2007, 07:52 UTC · re: Linus Torvalds · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

On Wed, Apr 18, 2007 at 01:35:05PM -0700, Linus Torvalds wrote:
Show 8 quoted lines
> So maybe each strategy could have "sub-strategies" for other file types.
> 
> Ie something like
> 
> 	[merge "ours"]
> 		name = pick our own version
> 		driver = /bin/true
> 		symlinks = /bin/true
or different attributes for other file types:

[attr] *@ merge=ours # for symlinks [attr] */ merge=submodule # for dirlinks

-- 
Martin Waitz
Junio C Hamano· Apr 19, 2007, 08:08 UTC · re: Martin Waitz · lore

Re: [PATCH 0/2] Custom low-level merge driver support.

Martin Waitz <tali@admingilde.org> writes:
> or different attributes for other file types:
>
> [attr] *@ merge=ours		# for symlinks
> [attr] */ merge=submodule	# for dirlinks

We already have much more general mechanism to model this on for merges. Look at how git-merge-one-file is invoked. It gets the mode bits which tells what kind of object the path is, for all three (i.e. ancestor, ours, and theirs).

So a single, perhaps not so low-level, merge driver can take the same set of attributes merge-one-file makes use of, not just the current set of %O/%A/%B temporary files, and make intelligent decision to override what is hardcoded in merge-recursive if it wants to.

We do not need to pollute the gitattribute mechanism for this application; it can and should stay about path*name*s.

Junio C Hamano· Apr 18, 2007, 18:28 UTC · re: Linus Torvalds · lore

[PATCH] Custom low-level merge driver: change the configuration scheme.

This changes the configuration syntax for defining a low-level merge driver to be:

	[merge "<<drivername>>"]
		driver = "<<command line>>"
		name = "<<driver description>>"

which is much nicer to read and is extensible. Credit goes to Martin Waitz and Linus.

In addition, when we use an external low-level merge driver, it is reported as an extra output from merge-recursive, using the value of merge.<<drivername>.name variable.

The demonstration in t6026 has also been updated.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * This is on top of what I pushed out on 'pu' last night.  This
   being my git day, I am hoping I can push this out in 'next'
   today.
 merge-recursive.c     |  202 ++++++++++++++++++++++++++++++-------------------
 t/t6026-merge-attr.sh |    8 ++-
 2 files changed, 131 insertions(+), 79 deletions(-)
Show changes to 2 files +131 −79

merge-recursive.c, t/t6026-merge-attr.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 5983000..4af69d7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -661,14 +661,31 @@ static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
 	mm->size = size;
 }
 
-/* Low-level merge functions */
-typedef int (*ll_merge_fn)(const char *cmd,
+/*
+ * Customizable low-level merge drivers support.
+ */
+
+struct ll_merge_driver;
+typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
+			   const char *path,
 			   mmfile_t *orig,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
 			   mmbuffer_t *result);
 
-static int ll_xdl_merge(const char *cmd__unused,
+struct ll_merge_driver {
+	const char *name;
+	const char *description;
+	ll_merge_fn fn;
+	struct ll_merge_driver *next;
+	char *cmdline;
+};
+
+/*
+ * Built-in low-levels
+ */
+static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
+			const char *path_unused,
 			mmfile_t *orig,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
@@ -684,7 +701,8 @@ static int ll_xdl_merge(const char *cmd__unused,
 			 result);
 }
 
-static int ll_union_merge(const char *cmd__unused,
+static int ll_union_merge(const struct ll_merge_driver *drv_unused,
+			  const char *path_unused,
 			  mmfile_t *orig,
 			  mmfile_t *src1, const char *name1,
 			  mmfile_t *src2, const char *name2,
@@ -694,8 +712,8 @@ static int ll_union_merge(const char *cmd__unused,
 	long size;
 	const int marker_size = 7;
 
-	int status = ll_xdl_merge(cmd__unused, orig,
-				  src1, NULL, src2, NULL, result);
+	int status = ll_xdl_merge(drv_unused, path_unused,
+				  orig, src1, NULL, src2, NULL, result);
 	if (status <= 0)
 		return status;
 	size = result->size;
@@ -726,7 +744,8 @@ static int ll_union_merge(const char *cmd__unused,
 	return 0;
 }
 
-static int ll_binary_merge(const char *cmd__unused,
+static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
+			   const char *path_unused,
 			   mmfile_t *orig,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
@@ -745,14 +764,13 @@ static int ll_binary_merge(const char *cmd__unused,
 	return 1;
 }
 
-static struct {
-	const char *name;
-	ll_merge_fn fn;
-} ll_merge_fns[] = {
-	{ "binary", ll_binary_merge },
-	{ "text", ll_xdl_merge },
-	{ "union", ll_union_merge },
-	{ NULL, NULL },
+#define LL_BINARY_MERGE 0
+#define LL_TEXT_MERGE 1
+#define LL_UNION_MERGE 2
+static struct ll_merge_driver ll_merge_drv[] = {
+	{ "binary", "built-in binary merge", ll_binary_merge },
+	{ "text", "built-in 3-way text merge", ll_xdl_merge },
+	{ "union", "built-in union merge", ll_union_merge },
 };
 
 static void create_temp(mmfile_t *src, char *path)
@@ -768,7 +786,11 @@ static void create_temp(mmfile_t *src, char *path)
 	close(fd);
 }
 
-static int ll_ext_merge(const char *cmd,
+/*
+ * User defined low-level merge driver support.
+ */
+static int ll_ext_merge(const struct ll_merge_driver *fn,
+			const char *path,
 			mmfile_t *orig,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
@@ -796,7 +818,10 @@ static int ll_ext_merge(const char *cmd,
 	interp_set_entry(table, 1, temp[1]);
 	interp_set_entry(table, 2, temp[2]);
 
-	interpolate(cmdbuf, sizeof(cmdbuf), cmd, table, 3);
+	output(1, "merging %s using %s", path,
+	       fn->description ? fn->description : fn->name);
+
+	interpolate(cmdbuf, sizeof(cmdbuf), fn->cmdline, table, 3);
 
 	memset(&child, 0, sizeof(child));
 	child.argv = args;
@@ -833,101 +858,124 @@ static int ll_ext_merge(const char *cmd,
 /*
  * merge.default and merge.driver configuration items
  */
-static struct user_merge_fn {
-	struct user_merge_fn *next;
-	const char *name;
-	char *cmdline;
-	char b_[1];
-} *ll_user_merge_fns, **ll_user_merge_fns_tail;
+static struct ll_merge_driver *ll_user_merge, **ll_user_merge_tail;
 static const char *default_ll_merge;
 
 static int read_merge_config(const char *var, const char *value)
 {
-	struct user_merge_fn *fn;
-	int blen, nlen;
+	struct ll_merge_driver *fn;
+	const char *ep, *name;
+	int namelen;
 
 	if (!strcmp(var, "merge.default")) {
-		default_ll_merge = strdup(value);
+		if (value)
+			default_ll_merge = strdup(value);
 		return 0;
 	}
 
-	if (strcmp(var, "merge.driver"))
+	/*
+	 * We are not interested in anything but "merge.<name>.variable";
+	 * especially, we do not want to look at variables such as
+	 * "merge.summary", "merge.tool", and "merge.verbosity".
+	 */
+	if (prefixcmp(var, "merge.") || (ep = strchr(var + 6, '.')) == NULL)
 		return 0;
-	if (!value)
-		return error("%s: lacks value", var);
+
 	/*
-	 * merge.driver is a multi-valued configuration, whose value is
-	 * of form:
-	 *
-	 *	name command-line
-	 *
-	 * The command-line will be interpolated with the following
-	 * tokens and is given to the shell:
-	 *
-	 *    %O - temporary file name for the merge base.
-	 *    %A - temporary file name for our version.
-	 *    %B - temporary file name for the other branches' version.
-	 *
-	 * The external merge driver should write the results in the file
-	 * named by %A, and signal that it has done with exit status 0.
+	 * Find existing one as we might be processing merge.<name>.var2
+	 * after seeing merge.<name>.var1.
 	 */
-	for (nlen = -1, blen = 0; value[blen]; blen++)
-		if (nlen < 0 && isspace(value[blen]))
-			nlen = blen;
-	if (nlen < 0)
-		return error("%s '%s': lacks command line", var, value);
-	fn = xcalloc(1, sizeof(struct user_merge_fn) + blen + 1);
-	memcpy(fn->b_, value, blen + 1);
-	fn->name = fn->b_;
-	fn->b_[nlen] = 0;
-	fn->cmdline = fn->b_ + nlen + 1;
-	fn->next = *ll_user_merge_fns_tail;
-	*ll_user_merge_fns_tail = fn;
+	name = var + 6;
+	namelen = ep - name;
+	for (fn = ll_user_merge; fn; fn = fn->next)
+		if (!strncmp(fn->name, name, namelen) && !fn->name[namelen])
+			break;
+	if (!fn) {
+		char *namebuf;
+		fn = xcalloc(1, sizeof(struct ll_merge_driver));
+		namebuf = xmalloc(namelen + 1);
+		memcpy(namebuf, name, namelen);
+		namebuf[namelen] = 0;
+		fn->name = namebuf;
+		fn->fn = ll_ext_merge;
+		fn->next = *ll_user_merge_tail;
+		*ll_user_merge_tail = fn;
+	}
+
+	ep++;
+
+	if (!strcmp("name", ep)) {
+		if (!value)
+			return error("%s: lacks value", var);
+		fn->description = strdup(value);
+		return 0;
+	}
+
+	if (!strcmp("driver", ep)) {
+		if (!value)
+			return error("%s: lacks value", var);
+		/*
+		 * merge.<name>.driver specifies the command line:
+		 *
+		 *	command-line
+		 *
+		 * The command-line will be interpolated with the following
+		 * tokens and is given to the shell:
+		 *
+		 *    %O - temporary file name for the merge base.
+		 *    %A - temporary file name for our version.
+		 *    %B - temporary file name for the other branches' version.
+		 *
+		 * The external merge driver should write the results in the
+		 * file named by %A, and signal that it has done with zero exit
+		 * status.
+		 */
+		fn->cmdline = strdup(value);
+		return 0;
+	}
+
 	return 0;
 }
 
 static void initialize_ll_merge(void)
 {
-	if (ll_user_merge_fns_tail)
+	if (ll_user_merge_tail)
 		return;
-	ll_user_merge_fns_tail = &ll_user_merge_fns;
+	ll_user_merge_tail = &ll_user_merge;
 	git_config(read_merge_config);
 }
 
-static ll_merge_fn find_ll_merge_fn(void *merge_attr, const char **cmdline)
+static const struct ll_merge_driver *find_ll_merge_driver(void *merge_attr)
 {
-	struct user_merge_fn *fn;
+	struct ll_merge_driver *fn;
 	const char *name;
 	int i;
 
 	initialize_ll_merge();
 
 	if (ATTR_TRUE(merge_attr))
-		return ll_xdl_merge;
+		return &ll_merge_drv[LL_TEXT_MERGE];
 	else if (ATTR_FALSE(merge_attr))
-		return ll_binary_merge;
+		return &ll_merge_drv[LL_BINARY_MERGE];
 	else if (ATTR_UNSET(merge_attr)) {
 		if (!default_ll_merge)
-			return ll_xdl_merge;
+			return &ll_merge_drv[LL_TEXT_MERGE];
 		else
 			name = default_ll_merge;
 	}
 	else
 		name = merge_attr;
 
-	for (fn = ll_user_merge_fns; fn; fn = fn->next) {
-		if (!strcmp(fn->name, name)) {
-			*cmdline = fn->cmdline;
-			return ll_ext_merge;
-		}
-	}
+	for (fn = ll_user_merge; fn; fn = fn->next)
+		if (!strcmp(fn->name, name))
+			return fn;
 
-	for (i = 0; ll_merge_fns[i].name; i++)
-		if (!strcmp(ll_merge_fns[i].name, name))
-			return ll_merge_fns[i].fn;
+	for (i = 0; i < ARRAY_SIZE(ll_merge_drv); i++)
+		if (!strcmp(ll_merge_drv[i].name, name))
+			return &ll_merge_drv[i];
 
 	/* default to the 3-way */
-	return ll_xdl_merge;
+	return &ll_merge_drv[LL_TEXT_MERGE];
 }
 
 static void *git_path_check_merge(const char *path)
@@ -953,8 +1001,7 @@ static int ll_merge(mmbuffer_t *result_buf,
 	char *name1, *name2;
 	int merge_status;
 	void *merge_attr;
-	ll_merge_fn fn;
-	const char *driver = NULL;
+	const struct ll_merge_driver *driver;
 
 	name1 = xstrdup(mkpath("%s:%s", branch1, a->path));
 	name2 = xstrdup(mkpath("%s:%s", branch2, b->path));
@@ -964,10 +1011,11 @@ static int ll_merge(mmbuffer_t *result_buf,
 	fill_mm(b->sha1, &src2);
 
 	merge_attr = git_path_check_merge(a->path);
-	fn = find_ll_merge_fn(merge_attr, &driver);
+	driver = find_ll_merge_driver(merge_attr);
 
-	merge_status = fn(driver, &orig,
-			  &src1, name1, &src2, name2, result_buf);
+	merge_status = driver->fn(driver, a->path,
+				  &orig, &src1, name1, &src2, name2,
+				  result_buf);
 
 	free(name1);
 	free(name2);
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 1732b60..56fc341 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -98,7 +98,9 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.driver "custom ./custom-merge %O %A %B 0" &&
+	merge.custom.driver "./custom-merge %O %A %B 0" &&
+	git config --replace-all \
+	merge.custom.name "custom merge driver for testing" &&
 
 	git merge master &&
 
@@ -117,7 +119,9 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.driver "custom ./custom-merge %O %A %B 1" &&
+	merge.custom.driver "./custom-merge %O %A %B 1" &&
+	git config --replace-all \
+	merge.custom.name "custom merge driver for testing" &&
 
 	if git merge master
 	then
-- 
1.5.1.1.901.g25ba

← back to recent threads