threads / patch / 19603

v5, 3 partscompat: add a mkstemps() compatibility function

Subject: [PATCH v5 1/3] compat: add a mkstemps() compatibility function

## tl;dr

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

replies: 4people: 2as markdown or json

David Aguilar· May 30, 2009, 02:18 UTC · lore

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

Signed-off-by: David Aguilar <davvid@gmail.com>
Tested-by: Johannes Sixt <j6t@kdbg.org> (Windows)
---

This is unchanged since v3, but I'm including it for completeness.

 Makefile          |   19 +++++++++++++++
 compat/mkstemps.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 config.mak.in     |    1 +
 configure.ac      |    6 ++++
 git-compat-util.h |    5 ++++
 5 files changed, 98 insertions(+), 0 deletions(-)
 create mode 100644 compat/mkstemps.c
Show changes to 5 files +98 −0

Makefile, compat/mkstemps.c, config.mak.in, configure.ac, git-compat-util.h

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..87ebc2a
--- /dev/null
+++ b/compat/mkstemps.c
@@ -0,0 +1,67 @@
+#include "../git-compat-util.h"
+
+#ifndef TMP_MAX
+#define TMP_MAX 16384
+#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_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/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..95dccd4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -676,6 +676,12 @@ GIT_CHECK_FUNC(mkdtemp,
 [NO_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.
 #
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.178.g4daa97
David Aguilar· May 30, 2009, 02:18 UTC · re: David Aguilar · lore

[PATCH v5 2/3] path: add a find_basename() portability function

find_basename() is a simpler version of basename(). It maintains constness and thus does not require you to pass in a copy. It is only concerned with finding the last directory separator in a string and returning a pointer.

This function was written because Windows does not have basename(). It is called "find_basename()" to avoid name collisions and to provide a hint that it is not quite the same thing as POSIX basename().

Signed-off-by: David Aguilar <davvid@gmail.com>
---
As suggested by Peff.
 cache.h |    1 +
 path.c  |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)
Show changes to 2 files +15 −0

cache.h, path.c

diff --git a/cache.h b/cache.h
index b8503ad..08b6f42 100644
--- a/cache.h
+++ b/cache.h
@@ -640,6 +640,7 @@ static inline int is_absolute_path(const char *path)
 	return path[0] == '/' || has_dos_drive_prefix(path);
 }
 int is_directory(const char *);
+const char *find_basename(const char *path);
 const char *make_absolute_path(const char *path);
 const char *make_nonrelative_path(const char *path);
 const char *make_relative_path(const char *abs, const char *base);
diff --git a/path.c b/path.c
index 8a0a674..7a2fe14 100644
--- a/path.c
+++ b/path.c
@@ -358,6 +358,20 @@ int set_shared_perm(const char *path, int mode)
 	return 0;
 }
 
