threads / patch / 19509

v3diff: generate prettier filenames when using GIT_EXTERNAL_DIFF

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

## tl;dr

7 messages between May 26, 2009 and May 26, 2009. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

David Aguilar· May 26, 2009, 05:36 UTC · lore

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(-)
Show changes to 4 files +137 −8

cache.h, diff.c, path.c, t/t4020-diff-external.sh

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
Stephen Boyd· May 26, 2009, 06:01 UTC · re: David Aguilar · lore

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

David Aguilar wrote:
Show 18 quoted lines
> -	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"?

David Aguilar· May 26, 2009, 17:37 UTC · re: Stephen Boyd · lore

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

On Mon, May 25, 2009 at 11:01:28PM -0700, Stephen Boyd wrote:
Show 23 quoted lines
> 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
Junio C Hamano· May 26, 2009, 06:12 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
Show 15 quoted lines
> 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.

Markus Heidelberg· May 26, 2009, 20:31 UTC · re: David Aguilar · lore

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

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
David Aguilar· May 26, 2009, 20:52 UTC · re: Markus Heidelberg · lore

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

On Tue, May 26, 2009 at 10:31:50PM +0200, Markus Heidelberg wrote:
Show 11 quoted lines
> 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
Junio C Hamano· May 26, 2009, 21:41 UTC · re: David Aguilar · lore

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

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.

← back to recent threads