[PATCH] Use diff3 instead of merge in merge-recursive.

[PATCH] Use diff3 instead of merge in merge-recursive.

[Uwe Zeisberger]

If no error occurs, merge (from rcs 5.7) is nothing but:

	diff3 -E -am -L label1 -L label2 -L label3 file1 file2 file3 > tmpfile
	cat tmpfile > file1

Using diff3 directly saves one fork per conflicting file.

Signed-off-by: Uwe Zeisberger <zeisberg@informatik.uni-freiburg.de>
---
 merge-recursive.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

It passes `make test` when NO_SVN_TESTS is defined.  (I'll write a
separate mail about that.)

I didn't made any timing tests or further tests for correctness, but I
hope Johannes still has the framework from the time when he converted
the Python script to C?  

@Johannes: If so, could you test this patch?

Best regards
Uwe

diff --git a/merge-recursive.c b/merge-recursive.c
index 2ba43ae..9e3f9d7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -657,8 +657,11 @@ static struct merge_file_info merge_file
 			char orig[PATH_MAX];
 			char src1[PATH_MAX];
 			char src2[PATH_MAX];
+			char tmppath[PATH_MAX];
+
 			const char *argv[] = {
-				"merge", "-L", NULL, "-L", NULL, "-L", NULL,
+				"diff3", "-E", "-am",
+				"-L", NULL, "-L", NULL, "-L", NULL,
 				NULL, NULL, NULL,
 				NULL
 			};
@@ -668,23 +671,31 @@ static struct merge_file_info merge_file
 			git_unpack_file(a->sha1, src1);
 			git_unpack_file(b->sha1, src2);
 
-			argv[2] = la = xstrdup(mkpath("%s/%s", branch1, a->path));
-			argv[6] = lb = xstrdup(mkpath("%s/%s", branch2, b->path));
-			argv[4] = lo = xstrdup(mkpath("orig/%s", o->path));
-			argv[7] = src1;
-			argv[8] = orig;
-			argv[9] = src2,
+			argv[4] = la = xstrdup(mkpath("%s/%s", branch1, a->path));
+			argv[8] = lb = xstrdup(mkpath("%s/%s", branch2, b->path));
+			argv[6] = lo = xstrdup(mkpath("orig/%s", o->path));
+			argv[9] = src1;
+			argv[10] = orig;
+			argv[11] = src2;
+
+			fd = git_mkstemp(tmppath, sizeof(tmppath),
+					".merge_file_XXXXXX");
+			if (fd < 0)
+				die("unable to create temp-file");
+
+			dup2(fd, 1);
+			close(fd);
 
-			code = run_command_v(10, argv);
+			code = run_command_v(12, argv);
 
 			free(la);
 			free(lb);
 			free(lo);
 			if (code && code < -256) {
-				die("Failed to execute 'merge'. merge(1) is used as the "
-				    "file-level merge tool. Is 'merge' in your path?");
+				die("Failed to execute 'diff3'. diff3(1) is used as the "
+				    "file-level merge tool. Is 'diff3' in your path?");
 			}
-			fd = open(src1, O_RDONLY);
+			fd = open(tmppath, O_RDONLY);
 			if (fd < 0 || fstat(fd, &st) < 0 ||
 					index_fd(result.sha, fd, &st, 1,
 						"blob"))
@@ -693,6 +704,7 @@ static struct merge_file_info merge_file
 			unlink(orig);
 			unlink(src1);
 			unlink(src2);
