{"thread":{"id":"19550","subject":"[PATCH v2 1/2] compat: add a mkstemps() compatibility function","startedAt":"2009-05-28T06:11:16Z","lastAt":"2009-05-28T22:06:06Z","messageCount":7,"participants":["David Aguilar","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"114888","messageId":"1243491077-27738-1-git-send-email-davvid@gmail.com","threadId":"19550","inReplyTo":null,"subject":"[PATCH v2 1/2] compat: add a mkstemps() compatibility function","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-28T06:11:16Z","receivedAt":"2009-05-28T06:11:16Z","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>\n---\n\nThis includes Jakub's autoconf fixups.\n\n Makefile          |   19 +++++++++++++\n compat/mkstemps.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n config.mak.in     |    1 +\n configure.ac      |    6 ++++\n git-compat-util.h |    5 +++\n 5 files changed, 109 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..10f9ed6\n--- /dev/null\n+++ b/compat/mkstemps.c\n@@ -0,0 +1,78 @@\n+#include <string.h>\n+#include <errno.h>\n+#include <stdio.h>\n+#include <fcntl.h>\n+#include <inttypes.h>\n+#include <unistd.h>\n+#include <sys/time.h>\n+#include <sys/types.h>\n+\n+#ifndef TMP_MAX\n+#define TMP_MAX 16384\n+#endif\n+\n+#ifndef O_BINARY\n+#define O_BINARY 0\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_BINARY|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.3.1.30.g55524\n"},{"id":"114889","messageId":"1243491077-27738-2-git-send-email-davvid@gmail.com","threadId":"19550","inReplyTo":"1243491077-27738-1-git-send-email-davvid@gmail.com","subject":"[PATCH v2 2/2] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-28T06:11:17Z","receivedAt":"2009-05-28T06:11:17Z","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 use GIT_EXTERNAL_DIFF ended up\nwith filenames such as \".diff_XXXXXX\".\n\nThis specializes the GIT_EXTERNAL_DIFF code to generate\nuser-friendly filenames when creating temporary files.\n\nDiffing \"name.ext\" now generates \"XXXXXX_name.ext\".\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n\nThis includes Hannes' suggestion to avoid basename()\nfor Windows portability.\n\n cache.h                  |    2 ++\n diff.c                   |   45 +++++++++++++++++++++++++++++++++++++--------\n path.c                   |   19 +++++++++++++++++++\n t/t4020-diff-external.sh |    9 +++++++++\n 4 files changed, 67 insertions(+), 8 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..60f07a0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1960,12 +1960,37 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \t\t\t   void *blob,\n \t\t\t   unsigned long size,\n \t\t\t   const unsigned char *sha1,\n-\t\t\t   int mode)\n+\t\t\t   int mode,\n+\t\t\t   int pretty_filename)\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfd = git_mkstemp(temp->tmp_path, PATH_MAX, \".diff_XXXXXX\");\n+\tif (pretty_filename) {\n+\t\t/* Generate \"XXXXXX_filename\" */\n+\t\tstruct strbuf pretty_name = STRBUF_INIT;\n+\t\tchar *basename = ((char*)path) + strlen(path) - 1;\n+\n+\t\t/* Windows lacks basename() */\n+\t\twhile(*basename && basename > path) {\n+\t\t\tbasename--;\n+\t\t\tif (is_dir_sep(*basename)) {\n+\t\t\t\tbasename++;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addstr(&pretty_name, \"XXXXXX_\");\n+\t\tstrbuf_addstr(&pretty_name, basename);\n+\n+\t\tfd = git_mkstemps(temp->tmp_path, PATH_MAX,\n+\t\t\tpretty_name.buf, strlen(basename) + 1);\n+\n+\t\tstrbuf_release(&pretty_name);\n+\t}\n+\telse {\n+\t\tfd = git_mkstemp(temp->tmp_path, PATH_MAX, \".diff_XXXXXX\");\n+\t}\n \tif (fd < 0)\n \t\tdie(\"unable to create temp-file: %s\", strerror(errno));\n \tif (convert_to_working_tree(path,\n@@ -1984,7 +2009,8 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n }\n \n static struct diff_tempfile *prepare_temp_file(const char *name,\n-\t\tstruct diff_filespec *one)\n+\t\tstruct diff_filespec *one,\n+\t\tint pretty_filename)\n {\n \tstruct diff_tempfile *temp = claim_diff_tempfile();\n \n@@ -2021,7 +2047,8 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\t\t\t       (one->sha1_valid ?\n \t\t\t\t\tone->sha1 : null_sha1),\n \t\t\t\t       (one->sha1_valid ?\n-\t\t\t\t\tone->mode : S_IFLNK));\n+\t\t\t\t\tone->mode : S_IFLNK),\n+\t\t\t\t       pretty_filename);\n \t\t\tstrbuf_release(&sb);\n \t\t}\n \t\telse {\n@@ -2045,7 +2072,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\tif (diff_populate_filespec(one, 0))\n \t\t\tdie(\"cannot read data blob for %s\", one->path);\n \t\tprep_temp_blob(name, temp, one->data, one->size,\n-\t\t\t       one->sha1, one->mode);\n+\t\t\t       one->sha1, one->mode, pretty_filename);\n \t}\n \treturn temp;\n }\n@@ -2071,8 +2098,9 @@ static void run_external_diff(const char *pgm,\n \tif (one && two) {\n \t\tstruct diff_tempfile *temp_one, *temp_two;\n \t\tconst char *othername = (other ? other : name);\n-\t\ttemp_one = prepare_temp_file(name, one);\n-\t\ttemp_two = prepare_temp_file(othername, two);\n+\t\tint pretty_filename = 1;\n+\t\ttemp_one = prepare_temp_file(name, one, pretty_filename);\n+\t\ttemp_two = prepare_temp_file(othername, two, pretty_filename);\n \t\t*arg++ = pgm;\n \t\t*arg++ = name;\n \t\t*arg++ = temp_one->name;\n@@ -3574,8 +3602,9 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \tconst char **arg = argv;\n \tstruct child_process child;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tint pretty_filename = 0;\n \n-\ttemp = prepare_temp_file(spec->path, spec);\n+\ttemp = prepare_temp_file(spec->path, spec, pretty_filename);\n \t*arg++ = pgm;\n \t*arg++ = temp->name;\n \t*arg = NULL;\ndiff --git a/path.c b/path.c\nindex 8a0a674..090b490 100644\n--- a/path.c\n+++ b/path.c\n@@ -140,6 +140,25 @@ int git_mkstemp(char *path, size_t len, const char *template)\n }\n \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+\n int validate_headref(const char *path)\n {\n \tstruct stat st;\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.3.1.30.g55524\n"},{"id":"114892","messageId":"4A1E3834.3020502@viscovery.net","threadId":"19550","inReplyTo":"1243491077-27738-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 1/2] compat: add a mkstemps() compatibility function","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-05-28T07:07:32Z","receivedAt":"2009-05-28T07:07:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"David Aguilar schrieb:\n> +++ b/compat/mkstemps.c\n> @@ -0,0 +1,78 @@\n> +#include <string.h>\n> +#include <errno.h>\n> +#include <stdio.h>\n> +#include <fcntl.h>\n> +#include <inttypes.h>\n> +#include <unistd.h>\n> +#include <sys/time.h>\n> +#include <sys/types.h>\n...\n> +#ifndef O_BINARY\n> +#define O_BINARY 0\n> +#endif\n...\n> +\t\tfd = open(pattern, O_BINARY|O_CREAT|O_EXCL|O_RDWR, 0600);\n\nYou should not include \"random\" system headers, nor has mkstemps any\nbusiness deciding whether files are opened in binary mode. We are not\nusing O_BINARY anywhere else (except in compat/mingw.c). With the patch\nbelow squashed in (I hope it won't be wrapped) you can add:\n\nTested-by: Johannes Sixt <j6t@kdbg.org> (Windows)\n\nto both your patches. And, yes, I like them :-)\n\n-- Hannes\n\ndiff --git a/compat/mkstemps.c b/compat/mkstemps.c\nindex 10f9ed6..1cf7f3d 100644\n--- a/compat/mkstemps.c\n+++ b/compat/mkstemps.c\n@@ -1,20 +1,9 @@\n-#include <string.h>\n-#include <errno.h>\n-#include <stdio.h>\n-#include <fcntl.h>\n-#include <inttypes.h>\n-#include <unistd.h>\n-#include <sys/time.h>\n-#include <sys/types.h>\n+#include \"../git-compat-util.h\"\n\n #ifndef TMP_MAX\n #define TMP_MAX 16384\n #endif\n\n-#ifndef O_BINARY\n-#define O_BINARY 0\n-#endif\n-\n /* Adapted from libiberty's mkstemp.c. */\n int gitmkstemps(char *pattern, int suffix_len)\n {\n@@ -57,7 +46,7 @@ int gitmkstemps(char *pattern, int suffix_len)\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_BINARY|O_CREAT|O_EXCL|O_RDWR, 0600);\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"},{"id":"114908","messageId":"20090528093152.GA12566@gmail.com","threadId":"19550","inReplyTo":"4A1E3834.3020502@viscovery.net","subject":"Re: [PATCH v2 1/2] compat: add a mkstemps() compatibility function","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-28T09:31:53Z","receivedAt":"2009-05-28T09:31:53Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, May 28, 2009 at 09:07:32AM +0200, Johannes Sixt wrote:\n> David Aguilar schrieb:\n> \n> You should not include \"random\" system headers, nor has mkstemps any\n> business deciding whether files are opened in binary mode. We are not\n> using O_BINARY anywhere else (except in compat/mingw.c). With the patch\n> below squashed in (I hope it won't be wrapped) you can add:\n> \n> Tested-by: Johannes Sixt <j6t@kdbg.org> (Windows)\n> \n> to both your patches. And, yes, I like them :-)\n> \n> -- Hannes\n\n\nThanks.\nI'll squash these in and resend 1/2 in a moment.\n\n\n-- \n\n\tDavid\n"},{"id":"114936","messageId":"20090528174436.GA12723@coredump.intra.peff.net","threadId":"19550","inReplyTo":"1243491077-27738-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 2/2] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-28T17:44:36Z","receivedAt":"2009-05-28T17:44:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2009 at 11:11:17PM -0700, David Aguilar wrote:\n\n> +int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);\n\nFWIW, I find this name not very descriptive. From the name, I would\nexpect it to do the exact same thing as mkstemps, but be our own\npersonal implementation. But it is actually a wrapper that behaves\nsomewhat differently. So I wonder if \"mkstemps_tmpdir\" or something\nwould be a better name.\n\n> +\tif (pretty_filename) {\n> +\t\t/* Generate \"XXXXXX_filename\" */\n\nIs there a reason _not_ to always just use the pretty filename? It looks\nlike you turn it on for external diff, but off for textconv. I don't\nthink there is a reason not to use it for textconv.\n\nIs there some other code path that changes it that I'm missing?\n\n> +\t\tint pretty_filename = 1;\n> +\t\ttemp_one = prepare_temp_file(name, one, pretty_filename);\n> +\t\ttemp_two = prepare_temp_file(othername, two, pretty_filename);\n\nI think this reads much better as just:\n\n  temp_one = prepare_temp_file(name, one, 1);\n  temp_two = prepare_temp_file(othername, two, 1);\n\nThen it eliminates one bit of state for the reader to keep track of; I\ndon't have to wonder if pretty_filename might ever change. If I care\nabout what the '1' means, I can go look at the definition of\nprepare_temp_file (or if you really want to make it more\nself-documenting, make it a \"flags\" field and set the\nUSE_PRETTY_FILENAME flag).\n\nHowever, I suspect that all callers should use pretty filenames, and\nthen this bit would just go away.\n\n> +\tint pretty_filename = 0;\n>  \n> -\ttemp = prepare_temp_file(spec->path, spec);\n> +\ttemp = prepare_temp_file(spec->path, spec, pretty_filename);\n\nDitto here.\n\n-Peff\n"},{"id":"114970","messageId":"20090528213049.GA55167@gmail.com","threadId":"19550","inReplyTo":"20090528174436.GA12723@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-28T21:30:49Z","receivedAt":"2009-05-28T21:30:49Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, May 28, 2009 at 01:44:36PM -0400, Jeff King wrote:\n> On Wed, May 27, 2009 at 11:11:17PM -0700, David Aguilar wrote:\n> \n> > +int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);\n> \n> FWIW, I find this name not very descriptive. From the name, I would\n> expect it to do the exact same thing as mkstemps, but be our own\n> personal implementation. But it is actually a wrapper that behaves\n> somewhat differently. So I wonder if \"mkstemps_tmpdir\" or something\n> would be a better name.\n\nIt does exactly what git_mkstemp() does, plus the extra\nsuffix_len parameter.  If we rename this function we have to\nrename both.\n\n\n> Is there a reason _not_ to always just use the pretty filename? It looks\n> like you turn it on for external diff, but off for textconv. I don't\n> think there is a reason not to use it for textconv.\n\nI was not aware of the other code paths and only wanted to\naffect the one that I knew about.  I agree that making that the\ndefault behavior would be great, meaning we could drop the\npretty_filename flag altogether.\n\nIf you and others agree that the user-friendly names are a good\nthing to have by default then I can rework patch 2/2.\n\n> However, I suspect that all callers should use pretty filenames, and\n> then this bit would just go away.\n\nI fully agree.\n\n\n-- \n\t\tDavid\n"},{"id":"114973","messageId":"20090528220606.GA24148@coredump.intra.peff.net","threadId":"19550","inReplyTo":"20090528213049.GA55167@gmail.com","subject":"Re: [PATCH v2 2/2] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-28T22:06:06Z","receivedAt":"2009-05-28T22:06:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 28, 2009 at 02:30:49PM -0700, David Aguilar wrote:\n\n> > FWIW, I find this name not very descriptive. From the name, I would\n> > expect it to do the exact same thing as mkstemps, but be our own\n> > personal implementation. But it is actually a wrapper that behaves\n> > somewhat differently. So I wonder if \"mkstemps_tmpdir\" or something\n> > would be a better name.\n> \n> It does exactly what git_mkstemp() does, plus the extra\n> suffix_len parameter.  If we rename this function we have to\n> rename both.\n\nAh. Well, I think that is a crappy name, too, then, but it is not your\nfault. ;) So if there is precedent, I guess somebody else thought it was\na good idea, and you should leave your patch as-is.\n\n> I was not aware of the other code paths and only wanted to\n> affect the one that I knew about.  I agree that making that the\n> default behavior would be great, meaning we could drop the\n> pretty_filename flag altogether.\n> \n> If you and others agree that the user-friendly names are a good\n> thing to have by default then I can rework patch 2/2.\n\nSwitching to user-friendly temp filenames for textconv was on my todo\nlist, so I definitely think it is a good idea.\n\n-Peff\n"}]}