[PATCH 1/2] compat: add a mkstemps() compatibility function

[PATCH 1/2] compat: add a mkstemps() compatibility function

[David Aguilar]

mkstemps() is a BSD extension so provide an implementation
for cross-platform use.

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

This mkstemps() implementation is adapted from libiberty's
GPLv2+ mkstemps.c.

 Makefile          |   19 +++++++++++++
 compat/mkstemps.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |    5 +++
 3 files changed, 102 insertions(+), 0 deletions(-)
 create mode 100644 compat/mkstemps.c

diff --git a/Makefile b/Makefile
index eaae45d..a70b5f0 100644
--- a/Makefile
+++ b/Makefile
@@ -52,6 +52,8 @@ all::
 #
 # Define NO_MKDTEMP if you don't have mkdtemp in the C library.
 #
+# Define NO_MKSTEMPS if you don't have mkstemps in the C library.
+#
 # Define NO_SYS_SELECT_H if you don't have sys/select.h.
 #
 # Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.
@@ -636,10 +638,12 @@ EXTLIBS =
 
 ifeq ($(uname_S),Linux)
 	NO_STRLCPY = YesPlease
+	NO_MKSTEMPS = YesPlease
 	THREADED_DELTA_SEARCH = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	NO_STRLCPY = YesPlease
+	NO_MKSTEMPS = YesPlease
 	THREADED_DELTA_SEARCH = YesPlease
 endif
 ifeq ($(uname_S),UnixWare)
@@ -651,6 +655,7 @@ ifeq ($(uname_S),UnixWare)
 	SHELL_PATH = /usr/local/bin/bash
 	NO_IPV6 = YesPlease
 	NO_HSTRERROR = YesPlease
+	NO_MKSTEMPS = YesPlease
 	BASIC_CFLAGS += -Kthread
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
@@ -674,6 +679,7 @@ ifeq ($(uname_S),SCO_SV)
 	SHELL_PATH = /usr/bin/bash
 	NO_IPV6 = YesPlease
 	NO_HSTRERROR = YesPlease
+	NO_MKSTEMPS = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
 	NO_STRCASESTR = YesPlease
@@ -702,6 +708,7 @@ ifeq ($(uname_S),SunOS)
 	NO_MEMMEM = YesPlease
 	NO_HSTRERROR = YesPlease
 	NO_MKDTEMP = YesPlease
+	NO_MKSTEMPS = YesPlease
 	OLD_ICONV = UnfortunatelyYes
 	ifeq ($(uname_R),5.8)
 		NO_UNSETENV = YesPlease
@@ -724,6 +731,7 @@ ifeq ($(uname_O),Cygwin)
 	NO_D_INO_IN_DIRENT = YesPlease
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
+	NO_MKSTEMPS = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
@@ -767,11 +775,13 @@ ifeq ($(uname_S),NetBSD)
 	BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
 	THREADED_DELTA_SEARCH = YesPlease
 	USE_ST_TIMESPEC = YesPlease
+	NO_MKSTEMPS = YesPlease
 endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
 	NO_MKDTEMP = YesPlease
+	NO_MKSTEMPS = YesPlease
 	NO_STRLCPY = YesPlease
 	NO_NSEC = YesPlease
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
@@ -787,12 +797,14 @@ endif
 ifeq ($(uname_S),GNU)
 	# GNU/Hurd
 	NO_STRLCPY=YesPlease
+	NO_MKSTEMPS = YesPlease
 endif
 ifeq ($(uname_S),IRIX64)
 	NO_IPV6=YesPlease
 	NO_SETENV=YesPlease
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
+	NO_MKSTEMPS = YesPlease
 	NO_STRLCPY = YesPlease
 	NO_SOCKADDR_STORAGE=YesPlease
 	SHELL_PATH=/usr/gnu/bin/bash
@@ -805,6 +817,7 @@ ifeq ($(uname_S),HP-UX)
 	NO_SETENV=YesPlease
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
+	NO_MKSTEMPS = YesPlease
 	NO_STRLCPY = YesPlease
 	NO_MKDTEMP = YesPlease
 	NO_UNSETENV = YesPlease
@@ -834,6 +847,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_C99_FORMAT = YesPlease
 	NO_STRTOUMAX = YesPlease
 	NO_MKDTEMP = YesPlease
+	NO_MKSTEMPS = YesPlease
 	SNPRINTF_RETURNS_BOGUS = YesPlease
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
@@ -853,6 +867,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
+	NO_MKSTEMPS = YesPlease
 endif
 
 -include config.mak.autogen
@@ -1011,6 +1026,10 @@ ifdef NO_MKDTEMP
 	COMPAT_CFLAGS += -DNO_MKDTEMP
 	COMPAT_OBJS += compat/mkdtemp.o
 endif
