{"thread":{"id":"19566","subject":"[PATCH v4 1/2] compat: add a mkstemps() compatibility function","startedAt":"2009-05-29T00:49:23Z","lastAt":"2009-05-30T01:47:51Z","messageCount":5,"participants":["David Aguilar","Jeff King"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"114975","messageId":"1243558164-74756-1-git-send-email-davvid@gmail.com","threadId":"19566","inReplyTo":null,"subject":"[PATCH v4 1/2] compat: add a mkstemps() compatibility function","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-29T00:49:23Z","receivedAt":"2009-05-29T00:49:23Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"mkstemps() is a BSD extension so provide an implementation\nfor cross-platform use.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\nTested-by: Johannes Sixt <j6t@kdbg.org> (Windows)\n---\n\nThis patch is unchanged from last time.\nI'm including it here for completeness.\n\n Makefile          |   19 +++++++++++++++\n compat/mkstemps.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n config.mak.in     |    1 +\n configure.ac      |    6 ++++\n git-compat-util.h |    5 ++++\n 5 files changed, 98 insertions(+), 0 deletions(-)\n create mode 100644 compat/mkstemps.c\n\ndiff --git a/Makefile b/Makefile\nindex eaae45d..a70b5f0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -52,6 +52,8 @@ all::\n #\n # Define NO_MKDTEMP if you don't have mkdtemp in the C library.\n #\n+# Define NO_MKSTEMPS if you don't have mkstemps in the C library.\n+#\n # Define NO_SYS_SELECT_H if you don't have sys/select.h.\n #\n # Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.\n@@ -636,10 +638,12 @@ EXTLIBS =\n \n ifeq ($(uname_S),Linux)\n \tNO_STRLCPY = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tTHREADED_DELTA_SEARCH = YesPlease\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tNO_STRLCPY = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tTHREADED_DELTA_SEARCH = YesPlease\n endif\n ifeq ($(uname_S),UnixWare)\n@@ -651,6 +655,7 @@ ifeq ($(uname_S),UnixWare)\n \tSHELL_PATH = /usr/local/bin/bash\n \tNO_IPV6 = YesPlease\n \tNO_HSTRERROR = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tBASIC_CFLAGS += -Kthread\n \tBASIC_CFLAGS += -I/usr/local/include\n \tBASIC_LDFLAGS += -L/usr/local/lib\n@@ -674,6 +679,7 @@ ifeq ($(uname_S),SCO_SV)\n \tSHELL_PATH = /usr/bin/bash\n \tNO_IPV6 = YesPlease\n \tNO_HSTRERROR = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tBASIC_CFLAGS += -I/usr/local/include\n \tBASIC_LDFLAGS += -L/usr/local/lib\n \tNO_STRCASESTR = YesPlease\n@@ -702,6 +708,7 @@ ifeq ($(uname_S),SunOS)\n \tNO_MEMMEM = YesPlease\n \tNO_HSTRERROR = YesPlease\n \tNO_MKDTEMP = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tOLD_ICONV = UnfortunatelyYes\n \tifeq ($(uname_R),5.8)\n \t\tNO_UNSETENV = YesPlease\n@@ -724,6 +731,7 @@ ifeq ($(uname_O),Cygwin)\n \tNO_D_INO_IN_DIRENT = YesPlease\n \tNO_STRCASESTR = YesPlease\n \tNO_MEMMEM = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tNO_SYMLINK_HEAD = YesPlease\n \tNEEDS_LIBICONV = YesPlease\n \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n@@ -767,11 +775,13 @@ ifeq ($(uname_S),NetBSD)\n \tBASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib\n \tTHREADED_DELTA_SEARCH = YesPlease\n \tUSE_ST_TIMESPEC = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n endif\n ifeq ($(uname_S),AIX)\n \tNO_STRCASESTR=YesPlease\n \tNO_MEMMEM = YesPlease\n \tNO_MKDTEMP = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tNO_STRLCPY = YesPlease\n \tNO_NSEC = YesPlease\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n@@ -787,12 +797,14 @@ endif\n ifeq ($(uname_S),GNU)\n \t# GNU/Hurd\n \tNO_STRLCPY=YesPlease\n+\tNO_MKSTEMPS = YesPlease\n endif\n ifeq ($(uname_S),IRIX64)\n \tNO_IPV6=YesPlease\n \tNO_SETENV=YesPlease\n \tNO_STRCASESTR=YesPlease\n \tNO_MEMMEM = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tNO_STRLCPY = YesPlease\n \tNO_SOCKADDR_STORAGE=YesPlease\n \tSHELL_PATH=/usr/gnu/bin/bash\n@@ -805,6 +817,7 @@ ifeq ($(uname_S),HP-UX)\n \tNO_SETENV=YesPlease\n \tNO_STRCASESTR=YesPlease\n \tNO_MEMMEM = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tNO_STRLCPY = YesPlease\n \tNO_MKDTEMP = YesPlease\n \tNO_UNSETENV = YesPlease\n@@ -834,6 +847,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tNO_C99_FORMAT = YesPlease\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n \tSNPRINTF_RETURNS_BOGUS = YesPlease\n \tNO_SVN_TESTS = YesPlease\n \tNO_PERL_MAKEMAKER = YesPlease\n@@ -853,6 +867,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n endif\n ifneq (,$(findstring arm,$(uname_M)))\n \tARM_SHA1 = YesPlease\n+\tNO_MKSTEMPS = YesPlease\n endif\n \n -include config.mak.autogen\n@@ -1011,6 +1026,10 @@ ifdef NO_MKDTEMP\n \tCOMPAT_CFLAGS += -DNO_MKDTEMP\n \tCOMPAT_OBJS += compat/mkdtemp.o\n endif\n+ifdef NO_MKSTEMPS\n+\tCOMPAT_CFLAGS += -DNO_MKSTEMPS\n+\tCOMPAT_OBJS += compat/mkstemps.o\n+endif\n ifdef NO_UNSETENV\n \tCOMPAT_CFLAGS += -DNO_UNSETENV\n \tCOMPAT_OBJS += compat/unsetenv.o\ndiff --git a/compat/mkstemps.c b/compat/mkstemps.c\nnew file mode 100644\nindex 0000000..87ebc2a\n--- /dev/null\n+++ b/compat/mkstemps.c\n@@ -0,0 +1,67 @@\n+#include \"../git-compat-util.h\"\n+\n+#ifndef TMP_MAX\n+#define TMP_MAX 16384\n+#endif\n+\n+/* Adapted from libiberty's mkstemp.c. */\n+int gitmkstemps(char *pattern, int suffix_len)\n+{\n+\tstatic const char letters[] =\n+\t\t\"abcdefghijklmnopqrstuvwxyz\"\n+\t\t\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+\t\t\"0123456789\";\n+\tstatic const int num_letters = 62;\n+\tuint64_t value;\n+\tstruct timeval tv;\n+\tchar *template;\n+\tsize_t len;\n+\tint fd, count;\n+\n+\tlen = strlen(pattern);\n+\n+\tif (len < 6 + suffix_len) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\tif (strncmp(&pattern[len - 6 - suffix_len], \"XXXXXX\", 6)) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\t/* Replace pattern's XXXXXX characters with randomness.\n+\t * Try TMP_MAX different filenames.\n+\t */\n+\tgettimeofday(&tv, NULL);\n+\tvalue = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();\n+\ttemplate = &pattern[len - 6 - suffix_len];\n+\tfor (count = 0; count < TMP_MAX; ++count) {\n+\t\tuint64_t v = value;\n+\t\t/* Fill in the random bits. */\n+\t\ttemplate[0] = letters[v % num_letters]; v/= num_letters;\n+\t\ttemplate[1] = letters[v % num_letters]; v/= num_letters;\n+\t\ttemplate[2] = letters[v % num_letters]; v/= num_letters;\n+\t\ttemplate[3] = letters[v % num_letters]; v/= num_letters;\n+\t\ttemplate[4] = letters[v % num_letters]; v/= num_letters;\n+\t\ttemplate[5] = letters[v % num_letters]; v/= num_letters;\n+\n+\t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);\n+\t\tif (fd > 0)\n+\t\t\treturn fd;\n+\t\t/* Fatal error (EPERM, ENOSPC etc).\n+\t\t * It doesn't make sense to loop.\n+\t\t */\n+\t\tif (errno != EEXIST)\n+\t\t\tbreak;\n+\t\t/* This is a random value.  It is only necessary that\n+\t\t * the next TMP_MAX values generated by adding 7777 to\n+\t\t * VALUE are different with (module 2^32).\n+\t\t */\n+\t\tvalue += 7777;\n+\t}\n+\t/* We return the null string if we can't find a unique file name.  */\n+\tpattern[0] = '\\0';\n+\terrno = EINVAL;\n+\treturn -1;\n+}\ndiff --git a/config.mak.in b/config.mak.in\nindex 7cce0c1..b6619af 100644\n--- a/config.mak.in\n+++ b/config.mak.in\n@@ -46,6 +46,7 @@ NO_STRTOUMAX=@NO_STRTOUMAX@\n NO_SETENV=@NO_SETENV@\n NO_UNSETENV=@NO_UNSETENV@\n NO_MKDTEMP=@NO_MKDTEMP@\n+NO_MKSTEMPS=@NO_MKSTEMPS@\n NO_ICONV=@NO_ICONV@\n OLD_ICONV=@OLD_ICONV@\n NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@\ndiff --git a/configure.ac b/configure.ac\nindex 4e728bc..95dccd4 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -676,6 +676,12 @@ GIT_CHECK_FUNC(mkdtemp,\n [NO_MKDTEMP=],\n [NO_MKDTEMP=YesPlease])\n AC_SUBST(NO_MKDTEMP)\n+# Define NO_MKSTEMPS if you don't have mkstemps in the C library.\n+GIT_CHECK_FUNC(mkstemps,\n+[NO_MKSTEMPS=],\n+[NO_MKSTEMPS=YesPlease])\n+AC_SUBST(NO_MKSTEMPS)\n+#\n #\n # Define NO_MMAP if you want to avoid mmap.\n #\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c7cf2d5..f7217ad 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -232,6 +232,11 @@ extern int gitsetenv(const char *, const char *, int);\n extern char *gitmkdtemp(char *);\n #endif\n \n+#ifdef NO_MKSTEMPS\n+#define mkstemps gitmkstemps\n+extern int gitmkstemps(char *, int);\n+#endif\n+\n #ifdef NO_UNSETENV\n #define unsetenv gitunsetenv\n extern void gitunsetenv(const char *);\n-- \n1.6.1.3\n"},{"id":"114976","messageId":"1243558164-74756-2-git-send-email-davvid@gmail.com","threadId":"19566","inReplyTo":"1243558164-74756-1-git-send-email-davvid@gmail.com","subject":"[PATCH v4 2/2] diff: generate pretty filenames in prep_temp_blob()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-29T00:49:24Z","receivedAt":"2009-05-29T00:49:24Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Naturally, prep_temp_blob() did not care about filenames.\nAs a result, scripts that used GIT_EXTERNAL_DIFF or textconv\nended up with filenames such as \".diff_XXXXXX\".\n\nThis modifies prep_temp_blob() to generate user-friendly\nfilenames when creating temporary files.\n\nDiffing \"name.ext\" now generates \"XXXXXX_name.ext\".\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\nTested-by: Johannes Sixt <j6t@kdbg.org> (Windows)\n---\n\nThis includes Peff's suggestion to make this the\ndefault behavior.\n\nPeff mentioned that: \"Switching to user-friendly\ntemp filenames for textconv was on my todo list\".\n\nThis patch accomplishes exactly that for both\ntextconv and GIT_EXTERNAL_DIFF.\n\n cache.h                  |    2 ++\n diff.c                   |   19 ++++++++++++++++++-\n path.c                   |   16 ++++++++++++++++\n t/t4020-diff-external.sh |    9 +++++++++\n 4 files changed, 45 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b8503ad..871c984 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -614,6 +614,8 @@ extern int is_empty_blob_sha1(const unsigned char *sha1);\n \n int git_mkstemp(char *path, size_t n, const char *template);\n \n+int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);\n+\n /*\n  * NOTE NOTE NOTE!!\n  *\ndiff --git a/diff.c b/diff.c\nindex dcfbcb0..85503ce 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1964,8 +1964,24 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf template = STRBUF_INIT;\n+\tchar *basename = ((char*)path) + strlen(path) - 1;\n+\n+\t/* Windows lacks basename() */\n+\twhile(*basename && basename > path) {\n+\t\tbasename--;\n+\t\tif (is_dir_sep(*basename)) {\n+\t\t\tbasename++;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\t/* Generate \"XXXXXX_basename.ext\" */\n+\tstrbuf_addstr(&template, \"XXXXXX_\");\n+\tstrbuf_addstr(&template, basename);\n \n-\tfd = git_mkstemp(temp->tmp_path, PATH_MAX, \".diff_XXXXXX\");\n+\tfd = git_mkstemps(temp->tmp_path, PATH_MAX, template.buf,\n+\t\t\tstrlen(basename) + 1);\n \tif (fd < 0)\n \t\tdie(\"unable to create temp-file: %s\", strerror(errno));\n \tif (convert_to_working_tree(path,\n@@ -1981,6 +1997,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \ttemp->hex[40] = 0;\n \tsprintf(temp->mode, \"%06o\", mode);\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&template);\n }\n \n static struct diff_tempfile *prepare_temp_file(const char *name,\ndiff --git a/path.c b/path.c\nindex 8a0a674..047fdb0 100644\n--- a/path.c\n+++ b/path.c\n@@ -139,6 +139,22 @@ int git_mkstemp(char *path, size_t len, const char *template)\n \treturn mkstemp(path);\n }\n \n+/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */\n+int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)\n+{\n+\tconst char *tmp;\n+\tsize_t n;\n+\n+\ttmp = getenv(\"TMPDIR\");\n+\tif (!tmp)\n+\t\ttmp = \"/tmp\";\n+\tn = snprintf(path, len, \"%s/%s\", tmp, template);\n+\tif (len <= n) {\n+\t\terrno = ENAMETOOLONG;\n+\t\treturn -1;\n+\t}\n+\treturn mkstemps(path, suffix_len);\n+}\n \n int validate_headref(const char *path)\n {\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 0720001..4ea42e0 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -136,6 +136,15 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '\n \tGIT_EXTERNAL_DIFF=echo git diff\n '\n \n+test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '\n+\ttouch file.ext &&\n+\tgit add file.ext &&\n+\techo with extension > file.ext &&\n+\tGIT_EXTERNAL_DIFF=echo git diff file.ext | grep ......_file\\.ext &&\n+\tgit update-index --force-remove file.ext &&\n+\trm file.ext\n+'\n+\n echo \"#!$SHELL_PATH\" >fake-diff.sh\n cat >> fake-diff.sh <<\\EOF\n cat $2 >> crlfed.txt\n-- \n1.6.1.3\n"},{"id":"115041","messageId":"20090529195537.GA13961@coredump.intra.peff.net","threadId":"19566","inReplyTo":"1243558164-74756-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v4 2/2] diff: generate pretty filenames in prep_temp_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-29T19:55:37Z","receivedAt":"2009-05-29T19:55:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 28, 2009 at 05:49:24PM -0700, David Aguilar wrote:\n\n> @@ -1964,8 +1964,24 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n>  {\n>  \tint fd;\n>  \tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf template = STRBUF_INIT;\n> +\tchar *basename = ((char*)path) + strlen(path) - 1;\n\nWhy do you drop constness in this assignment?\n\n> +\t/* Windows lacks basename() */\n> +\twhile(*basename && basename > path) {\n> +\t\tbasename--;\n> +\t\tif (is_dir_sep(*basename)) {\n> +\t\t\tbasename++;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nThis is such an easily-factorable bit, maybe it makes sense to add as\nbasename() in compat/?\n\n-Peff\n"},{"id":"115062","messageId":"20090530012204.GA26210@gmail.com","threadId":"19566","inReplyTo":"20090529195537.GA13961@coredump.intra.peff.net","subject":"Re: [PATCH v4 2/2] diff: generate pretty filenames in prep_temp_blob()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-30T01:22:05Z","receivedAt":"2009-05-30T01:22:05Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, May 29, 2009 at 03:55:37PM -0400, Jeff King wrote:\n> On Thu, May 28, 2009 at 05:49:24PM -0700, David Aguilar wrote:\n> \n> > @@ -1964,8 +1964,24 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n> >  {\n> >  \tint fd;\n> >  \tstruct strbuf buf = STRBUF_INIT;\n> > +\tstruct strbuf template = STRBUF_INIT;\n> > +\tchar *basename = ((char*)path) + strlen(path) - 1;\n> \n> Why do you drop constness in this assignment?\n\n\nThat was a mistake.\n\n\n> > +\t/* Windows lacks basename() */\n> > +\twhile(*basename && basename > path) {\n> > +\t\tbasename--;\n> > +\t\tif (is_dir_sep(*basename)) {\n> > +\t\t\tbasename++;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> \n> This is such an easily-factorable bit, maybe it makes sense to add as\n> basename() in compat/?\n\nI would feel a little better calling it\ngit_basename(const char *) and keeping it in path.c\n(or another name if basename is no good)\n\nThe reason is from the manpage for basename:\n\n\"Both dirname() and basename() may modify the\ncontents of path, so it may be desirable to\npass a copy when calling one of these functions.\"\n\nThe above snippet is much simpler and does not have\nthat restriction since we can guarantee const-ness.\n\nAll this version cares about is being able to find the\nlast dir_sep and returning a pointer.\n\nWhat do you think?\n\n-- \n\n\tDavid\n"},{"id":"115064","messageId":"20090530014751.GA15948@coredump.intra.peff.net","threadId":"19566","inReplyTo":"20090530012204.GA26210@gmail.com","subject":"Re: [PATCH v4 2/2] diff: generate pretty filenames in prep_temp_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-30T01:47:51Z","receivedAt":"2009-05-30T01:47:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 29, 2009 at 06:22:05PM -0700, David Aguilar wrote:\n\n> > This is such an easily-factorable bit, maybe it makes sense to add as\n> > basename() in compat/?\n> \n> I would feel a little better calling it\n> git_basename(const char *) and keeping it in path.c\n> [...]\n> All this version cares about is being able to find the\n> last dir_sep and returning a pointer.\n> \n> What do you think?\n\nThat sounds good to me.\n\n-Peff\n"}]}