{"thread":{"id":"19509","subject":"[PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","startedAt":"2009-05-26T05:36:05Z","lastAt":"2009-05-26T21:41:28Z","messageCount":7,"participants":["David Aguilar","Stephen Boyd","Junio C Hamano","Markus Heidelberg"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"114730","messageId":"1243316165-99065-1-git-send-email-davvid@gmail.com","threadId":"19509","inReplyTo":null,"subject":"[PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-26T05:36:05Z","receivedAt":"2009-05-26T05:36:05Z","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\nprettier filenames.\n\nDiffing \"name.ext\" now generates \"name.XXXX.ext\".\nDiffing files with no extension now generates \"name_XXXX\".\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n\nI renamed a few variables and unrolled the inner loop in\ngit_mkstemps() since the last patch to make things easier to read/review.\n\nWe might as well go with this one if we haven't applied the\nprevious versions of this patch. \n\n cache.h                  |    2 +\n diff.c                   |   55 +++++++++++++++++++++++++++++++-----\n path.c                   |   70 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t4020-diff-external.sh |   18 ++++++++++++\n 4 files changed, 137 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 f06876b..226771d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1960,12 +1960,47 @@ 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\tstruct strbuf pretty_name = STRBUF_INIT;\n+\t\tchar *pathdup = xstrdup(path);\n+\t\tchar *base = basename(pathdup);\n+\t\tchar *dot = strchr(base, '.');\n+\t\tint suffix_len = 0;\n+\n+\t\tif (dot) {\n+\t\t\t/* path has an extension, e.g. \"foo.txt\";\n+\t\t\t * generate \"foo.XXXX.txt\".\n+\t\t\t */\n+\t\t\t*dot = '\\0';\n+\t\t\tstrbuf_addstr(&pretty_name, base);\n+\t\t\t*dot = '.';\n+\t\t\tstrbuf_addstr(&pretty_name, \".XXXXXX\");\n+\t\t\tsuffix_len = strlen(dot);\n+\t\t\tstrbuf_addstr(&pretty_name, dot);\n+\t\t}\n+\t\telse {\n+\t\t\t/* path has no extension, e.g. \"Makefile\";\n+\t\t\t * generate \"Makefile_XXXX\".\n+\t\t\t */\n+\t\t\tstrbuf_addstr(&pretty_name, base);\n+\t\t\tstrbuf_addstr(&pretty_name, \"_XXXXXX\");\n+\t\t}\n+\n+\t\tfd = git_mkstemps(temp->tmp_path, PATH_MAX,\n+\t\t\tpretty_name.buf, suffix_len);\n+\n+\t\tfree(pathdup);\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 +2019,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@@ -2025,7 +2061,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}\n \t\telse {\n \t\t\t/* we can borrow from the file in the work tree */\n@@ -2048,7 +2085,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@@ -2074,8 +2111,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@@ -3577,8 +3615,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..6292410 100644\n--- a/path.c\n+++ b/path.c\n@@ -12,6 +12,8 @@\n  */\n #include \"cache.h\"\n \n+#define GIT_MKSTEMPS_MAX 16384\n+\n static char bad_path[] = \"/bad-path/\";\n \n static char *get_pathname(void)\n@@ -140,6 +142,74 @@ 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+ * This is adapted from libguile's GPLv2 mkstemp.c.\n+ */\n+int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)\n+{\n+\tstatic const char letters[] =\n+\t\t\"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+\t\t\"0123456789_\";\n+\tconst char *tmp;\n+\tchar *pattern;\n+\tsize_t n;\n+\tsize_t path_noext_len, letters_len;\n+\tstruct timeval tv;\n+\tuint64_t value, v;\n+\tint fd, count;\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+\n+\tletters_len = strlen(letters);\n+\tpath_noext_len = n - suffix_len;\n+\n+\tif (path_noext_len < 0 || strlen(template) - suffix_len < 6) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\tif (strncmp(path + path_noext_len - 6, \"XXXXXX\", 6)) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\t/* Replace template's XXXXXX characters with randomness.\n+\t * Try GIT_MKSTEMPS_MAX different filenames.\n+\t */\n+\tgettimeofday(&tv, NULL);\n+\tvalue = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();\n+\tpattern = path + path_noext_len - 6;\n+\tfor (count = 0; count < GIT_MKSTEMPS_MAX; ++count) {\n+\t\tv = value;\n+\t\tpattern[0] = letters[v % letters_len]; v/= letters_len;\n+\t\tpattern[1] = letters[v % letters_len]; v/= letters_len;\n+\t\tpattern[2] = letters[v % letters_len]; v/= letters_len;\n+\t\tpattern[3] = letters[v % letters_len]; v/= letters_len;\n+\t\tpattern[4] = letters[v % letters_len]; v/= letters_len;\n+\t\tpattern[5] = letters[v % letters_len]; v/= letters_len;\n+\n+\t\tfd = open(path, O_CREAT | O_EXCL | O_RDWR, 0600);\n+\t\tif (fd > 0)\n+\t\t\treturn fd;\n+\t\t/* This is a random value.  It is only necessary that\n+\t\t * the next value generated by adding 7777 is different\n+\t\t * modulus 2^32.\n+\t\t */\n+\t\tvalue += 7777;\n+\t}\n+\terrno = EINVAL;\n+\treturn -1;\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..a9fdcb0 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -136,6 +136,24 @@ 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 with no ext' '\n+\ttouch filenoext &&\n+\tgit add filenoext &&\n+\techo no extension > filenoext &&\n+\tGIT_EXTERNAL_DIFF=echo git diff filenoext | grep filenoext_ &&\n+\tgit update-index --force-remove filenoext &&\n+\trm filenoext\n+'\n+\n+test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths with ext' '\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":"114732","messageId":"4A1B85B8.1050907@gmail.com","threadId":"19509","inReplyTo":"1243316165-99065-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-05-26T06:01:28Z","receivedAt":"2009-05-26T06:01:28Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"David Aguilar wrote:\n> -\tfd = git_mkstemp(temp->tmp_path, PATH_MAX, \".diff_XXXXXX\");\n> +\tif (pretty_filename) {\n> +\t\tstruct strbuf pretty_name = STRBUF_INIT;\n> +\t\tchar *pathdup = xstrdup(path);\n> +\t\tchar *base = basename(pathdup);\n> +\t\tchar *dot = strchr(base, '.');\n> +\t\tint suffix_len = 0;\n> +\n> +\t\tif (dot) {\n> +\t\t\t/* path has an extension, e.g. \"foo.txt\";\n> +\t\t\t * generate \"foo.XXXX.txt\".\n> +\t\t\t */\n> +\t\t\t*dot = '\\0';\n> +\t\t\tstrbuf_addstr(&pretty_name, base);\n> +\t\t\t*dot = '.';\n> +\t\t\tstrbuf_addstr(&pretty_name, \".XXXXXX\");\n> +\t\t\tsuffix_len = strlen(dot);\n> +\t\t\tstrbuf_addstr(&pretty_name, dot);\n\nThis *dot business annoys me. Would it be better to use strbuf_add()\nwith some pointer math thrown in? Also, what happens with files such as\n\"foo.bar.txt\"? Do we want \"foo.XXXXX.bar.txt\"?\n"},{"id":"114733","messageId":"7vr5ycqtog.fsf@alter.siamese.dyndns.org","threadId":"19509","inReplyTo":"1243316165-99065-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-26T06:12:15Z","receivedAt":"2009-05-26T06:12:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Naturally, prep_temp_blob() did not care about filenames.\n> As a result, scripts that use GIT_EXTERNAL_DIFF ended up\n> with filenames such as \".diff_XXXXXX\".\n>\n> This specializes the GIT_EXTERNAL_DIFF code to generate\n> prettier filenames.\n>\n> Diffing \"name.ext\" now generates \"name.XXXX.ext\".\n> Diffing files with no extension now generates \"name_XXXX\".\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>\n> I renamed a few variables and unrolled the inner loop in\n> git_mkstemps() since the last patch to make things easier to read/review.\n\nThanks.\n\nI actually think using mkstemps() where the function is natively\navailable, and using compat/mkstemps.c implementation where it is not, is\na saner approach.\n\nIt would make the implementation of git_mkstemps() easier to read, because\nthe interface to mkstemps(), even though it may not be in POSIX (nor in\nglibc), is a well known quantity and people do not need to follow into its\nimplementation when they want to follow the logic of higher level code.\n"},{"id":"114742","messageId":"20090526173726.GA5528@gmail.com","threadId":"19509","inReplyTo":"4A1B85B8.1050907@gmail.com","subject":"Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-26T17:37:29Z","receivedAt":"2009-05-26T17:37:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, May 25, 2009 at 11:01:28PM -0700, Stephen Boyd wrote:\n> David Aguilar wrote:\n> > -\tfd = git_mkstemp(temp->tmp_path, PATH_MAX, \".diff_XXXXXX\");\n> > +\tif (pretty_filename) {\n> > +\t\tstruct strbuf pretty_name = STRBUF_INIT;\n> > +\t\tchar *pathdup = xstrdup(path);\n> > +\t\tchar *base = basename(pathdup);\n> > +\t\tchar *dot = strchr(base, '.');\n> > +\t\tint suffix_len = 0;\n> > +\n> > +\t\tif (dot) {\n> > +\t\t\t/* path has an extension, e.g. \"foo.txt\";\n> > +\t\t\t * generate \"foo.XXXX.txt\".\n> > +\t\t\t */\n> > +\t\t\t*dot = '\\0';\n> > +\t\t\tstrbuf_addstr(&pretty_name, base);\n> > +\t\t\t*dot = '.';\n> > +\t\t\tstrbuf_addstr(&pretty_name, \".XXXXXX\");\n> > +\t\t\tsuffix_len = strlen(dot);\n> > +\t\t\tstrbuf_addstr(&pretty_name, dot);\n> \n> This *dot business annoys me. Would it be better to use strbuf_add()\n> with some pointer math thrown in? Also, what happens with files such as\n> \"foo.bar.txt\"? Do we want \"foo.XXXXX.bar.txt\"?\n\n\nThat was intentional; \"foo.tar.gz\" becomes \"foo.XXXXXX.tar.gz\",\nwhich is what I considered better behavior when writing it.\n\nThanks for the note about using strbuf_add().  I'll see if we\ncan simplify things by using it instead of strbuf_addstr().\n\nI should probably split the \"add compat/mkstemps\" and\n\"use it in git_mkstemps()\" parts into separate commits since\nwe're going to want to use the native mkstemps implementation if\nit's available.\n\n-- \n\t\tDavid\n"},{"id":"114744","messageId":"200905262231.50892.markus.heidelberg@web.de","threadId":"19509","inReplyTo":"1243316165-99065-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-05-26T20:31:50Z","receivedAt":"2009-05-26T20:31:50Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"David Aguilar, 26.05.2009:\n> Diffing \"name.ext\" now generates \"name.XXXX.ext\".\n> Diffing files with no extension now generates \"name_XXXX\".\n\nWould it simplify the patch when using \"name_XXXX.ext\" and \"name_XXXX\"?\nOr what about \"XXXX_name.ext\" and \"XXXX_name\"? Then you don't even have\nto search for the dot and it has the advantage to show the original\nfilename without some random letters in the middle of it.\n\nMarkus\n"},{"id":"114745","messageId":"20090526205230.GA66544@gmail.com","threadId":"19509","inReplyTo":"200905262231.50892.markus.heidelberg@web.de","subject":"Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-05-26T20:52:31Z","receivedAt":"2009-05-26T20:52:31Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, May 26, 2009 at 10:31:50PM +0200, Markus Heidelberg wrote:\n> David Aguilar, 26.05.2009:\n> > Diffing \"name.ext\" now generates \"name.XXXX.ext\".\n> > Diffing files with no extension now generates \"name_XXXX\".\n> \n> Would it simplify the patch when using \"name_XXXX.ext\" and \"name_XXXX\"?\n> Or what about \"XXXX_name.ext\" and \"XXXX_name\"? Then you don't even have\n> to search for the dot and it has the advantage to show the original\n> filename without some random letters in the middle of it.\n> \n> Markus\n> \n\nIn that case suffix_len is always len(basename) + 1\nand there's less strbuf juggling to do.\nThat is simpler.  Let's do that.\n\nMy next patch series will be two parts:\npart 1: add compat/mkstemps.c\npart 2: use it in prep_temp_blob() via git_mkstemps()\n\nPart 1 has me a little nervous because I'm not sure which\nplatforms in the Makefile need the\n\tNO_MKSTEMPS = YesPlease\nsetting.  I only have access to mac + linux, so I can do the\nright thing there, but I'll have to make a best guess for the\nrest of the platforms.\n\nPerhaps the best answer is to set them all to YesPlease except\nfor mac and openbsd, which I know have native implementations.\n\n-- \n\t\tDavid\n"},{"id":"114747","messageId":"7v1vqbpmnr.fsf@alter.siamese.dyndns.org","threadId":"19509","inReplyTo":"20090526205230.GA66544@gmail.com","subject":"Re: [PATCH v3] diff: generate prettier filenames when using GIT_EXTERNAL_DIFF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-26T21:41:28Z","receivedAt":"2009-05-26T21:41:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Perhaps the best answer is to set them all to YesPlease except\n> for mac and openbsd, which I know have native implementations.\n\nSome vintage of Solaris seems to have it:\n\n    http://docs.sun.com/app/docs/doc/819-2243/mkstemps-3c?a=view\n\nThe notes section of\n\n    http://developer.apple.com/documentation/Darwin/Reference/ManPages/man3/mkstemps.3.html\n\nsays\n\n    The mkstemps() function first appeared in OpenBSD 2.4, and later in\n     FreeBSD 3.4.\n"}]}