+ifdef NO_MKSTEMPS
+	COMPAT_CFLAGS += -DNO_MKSTEMPS
+	COMPAT_OBJS += compat/mkstemps.o
+endif
 ifdef NO_UNSETENV
 	COMPAT_CFLAGS += -DNO_UNSETENV
 	COMPAT_OBJS += compat/unsetenv.o
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
new file mode 100644
index 0000000..10f9ed6
--- /dev/null
+++ b/compat/mkstemps.c
@@ -0,0 +1,78 @@
+#include <string.h>
+#include <errno.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <unistd.h>
+#include <sys/time.h>
+#include <sys/types.h>
+
+#ifndef TMP_MAX
+#define TMP_MAX 16384
+#endif
+
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+
+/* Adapted from libiberty's mkstemp.c. */
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	static const char letters[] =
+		"abcdefghijklmnopqrstuvwxyz"
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"0123456789";
+	static const int num_letters = 62;
+	uint64_t value;
+	struct timeval tv;
+	char *template;
+	size_t len;
+	int fd, count;
+
+	len = strlen(pattern);
+
+	if (len < 6 + suffix_len) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	/* Replace pattern's XXXXXX characters with randomness.
+	 * Try TMP_MAX different filenames.
+	 */
+	gettimeofday(&tv, NULL);
+	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
+	template = &pattern[len - 6 - suffix_len];
+	for (count = 0; count < TMP_MAX; ++count) {
+		uint64_t v = value;
+		/* Fill in the random bits. */
+		template[0] = letters[v % num_letters]; v/= num_letters;
+		template[1] = letters[v % num_letters]; v/= num_letters;
+		template[2] = letters[v % num_letters]; v/= num_letters;
+		template[3] = letters[v % num_letters]; v/= num_letters;
+		template[4] = letters[v % num_letters]; v/= num_letters;
+		template[5] = letters[v % num_letters]; v/= num_letters;
+
+		fd = open(pattern, O_BINARY|O_CREAT|O_EXCL|O_RDWR, 0600);
+		if (fd > 0)
+			return fd;
+		/* Fatal error (EPERM, ENOSPC etc).
+		 * It doesn't make sense to loop.
+		 */
+		if (errno != EEXIST)
+			break;
+		/* This is a random value.  It is only necessary that
+		 * the next TMP_MAX values generated by adding 7777 to
+		 * VALUE are different with (module 2^32).
+		 */
+		value += 7777;
+	}
+	/* We return the null string if we can't find a unique file name.  */
+	pattern[0] = '\0';
+	errno = EINVAL;
+	return -1;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index c7cf2d5..f7217ad 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -232,6 +232,11 @@ extern int gitsetenv(const char *, const char *, int);
 extern char *gitmkdtemp(char *);
 #endif
 
+#ifdef NO_MKSTEMPS
+#define mkstemps gitmkstemps
+extern int gitmkstemps(char *, int);
+#endif
+
 #ifdef NO_UNSETENV
 #define unsetenv gitunsetenv
 extern void gitunsetenv(const char *);
-- 
1.6.3.1.169.g33fd

[PATCH 2/2] 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
user-friendly filenames when creating temporary files.

Diffing "name.ext" now generates "XXXXXX_name.ext".

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 cache.h                  |    2 ++
 diff.c                   |   40 ++++++++++++++++++++++++++++++++--------
 path.c                   |   19 +++++++++++++++++++
 t/t4020-diff-external.sh |   18 ++++++++++++++++++
 4 files changed, 71 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 dcfbcb0..23c52f5 100644
--- a/diff.c
+++ b/diff.c
@@ -13,6 +13,7 @@
 #include "utf8.h"
 #include "userdiff.h"
 #include "sigchain.h"
+#include <libgen.h>
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -1960,12 +1961,31 @@ 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) {
+		/* Generate "XXXXXX_filename" */
+		struct strbuf pretty_name = STRBUF_INIT;
+		char *pathdup = xstrdup(path);
+		char *base = basename(pathdup);
+		int suffix_len = strlen(base) + 1;
+
+		strbuf_addstr(&pretty_name, "XXXXXX_");
+		strbuf_addstr(&pretty_name, base);
+
+		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 +2004,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();
 
@@ -2021,7 +2042,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);
 			strbuf_release(&sb);
 		}
 		else {
@@ -2045,7 +2067,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;
 }
@@ -2071,8 +2093,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;
@@ -3574,8 +3597,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..090b490 100644
--- a/path.c
+++ b/path.c
@@ -140,6 +140,25 @@ int git_mkstemp(char *path, size_t len, const char *template)
 }
 
 
+
+/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */
+int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
+{
+	const char *tmp;
+	size_t n;
+
+	tmp = getenv("TMPDIR");
+	if (!tmp)
+		tmp = "/tmp";
+	n = snprintf(path, len, "%s/%s", tmp, template);
+	if (len <= n) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+	return mkstemps(path, suffix_len);
+}
+
+
 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..602d932 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.3.1.169.g33fd

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

