Re: Use a *real* built-in diff generator

Linus Torvalds [off-list ref] writes:

On Sat, 25 Mar 2006, Linus Torvalds wrote:

quoted

I'll be taking a look at trying to fix it. 

Actually, it ended up being easier than I expected it to be.

This (on top of the previous patch) should fix it.

This is a replacement for my previous one, which reduces the
changes to the testsuite.

I'll find time to omit prepare_temp_file() calls from diffcore
emit routines over the weekend.  Another useful project would be
to redo the combine-diff.c using the real built-in diff.

-- >8 --
[PATCH] built-in diff: minimum tweaks

This fixes up a couple of minor issues with the real built-in
diff to be more usable:

 - Omit ---/+++ header unless we emit diff output;

 - Detect and punt binary diff like GNU does;

 - Honor GIT_DIFF_OPTS minimally (only -u<number> and
   --unified=<number> are currently supported);

 - Omit line count of 1 from "@@ -l,k +m,n @@" hunk header
   (i.e. when k == 1 or n == 1)

 - Adjust testsuite for the lack of -p support.

Signed-off-by: Junio C Hamano <redacted>

---

 diff.c                 |   41 ++++++++++++++++++++++++++++++++++++-----
 t/t4001-diff-rename.sh |    2 +-
 xdiff/xutils.c         |   16 ++++++++++------
 3 files changed, 47 insertions(+), 12 deletions(-)

6ff2f393f16e56a5d3d611b4d3e8f039994ce0a8

diff --git a/diff.c b/diff.c
index f6a1f5d..cd2ce0f 100644
--- a/diff.c
+++ b/diff.c

@@ -212,16 +212,37 @@ static int fill_mmfile(mmfile_t *mf, con
 	return 0;
 }
 
+struct emit_callback {
+	const char **label_path;
+};
+
 static int fn_out(void *priv, mmbuffer_t *mb, int nbuf)
 {
 	int i;
+	struct emit_callback *ecbdata = priv;
 
+	if (ecbdata->label_path[0]) {
+		printf("--- %s\n", ecbdata->label_path[0]);
+		printf("+++ %s\n", ecbdata->label_path[1]);
+		ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
+	}
 	for (i = 0; i < nbuf; i++)
 		if (!fwrite(mb[i].ptr, mb[i].size, 1, stdout))
 			return -1;
 	return 0;
 }
 
+#define FIRST_FEW_BYTES 8000
+static int mmfile_is_binary(mmfile_t *mf)
+{
+	long sz = mf->size;
+	if (FIRST_FEW_BYTES < sz)
+		sz = FIRST_FEW_BYTES;
+	if (memchr(mf->ptr, 0, sz))
+		return 1;
+	return 0;
+}
+
 static const char *builtin_diff(const char *name_a,
 			 const char *name_b,
 			 struct diff_tempfile *temp,

@@ -306,22 +327,32 @@ static const char *builtin_diff(const ch
 	if (label_path[1][0] != '/')
 		label_path[1] = quote_two("b/", name_b);
 
-	printf("--- %s\n", label_path[0]);
-	printf("+++ %s\n", label_path[1]);
-
 	if (fill_mmfile(&mf1, temp[0].name) < 0 ||
 	    fill_mmfile(&mf2, temp[1].name) < 0)
 		die("unable to read files to diff");
 
-	/* Crazy xdl interfaces.. */
-	{
+	if (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2))
+		printf("Binary files %s and %s differ\n",
+		       label_path[0], label_path[1]);
+	else {
+		/* Crazy xdl interfaces.. */
+		const char *diffopts = getenv("GIT_DIFF_OPTS");
 		xpparam_t xpp;
 		xdemitconf_t xecfg;
 		xdemitcb_t ecb;
+		struct emit_callback ecbdata;
 
+		ecbdata.label_path = label_path;
 		xpp.flags = XDF_NEED_MINIMAL;
 		xecfg.ctxlen = 3;
+		if (!diffopts)
+			;
+		else if (!strncmp(diffopts, "--unified=", 10))
+			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
+		else if (!strncmp(diffopts, "-u", 2))
+			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
 		ecb.outf = fn_out;
+		ecb.priv = &ecbdata;
 		xdl_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
 	}
 

diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 2e3c20d..08c1131 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh

@@ -49,7 +49,7 @@ rename from path0
 rename to path1
 --- a/path0
 +++ b/path1
-@@ -8,7 +8,7 @@ Line 7
+@@ -8,7 +8,7 @@
  Line 8
  Line 9
  Line 10

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index b68afa2..8221806 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c

@@ -245,20 +245,24 @@ int xdl_emit_hunk_hdr(long s1, long c1, 
 
 	nb += xdl_num_out(buf + nb, c1 ? s1: 0);
 
-	memcpy(buf + nb, ",", 1);
-	nb += 1;
+	if (c1 != 1) {
+		memcpy(buf + nb, ",", 1);
+		nb += 1;
 
-	nb += xdl_num_out(buf + nb, c1);
+		nb += xdl_num_out(buf + nb, c1);
+	}
 
 	memcpy(buf + nb, " +", 2);
 	nb += 2;
 
 	nb += xdl_num_out(buf + nb, c2 ? s2: 0);
 
-	memcpy(buf + nb, ",", 1);
-	nb += 1;
+	if (c2 != 1) {
+		memcpy(buf + nb, ",", 1);
+		nb += 1;
 
-	nb += xdl_num_out(buf + nb, c2);
+		nb += xdl_num_out(buf + nb, c2);
+	}
 
 	memcpy(buf + nb, " @@\n", 4);
 	nb += 4;

-- 
1.2.4.g88d9

On Sat, 25 Mar 2006, Junio C Hamano wrote:

This is a replacement for my previous one, which reduces the
changes to the testsuite.

Looks good. With this, I think it's certainly ready for the "next" branch, 
and with some testing should be quite mergable into the main branch too.

I really didn't have to mess up the libxdiff files very much, so I don't 
think I introduced any new bugs, and libxdiff itself seems to be pretty 
stable (and that's partly from running the tests, but even more from just 
looking at the code and not having any 'Ewww! Gross!'-moments).

Which is not to say that there couldn't be bugs, but I think this is very 
much worth merging quickly..

I'll find time to omit prepare_temp_file() calls from diffcore
emit routines over the weekend.  Another useful project would be
to redo the combine-diff.c using the real built-in diff.

I think the pickaxe improvements would be an even bigger thing..

		Linus

On Sat, 25 Mar 2006, Junio C Hamano wrote:

This is a replacement for my previous one, which reduces the
changes to the testsuite.

Btw, your comment about the GNU diff @@-line simplification made me look 
at that again.

git-apply used to believe that a simplified line (ie with a single value) 
meant "x,x". While it seems that it really means "x,1".

Now, at worst, this would probably mean that some patches would get 
rejected (rather than mis-applied), so I think we've just never hit it. 
Also, with the default 3-line context I think that it simply _cannot_ be 
hit (since the only way that the result or source would have only one line 
in the resulting is it was the first line).

But if the "x" shorthand really means "x,1" (and I think you're right - 
using "-U 1" I can get that kind of shorthand) then apply.c should be 
fixed as follows (actual patch: removal of two characters).

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

diff --git a/apply.c b/apply.c
index 2da225a..e80cd15 100644
--- a/apply.c
+++ b/apply.c

@@ -693,7 +693,7 @@ static int parse_range(const char *line,
 	line += digits;
 	len -= digits;
 
-	*p2 = *p1;
+	*p2 = 1;
 	if (*line == ',') {
 		digits = parse_num(line+1, p2);
 		if (!digits)