+			unlink(tmppath);
 
 			result.clean = WEXITSTATUS(code) == 0;
 		} else {
-- 
1.4.3.rc2

-- 
Uwe Zeisberger

http://www.google.com/search?q=1+newton+in+kg*m+%2F+s%5E2

Re: [PATCH] Use diff3 instead of merge in merge-recursive.

[Jakub Narebski]

Uwe Zeisberger wrote:

> If no error occurs, merge (from rcs 5.7) is nothing but:
> 
>         diff3 -E -am -L label1 -L label2 -L label3 file1 file2 file3 > tmpfile
>         cat tmpfile > file1
> 
> Using diff3 directly saves one fork per conflicting file.

Doesn't xdiff library git uses have diff3/merge equivalent?
Couldn't we use that instead (on less dependency, better performance)?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] Use diff3 instead of merge in merge-recursive.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 789 bytes --]

Hi,

On Wed, 18 Oct 2006, Jakub Narebski wrote:

> Uwe Zeisberger wrote:
> 
> > If no error occurs, merge (from rcs 5.7) is nothing but:
> > 
> >         diff3 -E -am -L label1 -L label2 -L label3 file1 file2 file3 > tmpfile
> >         cat tmpfile > file1
> > 
> > Using diff3 directly saves one fork per conflicting file.
> 
> Doesn't xdiff library git uses have diff3/merge equivalent?
> Couldn't we use that instead (on less dependency, better performance)?

We have only half of libxdiff, lacking the patch functionality. And AFAIUI 
the minimal implementation of diff3 in xdiff just uses a diff of the first 
pair on the third file (or some other order). So, it is not really a 
diff3/merge: it would fail in fairly trivial cases.

Ciao,
Dscho

P.S.: Davide, correct me if I'm wrong.

Re: [PATCH] Use diff3 instead of merge in merge-recursive.

[Linus Torvalds]

On Wed, 18 Oct 2006, Jakub Narebski wrote:
> 
> Doesn't xdiff library git uses have diff3/merge equivalent?

Nope. It has something it calls "merge", but it's really just "apply the 
diff from the common base to the other end".

IOW, if "a" is your common ancestor, and "b1" and "b2" are the branches, 
it's literally

	diff a b1 | patch b2

and not actually a real 3-way merge.

As to why git uses "merge" - I have this strong memory of having seen 
machines that had one but not the other, and that, along with the fact 
that I've used "merge" personally, is why we call "merge" rather than 
diff3.

In Linux systems, "merge" usually comes with the RCS package, and "diff3" 
is usually from "diffutils". It may be that "diff3" is more common. I'm 
not sure what the history is, and what the situation would tend to be like 
on other systems..

		Linus

Re: [PATCH] Use diff3 instead of merge in merge-recursive.

[Daniel Barkalow]

On Wed, 18 Oct 2006, Linus Torvalds wrote:

> As to why git uses "merge" - I have this strong memory of having seen 
> machines that had one but not the other, and that, along with the fact 
> that I've used "merge" personally, is why we call "merge" rather than 
> diff3.
> 
> In Linux systems, "merge" usually comes with the RCS package, and "diff3" 
> is usually from "diffutils". It may be that "diff3" is more common. I'm 
> not sure what the history is, and what the situation would tend to be like 
> on other systems..

AFAICT, "merge" actually execs "diff3", so if a system has only one, 
either it's diff3, it's a "merge" from somewhere other than current rcs, 
or it doesn't actually work.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Use diff3 instead of merge in merge-recursive.

[Johannes Schindelin]

Hi Uwe,

On Wed, 18 Oct 2006, Uwe Zeisberger wrote:

> If no error occurs, merge (from rcs 5.7) is nothing but:
> 
> 	diff3 -E -am -L label1 -L label2 -L label3 file1 file2 file3 > tmpfile
> 	cat tmpfile > file1

Interesting. I wonder if we could streamline the code such that index_fd 
is called directly on the output of diff3? Of course, the result has to be 
removed when the call to diff3 fails.

> I didn't made any timing tests or further tests for correctness, but I
> hope Johannes still has the framework from the time when he converted
> the Python script to C?  
> 
> @Johannes: If so, could you test this patch?

I have to dig a little where I have it, but I think I can give it a try in 
a few hours (imagine this lyrics to the melody of the day job blues).

Ciao,
Dscho

Re: [PATCH] Use diff3 instead of merge in merge-recursive.

[Uwe Zeisberger]

Johannes Schindelin wrote:
> Hi Uwe,
> 
> On Wed, 18 Oct 2006, Uwe Zeisberger wrote:
> 
> > If no error occurs, merge (from rcs 5.7) is nothing but:
> > 
> > 	diff3 -E -am -L label1 -L label2 -L label3 file1 file2 file3 > tmpfile
> > 	cat tmpfile > file1
> 
> Interesting.

> I wonder if we could streamline the code such that index_fd 
> is called directly on the output of diff3? Of course, the result has to be 
> removed when the call to diff3 fails.
I thought about that, too.  But my primary intention was to get rid of
'merge', because the Solaris boxes I use from time to time lack merge,
but have (GNU) diff3[1].  I already had a mental note to look into that.

If Linus is right that there are systems that have merge but lack diff3,
then a combined approach is maybe the best?  That is, try diff3 and if
that is missing, try merge.  (Or the other way round if you prefer.)

OK, I looked a bit deeper into rcs, and it seems to handle the BSD diff3
case.  So Linus might be right.

BTW, merge -p sends the merged result to stdout instead of overwriting
the first file given.  That is

	merge -p -L label1 -L label2 -L label3 file1 file2 file3

and (GNU)

	diff3 -E -am -L label1 -L label2 -L label3 file1 file2 file3

are exactly equivalent.
So if that option of merge is old enough, these are the candidates for
the "combined approach" (see above).

> > I didn't made any timing tests or further tests for correctness, but I
> > hope Johannes still has the framework from the time when he converted
> > the Python script to C?  
> > 
> > @Johannes: If so, could you test this patch?
> 
> I have to dig a little where I have it, but I think I can give it a try in 
> a few hours (imagine this lyrics to the melody of the day job blues).
Seems to be a long blues because you didn't sent any results. :-(

Best regards
Uwe

[1] They also have a version of diff3 (I guess from BSD) that is not
suited to be used for merging, at least rcs' merge cannot use it.

-- 
Uwe Zeisberger

If a lawyer and an IRS agent were both drowning, and you could only save
one of them, would you go to lunch or read the paper?

Re: [PATCH] Use diff3 instead of merge in merge-recursive.

[Johannes Schindelin]

Hi,

On Fri, 20 Oct 2006, Uwe Zeisberger wrote:

> Johannes Schindelin wrote:
>
> > I have to dig a little where I have it, but I think I can give it a try in 
> > a few hours (imagine this lyrics to the melody of the day job blues).
>
> Seems to be a long blues because you didn't sent any results. :-(

Yes. It was a long blues, and now instead of going to sleep I tested it. 
In the long run, it was negligible, with a high local variability (which 
stems from the fact that I had to run this test while the machine was 
under high load, which it will be for another 48 hours minimum).

It makes sense that performance-wise, it will not make a great difference. 
After all, the expensive operation is not the file-writing, but the 
merging pass.

Ciao,
Dscho