+/* return the basename of a path */
+const char *find_basename(const char *path)
+{
+	const char *basename = path + strlen(path) - 1;
+	while(*basename && basename > path) {
+		basename--;
+		if (is_dir_sep(*basename)) {
+			basename++;
+			break;
+		}
+	}
+	return basename;
+}
+
 const char *make_relative_path(const char *abs, const char *base)
 {
 	static char buf[PATH_MAX + 1];

-- 
1.6.3.1.178.g4daa97
David Aguilar· May 30, 2009, 02:18 UTC · re: David Aguilar · lore

[PATCH v5 3/3] diff: generate pretty filenames in prep_temp_blob()

Naturally, prep_temp_blob() did not care about filenames. As a result, GIT_EXTERNAL_DIFF and textconv generated filenames such as ".diff_XXXXXX".

This modifies prep_temp_blob() 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>
---

The difference since v4 is that find_basename() was factored out into its own function.

 cache.h                  |    2 ++
 diff.c                   |   10 +++++++++-
 path.c                   |   16 ++++++++++++++++
 t/t4020-diff-external.sh |    9 +++++++++
 4 files changed, 36 insertions(+), 1 deletions(-)
Show changes to 4 files +36 −1

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

diff --git a/cache.h b/cache.h
index 08b6f42..337458d 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..7edee28 100644
--- a/diff.c
+++ b/diff.c
@@ -1964,8 +1964,15 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf template = STRBUF_INIT;
+	const char *basename = find_basename(path);
 
-	fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
+	/* Generate "XXXXXX_basename.ext" */
+	strbuf_addstr(&template, "XXXXXX_");
+	strbuf_addstr(&template, basename);
+
+	fd = git_mkstemps(temp->tmp_path, PATH_MAX, template.buf,
+			strlen(basename) + 1);
 	if (fd < 0)
 		die("unable to create temp-file: %s", strerror(errno));
 	if (convert_to_working_tree(path,
@@ -1981,6 +1988,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 	temp->hex[40] = 0;
 	sprintf(temp->mode, "%06o", mode);
 	strbuf_release(&buf);
+	strbuf_release(&template);
 }
 
 static struct diff_tempfile *prepare_temp_file(const char *name,
diff --git a/path.c b/path.c
index 7a2fe14..4f7fe8f 100644
--- a/path.c
+++ b/path.c
@@ -139,6 +139,22 @@ int git_mkstemp(char *path, size_t len, const char *template)
 	return mkstemp(path);
 }
 
+/* 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)
 {
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 0720001..4ea42e0 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -136,6 +136,15 @@ 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' '
+	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.178.g4daa97
Jeff King· May 30, 2009, 14:05 UTC · re: David Aguilar · lore

Re: [PATCH v5 2/3] path: add a find_basename() portability function

On Fri, May 29, 2009 at 07:18:09PM -0700, David Aguilar wrote:
Show 13 quoted lines
> +/* return the basename of a path */
> +const char *find_basename(const char *path)
> +{
> +	const char *basename = path + strlen(path) - 1;
> +	while(*basename && basename > path) {
> +		basename--;
> +		if (is_dir_sep(*basename)) {
> +			basename++;
> +			break;
> +		}
> +	}
> +	return basename;
> +}

Hmm. Is there any point to the *basename condition in the loop? By using strlen, you are not going to go past any NULs, and you are already checking that you don't go past the beginning of the string.

Speaking of which, how does this handle an input of ""? It seems that it would return a pointer to one character before the string. Given your loop, you need to special-case when *path is NUL.

Also, how should trailing dir_seps be handled? basename() will actually return "" if given "foo/". Your implementation, when given "/foo/bar/" will return "bar/" (and it must keep the trailing bit since we are neither reallocating nor munging the input string). But given "/foo/bar//", it will return simply "/". I could see an argument for either "bar//" or "", but I think behaving differently for trailing "/" versus trailing "//" doesn't make sense.

-Peff
David Aguilar· May 30, 2009, 22:14 UTC · re: Jeff King · lore

Re: [PATCH v5 2/3] path: add a find_basename() portability function

On Sat, May 30, 2009 at 10:05:19AM -0400, Jeff King wrote:
Show 33 quoted lines
> On Fri, May 29, 2009 at 07:18:09PM -0700, David Aguilar wrote:
> 
> > +/* return the basename of a path */
> > +const char *find_basename(const char *path)
> > +{
> > +	const char *basename = path + strlen(path) - 1;
> > +	while(*basename && basename > path) {
> > +		basename--;
> > +		if (is_dir_sep(*basename)) {
> > +			basename++;
> > +			break;
> > +		}
> > +	}
> > +	return basename;
> > +}
> 
> Hmm. Is there any point to the *basename condition in the loop? By using
> strlen, you are not going to go past any NULs, and you are already
> checking that you don't go past the beginning of the string.
> 
> Speaking of which, how does this handle an input of ""? It seems that it
> would return a pointer to one character before the string. Given your
> loop, you need to special-case when *path is NUL.
> 
> Also, how should trailing dir_seps be handled? basename() will actually
> return "" if given "foo/". Your implementation, when given "/foo/bar/"
> will return "bar/" (and it must keep the trailing bit since we are
> neither reallocating nor munging the input string). But given
> "/foo/bar//", it will return simply "/". I could see an argument for
> either "bar//" or "", but I think behaving differently for trailing "/"
> versus trailing "//" doesn't make sense.
> 
> -Peff

You know.. going with the compat/basename.c solution is probably a better idea now that you've pointed out all these issues. Then we can just use the system's basename() when available and use our compat/basename.c otherwise (is it only for Windows?).

Here's basename() from libiberty.

It drops constness (likely to keep in line with the standard) and has some #defines that I'll have to translate into git equivalents (e.g. IS_DIR_SEPARATOR -> is_dir_sep).

Do we have a cross-platform ISALPHA thing (just thinking out loud -- I can check). I'm under the impression that going with something like this should get us in a better place.

What do you think?
char *basename (const char *name)
{
	const char *base;
#if defined (HAVE_DOS_BASED_FILE_SYSTEM)
	/* Skip over the disk name in MSDOS pathnames. */
	if (ISALPHA (name[0]) && name[1] == ':') 
		name += 2;
#endif
	for (base = name; *name; name++) {
		if (IS_DIR_SEPARATOR (*name)) {
			base = name + 1;
		}
	}
	return (char *) base;
}
-- 
		David

← back to recent threads