[PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[David Aguilar]

Naturally, prep_temp_blob() did not care about filenames.
As a result, scripts that use GIT_EXTERNAL_DIFF ended up
with filenames such as ".diff_XXXXXX".

This specializes the GIT_EXTERNAL_DIFF code to generate
prettier filenames.

Diffing "name.ext" now generates "name.XXXX.ext".
Diffing files with no extension now generates "name_XXXX".

Signed-off-by: David Aguilar <davvid@gmail.com>
---

I renamed a few variables and unrolled the inner loop in
git_mkstemps() since the last patch to make things easier to read/review.

We might as well go with this one if we haven't applied the
previous versions of this patch. 

 cache.h                  |    2 +
 diff.c                   |   55 +++++++++++++++++++++++++++++++-----
 path.c                   |   70 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t4020-diff-external.sh |   18 ++++++++++++
 4 files changed, 137 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index b8503ad..871c984 100644
--- a/cache.h
+++ b/cache.h
@@ -614,6 +614,8 @@ extern int is_empty_blob_sha1(const unsigned char *sha1);
 
 int git_mkstemp(char *path, size_t n, const char *template);
 
+int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);
+
 /*
  * NOTE NOTE NOTE!!
  *
diff --git a/diff.c b/diff.c
index f06876b..226771d 100644
--- a/diff.c
+++ b/diff.c
@@ -1960,12 +1960,47 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 			   void *blob,
 			   unsigned long size,
 			   const unsigned char *sha1,
-			   int mode)
+			   int mode,
+			   int pretty_filename)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
 
-	fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
+	if (pretty_filename) {
+		struct strbuf pretty_name = STRBUF_INIT;
+		char *pathdup = xstrdup(path);
+		char *base = basename(pathdup);
+		char *dot = strchr(base, '.');
+		int suffix_len = 0;
+
+		if (dot) {
+			/* path has an extension, e.g. "foo.txt";
+			 * generate "foo.XXXX.txt".
+			 */
+			*dot = '\0';
+			strbuf_addstr(&pretty_name, base);
+			*dot = '.';
+			strbuf_addstr(&pretty_name, ".XXXXXX");
+			suffix_len = strlen(dot);
+			strbuf_addstr(&pretty_name, dot);
+		}
+		else {
+			/* path has no extension, e.g. "Makefile";
+			 * generate "Makefile_XXXX".
+			 */
+			strbuf_addstr(&pretty_name, base);
+			strbuf_addstr(&pretty_name, "_XXXXXX");
+		}
+
+		fd = git_mkstemps(temp->tmp_path, PATH_MAX,
+			pretty_name.buf, suffix_len);
+
+		free(pathdup);
+		strbuf_release(&pretty_name);
+	}
+	else {
+		fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
+	}
 	if (fd < 0)
 		die("unable to create temp-file: %s", strerror(errno));
 	if (convert_to_working_tree(path,
@@ -1984,7 +2019,8 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 }
 
 static struct diff_tempfile *prepare_temp_file(const char *name,
-		struct diff_filespec *one)
+		struct diff_filespec *one,
+		int pretty_filename)
 {
 	struct diff_tempfile *temp = claim_diff_tempfile();
 
@@ -2025,7 +2061,8 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 				       (one->sha1_valid ?
 					one->sha1 : null_sha1),
 				       (one->sha1_valid ?
-					one->mode : S_IFLNK));
+					one->mode : S_IFLNK),
+				       pretty_filename);
 		}
 		else {
 			/* we can borrow from the file in the work tree */
@@ -2048,7 +2085,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 		if (diff_populate_filespec(one, 0))
 			die("cannot read data blob for %s", one->path);
 		prep_temp_blob(name, temp, one->data, one->size,
-			       one->sha1, one->mode);
+			       one->sha1, one->mode, pretty_filename);
 	}
 	return temp;
 }
