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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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(-)

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

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

[Johannes Sixt]

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.

-- Hannes

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

[Johannes Schindelin]

Hi,

On Wed, 18 Apr 2007, Johannes Sixt wrote:

> 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

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

[Junio C Hamano]

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(-)

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

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

[Johannes Schindelin]

Hi,

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

Hmm?

Ciao,
Dscho

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

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

hoi :)

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

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> 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'.

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

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

hoi :)

On Wed, Apr 18, 2007 at 08:56:25AM -0700, Junio C Hamano wrote:
> 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Linus Torvalds]

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. 

		Linus

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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.

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

[Linus Torvalds]

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".

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

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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.

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

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> 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(-)

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

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

[Linus Torvalds]

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

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

[Linus Torvalds]

On Wed, 18 Apr 2007, Junio C Hamano wrote:
> 
> 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

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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.

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

[Linus Torvalds]

On Wed, 18 Apr 2007, Junio C Hamano wrote:
> 
> 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

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

[David Lang]

On Wed, 18 Apr 2007, Linus Torvalds wrote:

> 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

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

[Junio C Hamano]

David Lang <david.lang@digitalinsight.com> writes:

> 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.

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

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Wed, Apr 18, 2007 at 01:35:05PM -0700, Linus Torvalds 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

or different attributes for other file types:

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

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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(-)

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