[David Aguilar]

On Tue, May 26, 2009 at 08:19:24PM -0700, David Aguilar wrote:
> --- 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
>  '

Sorry for including both tests here.
I forgot to remove the first test when we changed the naming
convention to 6X_name and not special-casing filenames with
dots in them.

I guess I'll resend w/out it.  I'll wait until wednesday
evening before resending to see if anyone spots any other
fixups.


The rationale for the original naming convention was that it
was arguably easier for users to spot what file they're
working on when playing games with dot.

compare:

a) !!!!!!_Makefile

b) Makefile_!!!!!!

and:

a) !!!!!!_diff.c

b) diff.!!!!!!.c


In both examples you already know what filename you're talking
about before scanning past any other information.

.gitignore would look like .!!!!!!.gitignore, which isn't too
bad either.

Anyways, not looking to bikeshed, so I'll wait to here what
others have to say and continue.  The patch is fine as-is too
sans the first test.


> +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.3.1.169.g33fd
> 

-- 

	David

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

[Johannes Sixt]

David Aguilar schrieb:
> +#include <libgen.h>

We don't have libgen.h on Windows :-( (at least not with my aging gcc
3.something). Would you please open-code the call to basename()?

-- Hannes

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

[David Aguilar]

On Wed, May 27, 2009 at 08:34:57AM +0200, Johannes Sixt wrote:
> David Aguilar schrieb:
> > +#include <libgen.h>
> 
> We don't have libgen.h on Windows :-( (at least not with my aging gcc
> 3.something). Would you please open-code the call to basename()?
> 
> -- Hannes
> 

Thanks for catching that.
I'll roll that into the next version.

-- 

	David

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

[David Aguilar]

On Wed, May 27, 2009 at 08:34:57AM +0200, Johannes Sixt wrote:
> David Aguilar schrieb:
> > +#include <libgen.h>
> 
> We don't have libgen.h on Windows :-( (at least not with my aging gcc
> 3.something). Would you please open-code the call to basename()?
> 
> -- Hannes
> 

I don't know much about Windows.
Do we need to check for \ in addition to /?

-- 

	David

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

[Johannes Sixt]

David Aguilar schrieb:
> On Wed, May 27, 2009 at 08:34:57AM +0200, Johannes Sixt wrote:
>> David Aguilar schrieb:
>>> +#include <libgen.h>
>> We don't have libgen.h on Windows :-( (at least not with my aging gcc
>> 3.something). Would you please open-code the call to basename()?
> 
> I don't know much about Windows.
> Do we need to check for \ in addition to /?

If the paths have been taken from the index or have passed through
get_pathspec or prefix_filename, then it is sufficient to check for '/'.
Otherwise, use is_dir_sep() (note it is a macro: don't call it like
is_dir_sep(*c++)).

-- Hannes

Re: [PATCH 1/2] compat: add a mkstemps() compatibility function

[Jakub Narebski]

David Aguilar <davvid@gmail.com> writes:

> mkstemps() is a BSD extension so provide an implementation
> for cross-platform use.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> 
> This mkstemps() implementation is adapted from libiberty's
> GPLv2+ mkstemps.c.
> 
>  Makefile          |   19 +++++++++++++
>  compat/mkstemps.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  git-compat-util.h |    5 +++
>  3 files changed, 102 insertions(+), 0 deletions(-)
>  create mode 100644 compat/mkstemps.c

Could you squash this in?  I see that configure.ac is a bit outdated,
but we shouldn't make it even more so...

---
 config.mak.in |    1 +
 configure.ac  |    6 ++++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index 7cce0c1..b6619af 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -46,6 +46,7 @@ NO_STRTOUMAX=@NO_STRTOUMAX@
 NO_SETENV=@NO_SETENV@
 NO_UNSETENV=@NO_UNSETENV@
 NO_MKDTEMP=@NO_MKDTEMP@
+NO_MKSTEMPS=@NO_MKSTEMPS@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
diff --git a/configure.ac b/configure.ac
index 4e728bc..2ddf498 100644
--- a/configure.ac
+++ b/configure.ac
@@ -677,6 +677,12 @@ GIT_CHECK_FUNC(mkdtemp,
 [NO_MKDTEMP=YesPlease])
 AC_SUBST(NO_MKDTEMP)
 #
+# Define NO_MKSTEMPS if you don't have mkstemps in the C library.
+GIT_CHECK_FUNC(mkstemps,
+[NO_MKSTEMPS=],
+[NO_MKSTEMPS=YesPlease])
+AC_SUBST(NO_MKSTEMPS)
+#
 # Define NO_MMAP if you want to avoid mmap.
 #
 # Define NO_ICONV if your libc does not properly support iconv.
-- 
1.6.3.1