@@ -2074,8 +2111,9 @@ static void run_external_diff(const char *pgm,
 	if (one && two) {
 		struct diff_tempfile *temp_one, *temp_two;
 		const char *othername = (other ? other : name);
-		temp_one = prepare_temp_file(name, one);
-		temp_two = prepare_temp_file(othername, two);
+		int pretty_filename = 1;
+		temp_one = prepare_temp_file(name, one, pretty_filename);
+		temp_two = prepare_temp_file(othername, two, pretty_filename);
 		*arg++ = pgm;
 		*arg++ = name;
 		*arg++ = temp_one->name;
@@ -3577,8 +3615,9 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 	const char **arg = argv;
 	struct child_process child;
 	struct strbuf buf = STRBUF_INIT;
+	int pretty_filename = 0;
 
-	temp = prepare_temp_file(spec->path, spec);
+	temp = prepare_temp_file(spec->path, spec, pretty_filename);
 	*arg++ = pgm;
 	*arg++ = temp->name;
 	*arg = NULL;
diff --git a/path.c b/path.c
index 8a0a674..6292410 100644
--- a/path.c
+++ b/path.c
@@ -12,6 +12,8 @@
  */
 #include "cache.h"
 
+#define GIT_MKSTEMPS_MAX 16384
+
 static char bad_path[] = "/bad-path/";
 
 static char *get_pathname(void)
@@ -140,6 +142,74 @@ int git_mkstemp(char *path, size_t len, const char *template)
 }
 
 
+
+/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable.
+ * This is adapted from libguile's GPLv2 mkstemp.c.
+ */
+int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
+{
+	static const char letters[] =
+		"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"0123456789_";
+	const char *tmp;
+	char *pattern;
+	size_t n;
+	size_t path_noext_len, letters_len;
+	struct timeval tv;
+	uint64_t value, v;
+	int fd, count;
+
+	tmp = getenv("TMPDIR");
+	if (!tmp)
+		tmp = "/tmp";
+	n = snprintf(path, len, "%s/%s", tmp, template);
+	if (len <= n) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+
+	letters_len = strlen(letters);
+	path_noext_len = n - suffix_len;
+
+	if (path_noext_len < 0 || strlen(template) - suffix_len < 6) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (strncmp(path + path_noext_len - 6, "XXXXXX", 6)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	/* Replace template's XXXXXX characters with randomness.
+	 * Try GIT_MKSTEMPS_MAX different filenames.
+	 */
+	gettimeofday(&tv, NULL);
+	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
+	pattern = path + path_noext_len - 6;
+	for (count = 0; count < GIT_MKSTEMPS_MAX; ++count) {
+		v = value;
+		pattern[0] = letters[v % letters_len]; v/= letters_len;
+		pattern[1] = letters[v % letters_len]; v/= letters_len;
+		pattern[2] = letters[v % letters_len]; v/= letters_len;
+		pattern[3] = letters[v % letters_len]; v/= letters_len;
+		pattern[4] = letters[v % letters_len]; v/= letters_len;
+		pattern[5] = letters[v % letters_len]; v/= letters_len;
+
+		fd = open(path, O_CREAT | O_EXCL | O_RDWR, 0600);
+		if (fd > 0)
+			return fd;
+		/* This is a random value.  It is only necessary that
+		 * the next value generated by adding 7777 is different
+		 * modulus 2^32.
+		 */
+		value += 7777;
+	}
+	errno = EINVAL;
+	return -1;
+}
+
+
 int validate_headref(const char *path)
 {
 	struct stat st;
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 0720001..a9fdcb0 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -136,6 +136,24 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '
 	GIT_EXTERNAL_DIFF=echo git diff
 '
 
+test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths with no ext' '
+	touch filenoext &&
+	git add filenoext &&
+	echo no extension > filenoext &&
+	GIT_EXTERNAL_DIFF=echo git diff filenoext | grep filenoext_ &&
+	git update-index --force-remove filenoext &&
+	rm filenoext
+'
+
+test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths with ext' '
+	touch file.ext &&
+	git add file.ext &&
+	echo with extension > file.ext &&
+	GIT_EXTERNAL_DIFF=echo git diff file.ext | grep file\.......\.ext &&
+	git update-index --force-remove file.ext &&
+	rm file.ext
+'
+
 echo "#!$SHELL_PATH" >fake-diff.sh
 cat >> fake-diff.sh <<\EOF
 cat $2 >> crlfed.txt
-- 
1.6.1.3

Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[Stephen Boyd]

David Aguilar wrote:
> -	fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
> +	if (pretty_filename) {
> +		struct strbuf pretty_name = STRBUF_INIT;
> +		char *pathdup = xstrdup(path);
> +		char *base = basename(pathdup);
> +		char *dot = strchr(base, '.');
> +		int suffix_len = 0;
> +
> +		if (dot) {
> +			/* path has an extension, e.g. "foo.txt";
> +			 * generate "foo.XXXX.txt".
> +			 */
> +			*dot = '\0';
> +			strbuf_addstr(&pretty_name, base);
> +			*dot = '.';
> +			strbuf_addstr(&pretty_name, ".XXXXXX");
> +			suffix_len = strlen(dot);
> +			strbuf_addstr(&pretty_name, dot);

This *dot business annoys me. Would it be better to use strbuf_add()
with some pointer math thrown in? Also, what happens with files such as
"foo.bar.txt"? Do we want "foo.XXXXX.bar.txt"?

Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Naturally, prep_temp_blob() did not care about filenames.
> As a result, scripts that use GIT_EXTERNAL_DIFF ended up
> with filenames such as ".diff_XXXXXX".
>
> This specializes the GIT_EXTERNAL_DIFF code to generate
> prettier filenames.
>
> Diffing "name.ext" now generates "name.XXXX.ext".
> Diffing files with no extension now generates "name_XXXX".
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>
> I renamed a few variables and unrolled the inner loop in
> git_mkstemps() since the last patch to make things easier to read/review.

Thanks.

I actually think using mkstemps() where the function is natively
available, and using compat/mkstemps.c implementation where it is not, is
a saner approach.

It would make the implementation of git_mkstemps() easier to read, because
the interface to mkstemps(), even though it may not be in POSIX (nor in
glibc), is a well known quantity and people do not need to follow into its
implementation when they want to follow the logic of higher level code.

Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[David Aguilar]

On Mon, May 25, 2009 at 11:01:28PM -0700, Stephen Boyd wrote:
> David Aguilar wrote:
> > -	fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
> > +	if (pretty_filename) {
> > +		struct strbuf pretty_name = STRBUF_INIT;
> > +		char *pathdup = xstrdup(path);
> > +		char *base = basename(pathdup);
> > +		char *dot = strchr(base, '.');
> > +		int suffix_len = 0;
> > +
> > +		if (dot) {
> > +			/* path has an extension, e.g. "foo.txt";
> > +			 * generate "foo.XXXX.txt".
> > +			 */
> > +			*dot = '\0';
> > +			strbuf_addstr(&pretty_name, base);
> > +			*dot = '.';
> > +			strbuf_addstr(&pretty_name, ".XXXXXX");
> > +			suffix_len = strlen(dot);
> > +			strbuf_addstr(&pretty_name, dot);
> 
> This *dot business annoys me. Would it be better to use strbuf_add()
> with some pointer math thrown in? Also, what happens with files such as
> "foo.bar.txt"? Do we want "foo.XXXXX.bar.txt"?


That was intentional; "foo.tar.gz" becomes "foo.XXXXXX.tar.gz",
which is what I considered better behavior when writing it.

Thanks for the note about using strbuf_add().  I'll see if we
can simplify things by using it instead of strbuf_addstr().

I should probably split the "add compat/mkstemps" and
"use it in git_mkstemps()" parts into separate commits since
we're going to want to use the native mkstemps implementation if
it's available.

-- 
		David

Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[Markus Heidelberg]

David Aguilar, 26.05.2009:
> Diffing "name.ext" now generates "name.XXXX.ext".
> Diffing files with no extension now generates "name_XXXX".

Would it simplify the patch when using "name_XXXX.ext" and "name_XXXX"?
Or what about "XXXX_name.ext" and "XXXX_name"? Then you don't even have
to search for the dot and it has the advantage to show the original
filename without some random letters in the middle of it.

Markus

Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[David Aguilar]

On Tue, May 26, 2009 at 10:31:50PM +0200, Markus Heidelberg wrote:
> David Aguilar, 26.05.2009:
> > Diffing "name.ext" now generates "name.XXXX.ext".
> > Diffing files with no extension now generates "name_XXXX".
> 
> Would it simplify the patch when using "name_XXXX.ext" and "name_XXXX"?
> Or what about "XXXX_name.ext" and "XXXX_name"? Then you don't even have
> to search for the dot and it has the advantage to show the original
> filename without some random letters in the middle of it.
> 
> Markus
> 

In that case suffix_len is always len(basename) + 1
and there's less strbuf juggling to do.
That is simpler.  Let's do that.

My next patch series will be two parts:
part 1: add compat/mkstemps.c
part 2: use it in prep_temp_blob() via git_mkstemps()

Part 1 has me a little nervous because I'm not sure which
platforms in the Makefile need the
	NO_MKSTEMPS = YesPlease
setting.  I only have access to mac + linux, so I can do the
right thing there, but I'll have to make a best guess for the
rest of the platforms.

Perhaps the best answer is to set them all to YesPlease except
for mac and openbsd, which I know have native implementations.

-- 
		David

Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Perhaps the best answer is to set them all to YesPlease except
> for mac and openbsd, which I know have native implementations.

Some vintage of Solaris seems to have it:

    http://docs.sun.com/app/docs/doc/819-2243/mkstemps-3c?a=view

The notes section of

    http://developer.apple.com/documentation/Darwin/Reference/ManPages/man3/mkstemps.3.html

says

    The mkstemps() function first appeared in OpenBSD 2.4, and later in
     FreeBSD 3.4.