{"thread":{"id":"57769","subject":"[PATCH 1/2] tempfile: add mks_tempfile_dt()","startedAt":"2022-04-20T20:26:45Z","lastAt":"2022-04-20T20:30:27Z","messageCount":2,"participants":["René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"454000","messageId":"1c07dad1-3d3d-c52b-4440-631b46520254@web.de","threadId":"57769","inReplyTo":null,"subject":"[PATCH 1/2] tempfile: add mks_tempfile_dt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-20T20:26:09Z","receivedAt":"2022-04-20T20:26:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function to create a temporary file with a certain name in a\ntemporary directory created using mkdtemp(3).  Its result is more\nsightly than the paths created by mks_tempfile_ts(), which include\na random prefix.  That's useful for files passed to a program that\ndisplays their name, e.g. an external diff tool.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n tempfile.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n tempfile.h | 13 +++++++++++\n 2 files changed, 76 insertions(+)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 94aa18f3f7..2024c82691 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -56,6 +56,20 @@\n\n static VOLATILE_LIST_HEAD(tempfile_list);\n\n+static void remove_template_directory(struct tempfile *tempfile,\n+\t\t\t\t      int in_signal_handler)\n+{\n+\tif (tempfile->directorylen > 0 &&\n+\t    tempfile->directorylen < tempfile->filename.len &&\n+\t    tempfile->filename.buf[tempfile->directorylen] == '/') {\n+\t\tstrbuf_setlen(&tempfile->filename, tempfile->directorylen);\n+\t\tif (in_signal_handler)\n+\t\t\trmdir(tempfile->filename.buf);\n+\t\telse\n+\t\t\trmdir_or_warn(tempfile->filename.buf);\n+\t}\n+}\n+\n static void remove_tempfiles(int in_signal_handler)\n {\n \tpid_t me = getpid();\n@@ -74,6 +88,7 @@ static void remove_tempfiles(int in_signal_handler)\n \t\t\tunlink(p->filename.buf);\n \t\telse\n \t\t\tunlink_or_warn(p->filename.buf);\n+\t\tremove_template_directory(p, in_signal_handler);\n\n \t\tp->active = 0;\n \t}\n@@ -100,6 +115,7 @@ static struct tempfile *new_tempfile(void)\n \ttempfile->owner = 0;\n \tINIT_LIST_HEAD(&tempfile->list);\n \tstrbuf_init(&tempfile->filename, 0);\n+\ttempfile->directorylen = 0;\n \treturn tempfile;\n }\n\n@@ -198,6 +214,52 @@ struct tempfile *mks_tempfile_tsm(const char *filename_template, int suffixlen,\n \treturn tempfile;\n }\n\n+struct tempfile *mks_tempfile_dt(const char *directory_template,\n+\t\t\t\t const char *filename)\n+{\n+\tstruct tempfile *tempfile;\n+\tconst char *tmpdir;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint fd;\n+\tsize_t directorylen;\n+\n+\tif (!ends_with(directory_template, \"XXXXXX\")) {\n+\t\terrno = EINVAL;\n+\t\treturn NULL;\n+\t}\n+\n+\ttmpdir = getenv(\"TMPDIR\");\n+\tif (!tmpdir)\n+\t\ttmpdir = \"/tmp\";\n+\n+\tstrbuf_addf(&sb, \"%s/%s\", tmpdir, directory_template);\n+\tdirectorylen = sb.len;\n+\tif (!mkdtemp(sb.buf)) {\n+\t\tint orig_errno = errno;\n+\t\tstrbuf_release(&sb);\n+\t\terrno = orig_errno;\n+\t\treturn NULL;\n+\t}\n+\n+\tstrbuf_addf(&sb, \"/%s\", filename);\n+\tfd = open(sb.buf, O_CREAT | O_EXCL | O_RDWR, 0600);\n+\tif (fd < 0) {\n+\t\tint orig_errno = errno;\n+\t\tstrbuf_setlen(&sb, directorylen);\n+\t\trmdir(sb.buf);\n+\t\tstrbuf_release(&sb);\n+\t\terrno = orig_errno;\n+\t\treturn NULL;\n+\t}\n+\n+\ttempfile = new_tempfile();\n+\tstrbuf_swap(&tempfile->filename, &sb);\n+\ttempfile->directorylen = directorylen;\n+\ttempfile->fd = fd;\n+\tactivate_tempfile(tempfile);\n+\treturn tempfile;\n+}\n+\n struct tempfile *xmks_tempfile_m(const char *filename_template, int mode)\n {\n \tstruct tempfile *tempfile;\n@@ -316,6 +378,7 @@ void delete_tempfile(struct tempfile **tempfile_p)\n\n \tclose_tempfile_gently(tempfile);\n \tunlink_or_warn(tempfile->filename.buf);\n+\tremove_template_directory(tempfile, 0);\n \tdeactivate_tempfile(tempfile);\n \t*tempfile_p = NULL;\n }\ndiff --git a/tempfile.h b/tempfile.h\nindex 4de3bc77d2..d7804a214a 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -82,6 +82,7 @@ struct tempfile {\n \tFILE *volatile fp;\n \tvolatile pid_t owner;\n \tstruct strbuf filename;\n+\tsize_t directorylen;\n };\n\n /*\n@@ -198,6 +199,18 @@ static inline struct tempfile *xmks_tempfile(const char *filename_template)\n \treturn xmks_tempfile_m(filename_template, 0600);\n }\n\n+/*\n+ * Attempt to create a temporary directory in $TMPDIR and to create and\n+ * open a file in that new directory. Derive the directory name from the\n+ * template in the manner of mkdtemp(). Arrange for directory and file\n+ * to be deleted if the program exits before they are deleted\n+ * explicitly. On success return a tempfile whose \"filename\" member\n+ * contains the full path of the file and its \"fd\" member is open for\n+ * writing the file. On error return NULL and set errno appropriately.\n+ */\n+struct tempfile *mks_tempfile_dt(const char *directory_template,\n+\t\t\t\t const char *filename);\n+\n /*\n  * Associate a stdio stream with the temporary file (which must still\n  * be open). Return `NULL` (*without* deleting the file) on error. The\n--\n2.35.3\n"},{"id":"454001","messageId":"2ad1dd80-e79f-7304-219c-db24bb269c4d@web.de","threadId":"57769","inReplyTo":"1c07dad1-3d3d-c52b-4440-631b46520254@web.de","subject":"[PATCH 2/2] diff: use mks_tempfile_dt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-20T20:30:10Z","receivedAt":"2022-04-20T20:30:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Git uses temporary files to pass the contents of blobs to external diff\nprograms and textconv filters.  It calls mks_tempfile_ts() to create\nthem, which puts them all in the same directory.  This requires adding\na random name prefix.\n\nUse mks_tempfile_dt() instead, which allows the files to have arbitrary\nnames, each in their own separate temporary directory.  This way they\ncan have the same basename as the original blob, which looks nicer in\ngraphical diff programs.\n\nThe test in t4020 to check the prettiness of the temporary paths was\nneutered by 5476bdf0e8 (diff tests: don't ignore \"git diff\" exit code in\n\"read\" loop, 2022-03-07), which removed its grep check without replacing\nit with an equivalent test_cmp check.  Add one that only checks the\nbasename of the temporary file and nothing else.\n\nAnd make the test more robust while at it, by using test_when_finished\nto get rid of the added file even if the test fails.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n diff.c                   | 8 +-------\n t/t4020-diff-external.sh | 8 ++++----\n 2 files changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ef7159968b..e71cf75886 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4136,18 +4136,13 @@ static void prep_temp_blob(struct index_state *istate,\n \t\t\t   int mode)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf tempfile = STRBUF_INIT;\n \tchar *path_dup = xstrdup(path);\n \tconst char *base = basename(path_dup);\n \tstruct checkout_metadata meta;\n\n \tinit_checkout_metadata(&meta, NULL, NULL, oid);\n\n-\t/* Generate \"XXXXXX_basename.ext\" */\n-\tstrbuf_addstr(&tempfile, \"XXXXXX_\");\n-\tstrbuf_addstr(&tempfile, base);\n-\n-\ttemp->tempfile = mks_tempfile_ts(tempfile.buf, strlen(base) + 1);\n+\ttemp->tempfile = mks_tempfile_dt(\"git-blob-XXXXXX\", base);\n \tif (!temp->tempfile)\n \t\tdie_errno(\"unable to create temp-file\");\n \tif (convert_to_working_tree(istate, path,\n@@ -4162,7 +4157,6 @@ static void prep_temp_blob(struct index_state *istate,\n \toid_to_hex_r(temp->hex, oid);\n \txsnprintf(temp->mode, sizeof(temp->mode), \"%06o\", mode);\n \tstrbuf_release(&buf);\n-\tstrbuf_release(&tempfile);\n \tfree(path_dup);\n }\n\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 1219f8bd4c..858a5522f9 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -206,17 +206,17 @@ test_expect_success 'GIT_EXTERNAL_DIFF path counter/total' '\n '\n\n test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '\n+\ttest_when_finished \"git rm -f file.ext\" &&\n \ttouch file.ext &&\n \tgit add file.ext &&\n \techo with extension > file.ext &&\n\n \tcat >expect <<-EOF &&\n-\tfile.ext file $(git rev-parse --verify HEAD:file) 100644 file.ext $(test_oid zero) 100644\n+\tfile.ext\n \tEOF\n \tGIT_EXTERNAL_DIFF=echo git diff file.ext >out &&\n-\tcut -d\" \" -f1,3- <out >actual &&\n-\tgit update-index --force-remove file.ext &&\n-\trm file.ext\n+\tbasename $(cut -d\" \" -f2 <out) >actual &&\n+\ttest_cmp expect actual\n '\n\n echo \"#!$SHELL_PATH\" >fake-diff.sh\n--\n2.35.3\n"}]}