Re: combined diff does not detect binary files and ignores -diff attribute

[Jeff King <peff@peff.net>]

On Mon, May 23, 2011 at 07:31:02PM -0400, Jay Soffian wrote:

> 1. The temporary path git generates loses the original file extension.
> At least in the case of Mac OS X, ibtool gets angry when the ".xib"
> isn' there. I worked around this with a wrapper, but maybe git could
> preserve the extension.

Hmm, I thought we did preserve the extension. I think it may actually be
related to me making a fake filespec and not passing the path in.

Does the (totally untested) patch below fix it for you?

> 2. It's not really clear to me when one should use textconv vs a
> custom diff driver. (Also, --no-textconv isn't documented...).

The motivation for textconv was originally a combination of "when you
are too lazy to write a full diff driver" and "when you like how git
formats the diff with pretty colors and word-diff". But now I think we
can add to that:

  1. When you want something fancy and git-ish like combined diff.

  2. When you want free caching to speed up repeated log viewing (try
     setting diff.*.cachetextconv to true).

Maybe it is worth adding an "advantages of textconv" section to
gitattributes(5) (the advantage of a full external diff command is that
it can be more flexible, or even graphical in nature).

Anyway, here's the patch.

---
diff --git a/combine-diff.c b/combine-diff.c
index fbed374..a4f2ff5 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -94,7 +94,8 @@ struct sline {
 };
 
 static char *grab_blob(const unsigned char *sha1, unsigned int mode,
-		       unsigned long *size, struct userdiff_driver *textconv)
+		       unsigned long *size, struct userdiff_driver *textconv,
+		       const char *path)
 {
 	char *blob;
 	enum object_type type;
@@ -110,7 +111,7 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode,
 	} else if (textconv) {
 		/* yuck, the textconv code is linked heavily with
 		 * filespecs */
-		struct diff_filespec *df = alloc_filespec("");
+		struct diff_filespec *df = alloc_filespec(path);
 		fill_filespec(df, sha1, mode);
 		*size = fill_textconv(textconv, df, &blob);
 		free_filespec(df);
@@ -214,7 +215,8 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
 			 mmfile_t *result_file,
 			 struct sline *sline, unsigned int cnt, int n,
 			 int num_parent, int result_deleted,
-			 struct userdiff_driver *textconv)
+			 struct userdiff_driver *textconv,
+			 const char *path)
 {
 	unsigned int p_lno, lno;
 	unsigned long nmask = (1UL << n);
@@ -227,7 +229,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
 	if (result_deleted)
 		return; /* result deleted */
 
-	parent_file.ptr = grab_blob(parent, mode, &sz, textconv);
+	parent_file.ptr = grab_blob(parent, mode, &sz, textconv, path);
 	parent_file.size = sz;
 	memset(&xpp, 0, sizeof(xpp));
 	xpp.flags = 0;
@@ -793,7 +795,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	/* Read the result of merge first */
 	if (!working_tree_file)
 		result = grab_blob(elem->sha1, elem->mode, &result_size,
-				   textconv);
+				   textconv, elem->path);
 	else {
 		/* Used by diff-tree to read from the working tree */
 		struct stat st;
@@ -817,10 +819,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			unsigned char sha1[20];
 			if (resolve_gitlink_ref(elem->path, "HEAD", sha1) < 0)
 				result = grab_blob(elem->sha1, elem->mode,
-						   &result_size, NULL);
+						   &result_size, NULL, NULL);
 			else
 				result = grab_blob(sha1, elem->mode,
-						   &result_size, NULL);
+						   &result_size, NULL, NULL);
 		} else if (textconv) {
 			struct diff_filespec *df = alloc_filespec(elem->path);
 			fill_filespec(df, null_sha1, st.st_mode);
@@ -893,7 +895,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			unsigned long size;
 			buf = grab_blob(elem->parent[i].sha1,
 					elem->parent[i].mode,
-					&size, NULL);
+					&size, NULL, NULL);
 			if (buffer_is_binary(buf, size))
 				is_binary = 1;
 			free(buf);
@@ -955,7 +957,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 				     elem->parent[i].mode,
 				     &result_file, sline,
 				     cnt, i, num_parent, result_deleted,
-				     textconv);
+				     textconv, elem->path);
 	}
 
 	show_hunks = make_hunks(sline, cnt, num_parent, dense);