{"thread":{"id":"66304","subject":"[PATCH] merge-ll: Cleanup merge driver temporaries after interrupt","startedAt":"2026-09-10T15:06:30Z","lastAt":"2026-09-29T18:25:39Z","messageCount":22,"participants":["Michal Koutný","Jeff King","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"552448","messageId":"20260910150608.1867930-1-mkoutny@suse.com","threadId":"66304","inReplyTo":null,"subject":"[PATCH] merge-ll: Cleanup merge driver temporaries after interrupt","fromName":"Michal Koutný","fromEmail":"mkoutny@suse.com","sentAt":"2026-09-10T15:06:07Z","receivedAt":"2026-09-10T15:06:30Z","isPatch":true,"body":"When there's a long(er) running merge driver helper, the user may just\ndecide to terminate it with Ctrl+C. That sends a signal to the driver\nprog and to the whole process group as well, including the git merge\ncommand proper. Hence the cleanup code would not run and .merge_file_*\nfiles are left behind.\n\nTransfer the idiom [1] from editor.c where the (process group) signal\ndelivery is approximated from the return code of the child process and\ndo the cleanup before going for good.\n\n[1] Note: when the helper SIGINTs alone, it'd tear down the git-merge too.\n\nReported-by: Jean Delvare \nSigned-off-by: Michal Koutný \n---\n merge-ll.c | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex ef5287dee8..bee30fb5dd 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -17,6 +17,7 @@\n #include \"quote.h\"\n #include \"strbuf.h\"\n #include \"gettext.h\"\n+#include \"sigchain.h\"\n \n struct ll_merge_driver;\n \n@@ -201,7 +202,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tint status, fd, i;\n+\tint status, fd, i, sig;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n \tassert(opts);\n@@ -240,7 +241,13 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n-\tstatus = run_command(&child);\n+\tstatus = -1;\n+\tif (start_command(&child) < 0)\n+\t\tgoto bad;\n+\tsigchain_push(SIGINT, SIG_IGN);\n+\tsigchain_push(SIGQUIT, SIG_IGN);\n+\tstatus = finish_command(&child);\n+\n \tfd = open(temp[1], O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\n@@ -262,9 +269,15 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\tret = LL_MERGE_OK;\n \telse if (status <= 128)\n \t\tret = LL_MERGE_CONFLICT;\n-\telse\n+\telse {\n \t\t/* died due to a signal: WTERMSIG(status) + 128 */\n+\t\tsig = status - 128;\n+\t\tsigchain_pop(SIGINT);\n+\t\tsigchain_pop(SIGQUIT);\n+\t\tif (sig == SIGINT || sig == SIGQUIT)\n+\t\t\traise(sig);\n \t\tret = LL_MERGE_ERROR;\n+\t}\n \treturn ret;\n }\n \n-- \n2.55.0\n\n"},{"id":"552464","messageId":"20260910162242.GC251185@coredump.intra.peff.net","threadId":"66304","inReplyTo":"20260910150608.1867930-1-mkoutny@suse.com","subject":"Re: [PATCH] merge-ll: Cleanup merge driver temporaries after interrupt","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-10T16:22:42Z","receivedAt":"2026-09-10T16:22:44Z","isPatch":true,"body":"On Thu, Sep 10, 2026 at 05:06:07PM +0200, Michal Koutný wrote:\n\n> When there's a long(er) running merge driver helper, the user may just\n> decide to terminate it with Ctrl+C. That sends a signal to the driver\n> prog and to the whole process group as well, including the git merge\n> command proper. Hence the cleanup code would not run and .merge_file_*\n> files are left behind.\n> \n> Transfer the idiom [1] from editor.c where the (process group) signal\n> delivery is approximated from the return code of the child process and\n> do the cleanup before going for good.\n\nWe have a temporary-file cleanup handler that we install already, which\nhandles signal propagation, atomicity, etc. It seems like it would be\nsimpler to just use that.\n\nIn the worst case we can just call register_tempfile() on each path, but\nI think this code could be taught to use the actual creation. Something\nlike the patch below (only lightly tested).\n\n---\ndiff --git a/merge-ll.c b/merge-ll.c\nindex ef5287dee8..d53f0fe4a6 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -17,6 +17,7 @@\n #include \"quote.h\"\n #include \"strbuf.h\"\n #include \"gettext.h\"\n+#include \"tempfile.h\"\n \n struct ll_merge_driver;\n \n@@ -174,15 +175,30 @@ static struct ll_merge_driver ll_merge_drv[] = {\n \t{ \"union\", \"built-in union merge\", ll_union_merge },\n };\n \n-static void create_temp(mmfile_t *src, char *path, size_t len)\n+static struct tempfile *create_temp(mmfile_t *src)\n {\n-\tint fd;\n-\n-\txsnprintf(path, len, \".merge_file_XXXXXX\");\n-\tfd = xmkstemp(path);\n-\tif (write_in_full(fd, src->ptr, src->size) < 0)\n+\tstruct tempfile *t = xmks_tempfile(\".merge_file_XXXXXX\");\n+\tif (write_in_full(t->fd, src->ptr, src->size) < 0)\n \t\tdie_errno(\"unable to write temp-file\");\n-\tclose(fd);\n+\tclose(t->fd);\n+\treturn t;\n+}\n+\n+static const char *get_temp_path(struct tempfile *t)\n+{\n+\t/*\n+\t * Tempfiles store the absolute path of the file, but\n+\t * we don't do any quoting against the shell, which\n+\t * can lead to problems if your path has spaces, etc, in it.\n+\t * Historically this was OK since we only provided relative\n+\t * paths which were fairly vanilla.\n+\t *\n+\t * We can work around it by going back to the relative path (since we\n+\t * know we created a tempfile in the cwd via create_temp() above).\n+\t * In the long run I think we ought to consider providing\n+\t * the absolute paths but correctly shell-quoting them.\n+\t */\n+\treturn basename(get_tempfile_path(t));\n }\n \n /*\n@@ -197,11 +213,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n-\tchar temp[3][50];\n+\tstruct tempfile *tmp_o, *tmp_a, *tmp_b;\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tint status, fd, i;\n+\tint status, fd;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n \tassert(opts);\n@@ -211,19 +227,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tresult->ptr = NULL;\n \tresult->size = 0;\n-\tcreate_temp(orig, temp[0], sizeof(temp[0]));\n-\tcreate_temp(src1, temp[1], sizeof(temp[1]));\n-\tcreate_temp(src2, temp[2], sizeof(temp[2]));\n+\ttmp_o = create_temp(orig);\n+\ttmp_a = create_temp(src1);\n+\ttmp_b = create_temp(src2);\n \n \twhile (strbuf_expand_step(&cmd, &format)) {\n \t\tif (skip_prefix(format, \"%\", &format))\n \t\t\tstrbuf_addch(&cmd, '%');\n \t\telse if (skip_prefix(format, \"O\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[0]);\n+\t\t\tstrbuf_addstr(&cmd, get_temp_path(tmp_o));\n \t\telse if (skip_prefix(format, \"A\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[1]);\n+\t\t\tstrbuf_addstr(&cmd, get_temp_path(tmp_a));\n \t\telse if (skip_prefix(format, \"B\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[2]);\n+\t\t\tstrbuf_addstr(&cmd, get_temp_path(tmp_b));\n \t\telse if (skip_prefix(format, \"L\", &format))\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n@@ -241,7 +257,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n-\tfd = open(temp[1], O_RDONLY);\n+\t/* really feels like we could just use strbuf_read_file() here? */\n+\tfd = open(get_tempfile_path(tmp_a), O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\n \tif (fstat(fd, &st))\n@@ -255,8 +272,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n close_bad:\n \tclose(fd);\n bad:\n-\tfor (i = 0; i < 3; i++)\n-\t\tunlink_or_warn(temp[i]);\n+\tdelete_tempfile(&tmp_o);\n+\tdelete_tempfile(&tmp_a);\n+\tdelete_tempfile(&tmp_b);\n \tstrbuf_release(&cmd);\n \tif (!status)\n \t\tret = LL_MERGE_OK;\n"},{"id":"552553","messageId":"aqQN_Q6ZAeyTy7WA@localhost.localdomain","threadId":"66304","inReplyTo":"20260910162242.GC251185@coredump.intra.peff.net","subject":"Re: [PATCH] merge-ll: Cleanup merge driver temporaries after interrupt","fromName":"Michal Koutný","fromEmail":"mkoutny@suse.com","sentAt":"2026-09-11T14:43:08Z","receivedAt":"2026-09-11T14:43:14Z","isPatch":true,"body":"Hi.\n\nOn Thu, Sep 10, 2026 at 12:22:42PM -0400, Jeff King wrote:\n> We have a temporary-file cleanup handler that we install already, which\n> handles signal propagation, atomicity, etc. It seems like it would be\n> simpler to just use that.\n\nThat sounds like even a better idiom to achieve the goal.\n\n> \n> In the worst case we can just call register_tempfile() on each path, but\n> I think this code could be taught to use the actual creation. Something\n> like the patch below (only lightly tested).\n\nI've tested it and it works (cleans up both after SIGINT and regular\ntermination).\n\n(There's only a warning about constness, one should not change the\ntempfile's path buffer. But here the ovewrite happens only if there were\ntrialing dirseps, which they aren't as the filename is under control.)\n\nDo you want me to send your variant as v2 or will you?\n\nThanks,\nMichal\n"},{"id":"552571","messageId":"20260911171044.GA1609692@coredump.intra.peff.net","threadId":"66304","inReplyTo":"aqQN_Q6ZAeyTy7WA@localhost.localdomain","subject":"[PATCH v2 0/3] merge-ll: Cleanup merge driver temporaries after","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-11T17:10:44Z","receivedAt":"2026-09-11T17:10:53Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 04:43:08PM +0200, Michal Koutný wrote:\n\n> > In the worst case we can just call register_tempfile() on each path, but\n> > I think this code could be taught to use the actual creation. Something\n> > like the patch below (only lightly tested).\n> \n> I've tested it and it works (cleans up both after SIGINT and regular\n> termination).\n\nThanks for testing. I considered putting something in the test suite,\nbut it gets ugly (we'd have the external driver pause, signal a fifo,\nthen kill git-merge and it with SIGINT). I guess an alternative would be\nsetting GIT_ALLOC_LIMIT to something low, and then generating a\ntoo-large output, which would cause xmalloc() to fail, which I believe\nwould also fail. But then we're not really testing the signal handling.\n\nHmm. I wonder if leaving the files could actually be a _feature_. If you\ncompleted the merge with the external tool but we barfed reading it back\nin, would it be useful to leave the file in place? It's possible, I\nsuppose, but I think it is more likely to be a nuisance (and we already\ndelete it for things like read() errors, just not anything that would\ncause us to die()).\n\n> (There's only a warning about constness, one should not change the\n> tempfile's path buffer. But here the ovewrite happens only if there were\n> trialing dirseps, which they aren't as the filename is under control.)\n\nYeah, I've fixed it in this iteration, plus a few tweaks:\n\n - I did the strbuf cleanup I mentioned (patch 1)\n\n - we should be using close_tempfile_gently() instead of close() on the\n tempfiles so that they don't get double-closed when deleting\n\n - that made me notice a small error-checking bug in the original code,\n fixed in patch 2\n\n> Do you want me to send your variant as v2 or will you?\n\nHere it is. I've labeled it v2, and I stole your commit message for the\nthird patch.\n\n [1/3]: merge-ll: use strbuf to read back external merge result\n [2/3]: merge-ll: catch close() errors when writing external tempfiles\n [3/3]: merge-ll: use tempfile API for external driver files\n\n merge-ll.c | 68 +++++++++++++++++++++++++++++-------------------------\n 1 file changed, 37 insertions(+), 31 deletions(-)\n\n-Peff\n"},{"id":"552572","messageId":"20260911171124.GA1610200@coredump.intra.peff.net","threadId":"66304","inReplyTo":"20260911171044.GA1609692@coredump.intra.peff.net","subject":"[PATCH v2 1/3] merge-ll: use strbuf to read back external merge result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-11T17:11:24Z","receivedAt":"2026-09-11T17:11:26Z","isPatch":true,"body":"After the external merge runs, we read the file back into a heap buffer.\nThis ancient code does it by hand, but these days we can make the code\nshorter and less error prone by using strbuf_read_file().\n\nIt's not quite a one-liner replacement, because we have to copy the\npointer and size into an mmbuffer_t. Two things to note there:\n\n 1. We can't just pass result->size to strbuf_detach(), since the\n former uses long instead of size_t (something that we'd ideally fix\n in the long run, but is way out of scope here).\n\n 2. We can leave result untouched on error; we zero it at the top of\n the function (confusingly we may still return LL_MERGE_OK and a\n NULL result if we hit an I/O error, but that is how the function\n has always behaved, and callers know to check for NULL).\n\nSigned-off-by: Jeff King \n---\nNot strictly needed for the rest of the series, but it felt like a\ncleanup worth doing, and it conflicts textually.\n\n merge-ll.c | 22 +++++++---------------\n 1 file changed, 7 insertions(+), 15 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex ef5287dee8..5b6af15e23 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -201,8 +201,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tint status, fd, i;\n-\tstruct stat st;\n+\tint status, i;\n+\tstruct strbuf result_buf = STRBUF_INIT;\n \tenum ll_merge_result ret;\n \tassert(opts);\n \n@@ -241,20 +241,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n-\tfd = open(temp[1], O_RDONLY);\n-\tif (fd < 0)\n-\t\tgoto bad;\n-\tif (fstat(fd, &st))\n-\t\tgoto close_bad;\n-\tresult->size = st.st_size;\n-\tresult->ptr = xmallocz(result->size);\n-\tif (read_in_full(fd, result->ptr, result->size) != result->size) {\n-\t\tFREE_AND_NULL(result->ptr);\n-\t\tresult->size = 0;\n+\n+\tif (strbuf_read_file(&result_buf, temp[1], 0) >= 0) {\n+\t\tresult->size = result_buf.len;\n+\t\tresult->ptr = strbuf_detach(&result_buf, NULL);\n \t}\n- close_bad:\n-\tclose(fd);\n- bad:\n+\n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\n \tstrbuf_release(&cmd);\n-- \n2.56.0.rc0.314.g7a874b6915\n\n"},{"id":"552573","messageId":"20260911171139.GB1610200@coredump.intra.peff.net","threadId":"66304","inReplyTo":"20260911171044.GA1609692@coredump.intra.peff.net","subject":"[PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-11T17:11:39Z","receivedAt":"2026-09-11T17:11:41Z","isPatch":true,"body":"When writing out tempfiles for an external merge driver, we catch the\ncase that write() fails, but not the follow-up close(). This close()\nwould usually succeed, but the system could report a delayed write error\n(e.g., on a network file system).\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 5b6af15e23..5a11a9613b 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -180,9 +180,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n \n \txsnprintf(path, len, \".merge_file_XXXXXX\");\n \tfd = xmkstemp(path);\n-\tif (write_in_full(fd, src->ptr, src->size) < 0)\n+\tif (write_in_full(fd, src->ptr, src->size) < 0 ||\n+\t close(fd) < 0)\n \t\tdie_errno(\"unable to write temp-file\");\n-\tclose(fd);\n }\n \n /*\n-- \n2.56.0.rc0.314.g7a874b6915\n\n"},{"id":"552574","messageId":"20260911171339.GC1610200@coredump.intra.peff.net","threadId":"66304","inReplyTo":"20260911171044.GA1609692@coredump.intra.peff.net","subject":"[PATCH v2 3/3] merge-ll: use tempfile API for external driver files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-11T17:13:39Z","receivedAt":"2026-09-11T17:13:41Z","isPatch":true,"body":"When there's a long(er) running merge driver helper, the user may just\ndecide to terminate it with Ctrl+C. That sends a signal to the driver\nprog and to the whole process group as well, including the git merge\ncommand proper. Hence the cleanup code would not run and .merge_file_*\nfiles are left behind.\n\nWe can fix this by using the tempfile API, which auto-cleans files on\nsignal or other error. That covers the Ctrl+C case above, as well as any\nother incidental death (e.g., allocation error due to a gigantic\noutput).\n\nNote that there is one gotcha here. The current code uses short,\nrelative filenames for the tempfiles (like \".merge_file_abc123\"). But\nthe tempfile API stores and returns absolute paths. Because we run the\nmerge driver as a shell command, this can result in problems if the\nleading directories contain shell metacharacters (like our tests, which\nput a space in the trash directory name for exactly this purpose).\n\nIf we were starting from scratch, I'd say the correct solution here is\nto shell-quote the filenames we put in the command. But doing so isn't\nstrictly backwards compatible, because users might have their own shell\ncharacters. For example, if I configure a driver like this:\n\n [merge \"foo\"]\n driver = \"my-driver '%O' '%A' '%B'\"\n\nthen adding extra quoting will screw things up! Strictly speaking, this\nkind of quoting is wrong (it would fail if %A expanded to something with\na single-quote in it), but it is entirely harmless with the current\nvanilla relative paths. It doesn't seem worth breaking it.\n\nSo let's take the most conservative route, and just continue reporting\nthe relative paths.\n\nCommit-message-stolen-from: Michal Koutný \nReported-by: Jean Delvare \nSigned-off-by: Jeff King \n---\n merge-ll.c | 50 ++++++++++++++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 18 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 5a11a9613b..ec0f012b4f 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -17,6 +17,7 @@\n #include \"quote.h\"\n #include \"strbuf.h\"\n #include \"gettext.h\"\n+#include \"tempfile.h\"\n \n struct ll_merge_driver;\n \n@@ -174,15 +175,27 @@ static struct ll_merge_driver ll_merge_drv[] = {\n \t{ \"union\", \"built-in union merge\", ll_union_merge },\n };\n \n-static void create_temp(mmfile_t *src, char *path, size_t len)\n+static struct tempfile *create_temp(mmfile_t *src)\n {\n-\tint fd;\n-\n-\txsnprintf(path, len, \".merge_file_XXXXXX\");\n-\tfd = xmkstemp(path);\n-\tif (write_in_full(fd, src->ptr, src->size) < 0 ||\n-\t close(fd) < 0)\n+\tstruct tempfile *t = xmks_tempfile(\".merge_file_XXXXXX\");\n+\tif (write_in_full(t->fd, src->ptr, src->size) < 0 ||\n+\t close_tempfile_gently(t) < 0)\n \t\tdie_errno(\"unable to write temp-file\");\n+\treturn t;\n+}\n+\n+static const char *temp_path_basename(struct tempfile *t)\n+{\n+\t/*\n+\t * basename() takes a non-const pointer because it can\n+\t * modify the input string to remove trailing directory\n+\t * separators. We know that we don't have any because\n+\t * this is a clean path generated from our vanilla\n+\t * tempfile template.\n+\t *\n+\t * So casting away the const here is safe, albeit gross.\n+\t */\n+\treturn basename((char *)get_tempfile_path(t));\n }\n \n /*\n@@ -197,11 +210,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n-\tchar temp[3][50];\n+\tstruct tempfile *tmp_o, *tmp_a, *tmp_b;\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tint status, i;\n+\tint status;\n \tstruct strbuf result_buf = STRBUF_INIT;\n \tenum ll_merge_result ret;\n \tassert(opts);\n@@ -211,19 +224,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tresult->ptr = NULL;\n \tresult->size = 0;\n-\tcreate_temp(orig, temp[0], sizeof(temp[0]));\n-\tcreate_temp(src1, temp[1], sizeof(temp[1]));\n-\tcreate_temp(src2, temp[2], sizeof(temp[2]));\n+\ttmp_o = create_temp(orig);\n+\ttmp_a = create_temp(src1);\n+\ttmp_b = create_temp(src2);\n \n \twhile (strbuf_expand_step(&cmd, &format)) {\n \t\tif (skip_prefix(format, \"%\", &format))\n \t\t\tstrbuf_addch(&cmd, '%');\n \t\telse if (skip_prefix(format, \"O\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[0]);\n+\t\t\tstrbuf_addstr(&cmd, temp_path_basename(tmp_o));\n \t\telse if (skip_prefix(format, \"A\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[1]);\n+\t\t\tstrbuf_addstr(&cmd, temp_path_basename(tmp_a));\n \t\telse if (skip_prefix(format, \"B\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[2]);\n+\t\t\tstrbuf_addstr(&cmd, temp_path_basename(tmp_b));\n \t\telse if (skip_prefix(format, \"L\", &format))\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n@@ -242,13 +255,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n \n-\tif (strbuf_read_file(&result_buf, temp[1], 0) >= 0) {\n+\tif (strbuf_read_file(&result_buf, get_tempfile_path(tmp_a), 0) >= 0) {\n \t\tresult->size = result_buf.len;\n \t\tresult->ptr = strbuf_detach(&result_buf, NULL);\n \t}\n \n-\tfor (i = 0; i < 3; i++)\n-\t\tunlink_or_warn(temp[i]);\n+\tdelete_tempfile(&tmp_o);\n+\tdelete_tempfile(&tmp_a);\n+\tdelete_tempfile(&tmp_b);\n \tstrbuf_release(&cmd);\n \tif (!status)\n \t\tret = LL_MERGE_OK;\n-- \n2.56.0.rc0.314.g7a874b6915\n"},{"id":"552577","messageId":"CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com","threadId":"66304","inReplyTo":"20260911171124.GA1610200@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-09-11T18:06:33Z","receivedAt":"2026-09-11T18:06:46Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 10:11 AM Jeff King wrote:\n>\n> After the external merge runs, we read the file back into a heap buffer.\n> This ancient code does it by hand, but these days we can make the code\n> shorter and less error prone by using strbuf_read_file().\n>\n> It's not quite a one-liner replacement, because we have to copy the\n> pointer and size into an mmbuffer_t. Two things to note there:\n>\n> 1. We can't just pass result->size to strbuf_detach(), since the\n> former uses long instead of size_t (something that we'd ideally fix\n> in the long run, but is way out of scope here).\n>\n> 2. We can leave result untouched on error; we zero it at the top of\n> the function (confusingly we may still return LL_MERGE_OK and a\n> NULL result if we hit an I/O error, but that is how the function\n> has always behaved, and callers know to check for NULL).\n>\n> Signed-off-by: Jeff King \n> ---\n> Not strictly needed for the rest of the series, but it felt like a\n> cleanup worth doing, and it conflicts textually.\n>\n> merge-ll.c | 22 +++++++---------------\n> 1 file changed, 7 insertions(+), 15 deletions(-)\n>\n> diff --git a/merge-ll.c b/merge-ll.c\n> index ef5287dee8..5b6af15e23 100644\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> @@ -201,8 +201,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> struct strbuf cmd = STRBUF_INIT;\n> const char *format = fn->cmdline;\n> struct child_process child = CHILD_PROCESS_INIT;\n> - int status, fd, i;\n> - struct stat st;\n> + int status, i;\n> + struct strbuf result_buf = STRBUF_INIT;\n> enum ll_merge_result ret;\n> assert(opts);\n>\n> @@ -241,20 +241,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> child.use_shell = 1;\n> strvec_push(&child.args, cmd.buf);\n> status = run_command(&child);\n> - fd = open(temp[1], O_RDONLY);\n> - if (fd < 0)\n> - goto bad;\n> - if (fstat(fd, &st))\n> - goto close_bad;\n> - result->size = st.st_size;\n> - result->ptr = xmallocz(result->size);\n> - if (read_in_full(fd, result->ptr, result->size) != result->size) {\n> - FREE_AND_NULL(result->ptr);\n> - result->size = 0;\n> +\n> + if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) {\n> + result->size = result_buf.len;\n> + result->ptr = strbuf_detach(&result_buf, NULL);\n\nI know the type mismatch is pre-existing, but the order makes the new\nbehavior different. On LLP64, assuming the usual wraparound, a result\nof LONG_MAX + 101 narrows to the negative value LONG_MIN + 100 .\n\nThe old code narrows before xmallocz() , so it requests an impossibly\nlarge allocation and dies. The new code allocates the actual buffer\nfirst, then records a negative size; callers converting that size back\nto size_t could read past the allocation.\n\nWould a simple fail-fast make sense?\n\nif (result_buf.len > LONG_MAX)\n die(_(\"external merge result is too large\"));\n\n> }\n> - close_bad:\n> - close(fd);\n> - bad:\n> +\n> for (i = 0; i < 3; i++)\n> unlink_or_warn(temp[i]);\n> strbuf_release(&cmd);\n> --\n> 2.56.0.rc0.314.g7a874b6915\n\nOtherwise, looks nice.\n"},{"id":"552578","messageId":"CABPp-BG6wYkr4wjr-iqak9fYo4+49WvjROdZ_MK5=g27WcUmMA@mail.gmail.com","threadId":"66304","inReplyTo":"20260911171139.GB1610200@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-09-11T18:06:43Z","receivedAt":"2026-09-11T18:06:55Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 10:11 AM Jeff King wrote:\n>\n> When writing out tempfiles for an external merge driver, we catch the\n> case that write() fails, but not the follow-up close(). This close()\n> would usually succeed, but the system could report a delayed write error\n> (e.g., on a network file system).\n>\n> Signed-off-by: Jeff King \n> ---\n> merge-ll.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/merge-ll.c b/merge-ll.c\n> index 5b6af15e23..5a11a9613b 100644\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> @@ -180,9 +180,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n>\n> xsnprintf(path, len, \".merge_file_XXXXXX\");\n> fd = xmkstemp(path);\n> - if (write_in_full(fd, src->ptr, src->size) < 0)\n> + if (write_in_full(fd, src->ptr, src->size) < 0 ||\n> + close(fd) < 0)\n> die_errno(\"unable to write temp-file\");\n> - close(fd);\n> }\n>\n> /*\n> --\n> 2.56.0.rc0.314.g7a874b6915\n\nI got tripped up at first on this patch; if write_in_full() < 0, then\nwe won't explicitly close(), but since die will result in an implicit\nclose, that's not a problem.\n\nInstead, the only thing that changes is we also die if close() fails.\n\nLooks good.\n"},{"id":"552579","messageId":"CABPp-BFyKaByMYZ212O3cB2GD9OjNJNZEO+krf2GGs9vxFYPhw@mail.gmail.com","threadId":"66304","inReplyTo":"20260911171339.GC1610200@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-09-11T18:10:03Z","receivedAt":"2026-09-11T18:10:21Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 10:13 AM Jeff King wrote:\n>\n> When there's a long(er) running merge driver helper, the user may just\n> decide to terminate it with Ctrl+C. That sends a signal to the driver\n> prog and to the whole process group as well, including the git merge\n\nMinor nit:\n\nprog -> program ? or -> process ?\n\nOr maybe tweak whole sentence? : That sends a signal to the whole\nforeground process group, including both the driver and the git merge\nprocess.\n\n> command proper. Hence the cleanup code would not run and .merge_file_*\n> files are left behind.\n>\n> We can fix this by using the tempfile API, which auto-cleans files on\n> signal or other error. That covers the Ctrl+C case above, as well as any\n> other incidental death (e.g., allocation error due to a gigantic\n> output).\n>\n> Note that there is one gotcha here. The current code uses short,\n> relative filenames for the tempfiles (like \".merge_file_abc123\"). But\n> the tempfile API stores and returns absolute paths. Because we run the\n> merge driver as a shell command, this can result in problems if the\n> leading directories contain shell metacharacters (like our tests, which\n> put a space in the trash directory name for exactly this purpose).\n>\n> If we were starting from scratch, I'd say the correct solution here is\n> to shell-quote the filenames we put in the command. But doing so isn't\n> strictly backwards compatible, because users might have their own shell\n> characters. For example, if I configure a driver like this:\n>\n> [merge \"foo\"]\n> driver = \"my-driver '%O' '%A' '%B'\"\n>\n> then adding extra quoting will screw things up! Strictly speaking, this\n> kind of quoting is wrong (it would fail if %A expanded to something with\n> a single-quote in it), but it is entirely harmless with the current\n> vanilla relative paths. It doesn't seem worth breaking it.\n>\n> So let's take the most conservative route, and just continue reporting\n> the relative paths.\n>\n> Commit-message-stolen-from: Michal Koutný \n\n:-)\n\nBut maybe Commit-message-mostly-stolen-from? Much of your commit\nmessage is understandably about tempfile specifics, which the original\ndidn't have.\n\n(Yeah, probably not important enough to bother changing; I'm just\n\"thinking out loud\" as I read...)\n\n> Reported-by: Jean Delvare \n> Signed-off-by: Jeff King \n> ---\n> merge-ll.c | 50 ++++++++++++++++++++++++++++++++------------------\n> 1 file changed, 32 insertions(+), 18 deletions(-)\n>\n> diff --git a/merge-ll.c b/merge-ll.c\n> index 5a11a9613b..ec0f012b4f 100644\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> @@ -17,6 +17,7 @@\n> #include \"quote.h\"\n> #include \"strbuf.h\"\n> #include \"gettext.h\"\n> +#include \"tempfile.h\"\n>\n> struct ll_merge_driver;\n>\n> @@ -174,15 +175,27 @@ static struct ll_merge_driver ll_merge_drv[] = {\n> { \"union\", \"built-in union merge\", ll_union_merge },\n> };\n>\n> -static void create_temp(mmfile_t *src, char *path, size_t len)\n> +static struct tempfile *create_temp(mmfile_t *src)\n> {\n> - int fd;\n> -\n> - xsnprintf(path, len, \".merge_file_XXXXXX\");\n> - fd = xmkstemp(path);\n> - if (write_in_full(fd, src->ptr, src->size) < 0 ||\n> - close(fd) < 0)\n> + struct tempfile *t = xmks_tempfile(\".merge_file_XXXXXX\");\n> + if (write_in_full(t->fd, src->ptr, src->size) < 0 ||\n> + close_tempfile_gently(t) < 0)\n> die_errno(\"unable to write temp-file\");\n> + return t;\n> +}\n> +\n> +static const char *temp_path_basename(struct tempfile *t)\n> +{\n> + /*\n> + * basename() takes a non-const pointer because it can\n> + * modify the input string to remove trailing directory\n> + * separators. We know that we don't have any because\n> + * this is a clean path generated from our vanilla\n> + * tempfile template.\n> + *\n> + * So casting away the const here is safe, albeit gross.\n> + */\n> + return basename((char *)get_tempfile_path(t));\n\nThanks for the comment.\n\n> }\n>\n> /*\n> @@ -197,11 +210,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> const struct ll_merge_options *opts,\n> int marker_size)\n> {\n> - char temp[3][50];\n> + struct tempfile *tmp_o, *tmp_a, *tmp_b;\n> struct strbuf cmd = STRBUF_INIT;\n> const char *format = fn->cmdline;\n> struct child_process child = CHILD_PROCESS_INIT;\n> - int status, i;\n> + int status;\n> struct strbuf result_buf = STRBUF_INIT;\n> enum ll_merge_result ret;\n> assert(opts);\n> @@ -211,19 +224,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n>\n> result->ptr = NULL;\n> result->size = 0;\n> - create_temp(orig, temp[0], sizeof(temp[0]));\n> - create_temp(src1, temp[1], sizeof(temp[1]));\n> - create_temp(src2, temp[2], sizeof(temp[2]));\n> + tmp_o = create_temp(orig);\n> + tmp_a = create_temp(src1);\n> + tmp_b = create_temp(src2);\n>\n> while (strbuf_expand_step(&cmd, &format)) {\n> if (skip_prefix(format, \"%\", &format))\n> strbuf_addch(&cmd, '%');\n> else if (skip_prefix(format, \"O\", &format))\n> - strbuf_addstr(&cmd, temp[0]);\n> + strbuf_addstr(&cmd, temp_path_basename(tmp_o));\n> else if (skip_prefix(format, \"A\", &format))\n> - strbuf_addstr(&cmd, temp[1]);\n> + strbuf_addstr(&cmd, temp_path_basename(tmp_a));\n> else if (skip_prefix(format, \"B\", &format))\n> - strbuf_addstr(&cmd, temp[2]);\n> + strbuf_addstr(&cmd, temp_path_basename(tmp_b));\n> else if (skip_prefix(format, \"L\", &format))\n> strbuf_addf(&cmd, \"%d\", marker_size);\n> else if (skip_prefix(format, \"P\", &format))\n> @@ -242,13 +255,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> strvec_push(&child.args, cmd.buf);\n> status = run_command(&child);\n>\n> - if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) {\n> + if (strbuf_read_file(&result_buf, get_tempfile_path(tmp_a), 0) >= 0) {\n> result->size = result_buf.len;\n> result->ptr = strbuf_detach(&result_buf, NULL);\n> }\n>\n> - for (i = 0; i < 3; i++)\n> - unlink_or_warn(temp[i]);\n> + delete_tempfile(&tmp_o);\n> + delete_tempfile(&tmp_a);\n> + delete_tempfile(&tmp_b);\n> strbuf_release(&cmd);\n> if (!status)\n> ret = LL_MERGE_OK;\n> --\n> 2.56.0.rc0.314.g7a874b6915\n\nLooks good to me.\n"},{"id":"552580","messageId":"xmqq33vfbpwe.fsf@gitster.g","threadId":"66304","inReplyTo":"CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com","subject":"Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-11T18:32:01Z","receivedAt":"2026-09-11T18:32:04Z","isPatch":true,"body":"Elijah Newren writes:\n\n> The old code narrows before xmallocz() , so it requests an impossibly\n> large allocation and dies. The new code allocates the actual buffer\n> first, then records a negative size; callers converting that size back\n> to size_t could read past the allocation.\n>\n> Would a simple fail-fast make sense?\n>\n> if (result_buf.len > LONG_MAX)\n> die(_(\"external merge result is too large\"));\n\nIntereting find. That does sound sensible.\n"},{"id":"552705","messageId":"aqf0fw2igdjsXe-V@localhost.localdomain","threadId":"66304","inReplyTo":"20260911171044.GA1609692@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/3] merge-ll: Cleanup merge driver temporaries after","fromName":"Michal Koutný","fromEmail":"mkoutny@suse.com","sentAt":"2026-09-14T13:23:28Z","receivedAt":"2026-09-14T13:23:32Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 01:10:44PM -0400, Jeff King wrote:\n> On Fri, Sep 11, 2026 at 04:43:08PM +0200, Michal Koutný wrote:\n> \n> > > In the worst case we can just call register_tempfile() on each path, but\n> > > I think this code could be taught to use the actual creation. Something\n> > > like the patch below (only lightly tested).\n> > \n> > I've tested it and it works (cleans up both after SIGINT and regular\n> > termination).\n> \n> Thanks for testing. I considered putting something in the test suite,\n> but it gets ugly (we'd have the external driver pause, signal a fifo,\n> then kill git-merge and it with SIGINT). I guess an alternative would be\n> setting GIT_ALLOC_LIMIT to something low, and then generating a\n> too-large output, which would cause xmalloc() to fail, which I believe\n> would also fail. But then we're not really testing the signal handling.\n \n> Hmm. I wonder if leaving the files could actually be a _feature_. If you\n> completed the merge with the external tool but we barfed reading it back\n> in, would it be useful to leave the file in place? It's possible, I\n> suppose, but I think it is more likely to be a nuisance (and we already\n> delete it for things like read() errors, just not anything that would\n> cause us to die()).\n\nFrom the user perspective, this is unnecessary. (Potentially useful for\ndebugging the merge tool.)\nFor the former, the whole merge can retried (after restoring state), the\nlatter is quite rare and can be worked around easily when the merge tool is\nunder development.\n\n0.02€,\nMichal\n"},{"id":"552706","messageId":"aqf1Xzug5jbNDWlV@localhost.localdomain","threadId":"66304","inReplyTo":"CABPp-BFyKaByMYZ212O3cB2GD9OjNJNZEO+krf2GGs9vxFYPhw@mail.gmail.com","subject":"Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files","fromName":"Michal Koutný","fromEmail":"mkoutny@suse.com","sentAt":"2026-09-14T13:24:33Z","receivedAt":"2026-09-14T13:24:37Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 11:10:03AM -0700, Elijah Newren wrote:\n> But maybe Commit-message-mostly-stolen-from? Much of your commit\n> message is understandably about tempfile specifics, which the original\n> didn't have.\n\nIt's also OK, if you just add me to the Reported-by: chain ;-)\n\nMichal\n"},{"id":"552718","messageId":"20260914165350.GA32247@peff.net","threadId":"66304","inReplyTo":"CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com","subject":"Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-14T16:53:50Z","receivedAt":"2026-09-14T16:53:58Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 11:06:33AM -0700, Elijah Newren wrote:\n\n> > - result->size = st.st_size;\n> > - result->ptr = xmallocz(result->size);\n> > - if (read_in_full(fd, result->ptr, result->size) != result->size) {\n> > - FREE_AND_NULL(result->ptr);\n> > - result->size = 0;\n> > +\n> > + if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) {\n> > + result->size = result_buf.len;\n> > + result->ptr = strbuf_detach(&result_buf, NULL);\n> \n> I know the type mismatch is pre-existing, but the order makes the new\n> behavior different. On LLP64, assuming the usual wraparound, a result\n> of LONG_MAX + 101 narrows to the negative value LONG_MIN + 100 .\n> \n> The old code narrows before xmallocz() , so it requests an impossibly\n> large allocation and dies. The new code allocates the actual buffer\n> first, then records a negative size; callers converting that size back\n> to size_t could read past the allocation.\n\nHmm, yeah. I noticed the possible truncation, but reasoned that it was\nroughly the same before and after (the only difference being that we\nknow would actually have the full buffer, just a truncated size). But\nyou're right that negative values introduce their own distinct type of\nconfusion.\n\n> Would a simple fail-fast make sense?\n> \n> if (result_buf.len > LONG_MAX)\n> die(_(\"external merge result is too large\"));\n\nYeah. I think we should be doing that even with the current code, as\nit's possible for us to silently truncate a merge result (e.g., wrapping\nbeyond 4GB goes back to 0).\n\nThere's a similar case in read_mmfile(). There we actually bother to use\nxsize_t() to catch _some_ problems, but of course we are using \"long\"\nand not \"size_t\" in the mmfile, so it's still subject to truncation.\n\nWe can't just use read_mmfile() here, because there is an artificial\ndistinction between mmfile_t and mmbuffer_t, even though they hold the\nexact same members (IIRC, one is for \"output\"). But possibly we can use\nit and just assign the members, which is no worse than what we have to\ndo with the strbuf.\n\nI'll plan to add a check like the one above here and in read_mmfile(),\nand then look at re-working this cleanup to use that function. I'll\nprobably also peel this off of the other patches. It's really two\nseparate topics: this file-read cleanup, and the tempfile-deletion\nimprovement that started the thread.\n\n-Peff\n"},{"id":"552719","messageId":"20260914165654.GB32247@peff.net","threadId":"66304","inReplyTo":"CABPp-BG6wYkr4wjr-iqak9fYo4+49WvjROdZ_MK5=g27WcUmMA@mail.gmail.com","subject":"Re: [PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-14T16:56:54Z","receivedAt":"2026-09-14T16:56:56Z","isPatch":true,"body":"On Fri, Sep 11, 2026 at 11:06:43AM -0700, Elijah Newren wrote:\n\n> > - if (write_in_full(fd, src->ptr, src->size) < 0)\n> > + if (write_in_full(fd, src->ptr, src->size) < 0 ||\n> > + close(fd) < 0)\n> > die_errno(\"unable to write temp-file\");\n> > - close(fd);\n> > }\n> \n> I got tripped up at first on this patch; if write_in_full() < 0, then\n> we won't explicitly close(), but since die will result in an implicit\n> close, that's not a problem.\n> \n> Instead, the only thing that changes is we also die if close() fails.\n\nYeah, this is a subtle mistake that we've had to fix before. Doing:\n\n if (write_in_full(fd, ...) || close(fd))\n\treturn error(...);\n\nis a hard-to-spot leak. It's not present here because we're calling\ndie() instead of returning, but maybe it is worth writing it out to set\na good example, like:\n\n if (write_in_full(...))\n\tdie_errno(\"unable to write\");\n if (close(...))\n\tdie_errno(\"unable to close\");\n\nSince I'm re-rolling anyway.\n\n-Peff\n"},{"id":"552720","messageId":"20260914165741.GC32247@peff.net","threadId":"66304","inReplyTo":"aqf1Xzug5jbNDWlV@localhost.localdomain","subject":"Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-14T16:57:41Z","receivedAt":"2026-09-14T16:57:42Z","isPatch":true,"body":"On Mon, Sep 14, 2026 at 03:24:33PM +0200, Michal Koutný wrote:\n\n> On Fri, Sep 11, 2026 at 11:10:03AM -0700, Elijah Newren wrote:\n> > But maybe Commit-message-mostly-stolen-from? Much of your commit\n> > message is understandably about tempfile specifics, which the original\n> > didn't have.\n> \n> It's also OK, if you just add me to the Reported-by: chain ;-)\n\nThanks, I wanted to make sure I credited you but wasn't sure how. I'll\njust do that in the re-roll. :)\n\n-Peff\n"},{"id":"552721","messageId":"20260914165903.GD32247@peff.net","threadId":"66304","inReplyTo":"aqf0fw2igdjsXe-V@localhost.localdomain","subject":"Re: [PATCH v2 0/3] merge-ll: Cleanup merge driver temporaries after","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-14T16:59:03Z","receivedAt":"2026-09-14T16:59:04Z","isPatch":true,"body":"On Mon, Sep 14, 2026 at 03:23:28PM +0200, Michal Koutný wrote:\n\n> > Hmm. I wonder if leaving the files could actually be a _feature_. If you\n> > completed the merge with the external tool but we barfed reading it back\n> > in, would it be useful to leave the file in place? It's possible, I\n> > suppose, but I think it is more likely to be a nuisance (and we already\n> > delete it for things like read() errors, just not anything that would\n> > cause us to die()).\n> \n> From the user perspective, this is unnecessary. (Potentially useful for\n> debugging the merge tool.)\n> For the former, the whole merge can retried (after restoring state), the\n> latter is quite rare and can be worked around easily when the merge tool is\n> under development.\n\nI was more wondering if a user would be frustrated that they spent 30\nminutes doing a really complicated merge in the tool, and then that\noutput was lost. I'd guess it's pretty rare, though.\n\n-Peff\n"},{"id":"553538","messageId":"20260929051200.GA1100000@coredump.intra.peff.net","threadId":"66304","inReplyTo":"20260911171044.GA1609692@coredump.intra.peff.net","subject":"[PATCH v3 0/2] merge-ll: Cleanup merge driver temporaries after signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T05:12:00Z","receivedAt":"2026-09-29T05:12:08Z","isPatch":true,"body":"Here's a revised version of the series to switch merge-ll to use\ntempfile structs. Sorry, I got derailed a bit by travel.\n\nI dropped the v2 cleanup patch to use strbuf_read() for now. It was not\nstrictly related and I think there's a bit of a rabbit hole that extends\neven beyond this function. That might become its own series later.\n\nBeyond that, this is mostly the same as v2. I tweaked the error-checking\nfor close() in the first patch so that it's more obviously correct (and\ncan produce a slightly more informative message).\n\nThe range diff is below, though it's IMHO not very informative. The\ndrop of the cleanup patch a lot of uninteresting textual ripples.\n\n [1/2]: merge-ll: catch close() errors when writing external tempfiles\n [2/2]: merge-ll: use tempfile API for external driver files\n\n merge-ll.c | 51 +++++++++++++++++++++++++++++++++------------------\n 1 file changed, 33 insertions(+), 18 deletions(-)\n\n1: 62b4ac5ae0 < -: ---------- merge-ll: use strbuf to read back external merge result\n2: 020e3bfcbd < -: ---------- merge-ll: catch close() errors when writing external tempfiles\n-: ---------- > 1: c6a4b3146d merge-ll: catch close() errors when writing external tempfiles\n3: 914fafcd88 ! 2: b85e169cb3 merge-ll: use tempfile API for external driver files\n @@ Commit message\n \n When there's a long(er) running merge driver helper, the user may just\n decide to terminate it with Ctrl+C. That sends a signal to the driver\n - prog and to the whole process group as well, including the git merge\n + program and to the whole process group as well, including the git merge\n command proper. Hence the cleanup code would not run and .merge_file_*\n files are left behind.\n \n @@ Commit message\n So let's take the most conservative route, and just continue reporting\n the relative paths.\n \n - Commit-message-stolen-from: Michal Koutný \n Reported-by: Jean Delvare \n + Reported-by: Michal Koutný \n Signed-off-by: Jeff King \n \n ## merge-ll.c ##\n @@ merge-ll.c: static struct ll_merge_driver ll_merge_drv[] = {\n -\n -\txsnprintf(path, len, \".merge_file_XXXXXX\");\n -\tfd = xmkstemp(path);\n --\tif (write_in_full(fd, src->ptr, src->size) < 0 ||\n --\t close(fd) < 0)\n +-\tif (write_in_full(fd, src->ptr, src->size) < 0)\n +-\t\tdie_errno(_(\"unable to write %s\"), path);\n +-\tif (close(fd) < 0)\n +-\t\tdie_errno(_(\"unable to close %s\"), path);\n +\tstruct tempfile *t = xmks_tempfile(\".merge_file_XXXXXX\");\n -+\tif (write_in_full(t->fd, src->ptr, src->size) < 0 ||\n -+\t close_tempfile_gently(t) < 0)\n - \t\tdie_errno(\"unable to write temp-file\");\n ++\tif (write_in_full(t->fd, src->ptr, src->size) < 0)\n ++\t\tdie_errno(_(\"unable to write %s\"), get_tempfile_path(t));\n ++\tif (close_tempfile_gently(t) < 0)\n ++\t\tdie_errno(_(\"unable to close %s\"), get_tempfile_path(t));\n +\treturn t;\n +}\n +\n @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n --\tint status, i;\n -+\tint status;\n - \tstruct strbuf result_buf = STRBUF_INIT;\n +-\tint status, fd, i;\n ++\tint status, fd;\n + \tstruct stat st;\n \tenum ll_merge_result ret;\n \tassert(opts);\n @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n + \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n - \n --\tif (strbuf_read_file(&result_buf, temp[1], 0) >= 0) {\n -+\tif (strbuf_read_file(&result_buf, get_tempfile_path(tmp_a), 0) >= 0) {\n - \t\tresult->size = result_buf.len;\n - \t\tresult->ptr = strbuf_detach(&result_buf, NULL);\n - \t}\n - \n +-\tfd = open(temp[1], O_RDONLY);\n ++\tfd = open(get_tempfile_path(tmp_a), O_RDONLY);\n + \tif (fd < 0)\n + \t\tgoto bad;\n + \tif (fstat(fd, &st))\n +@@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n + close_bad:\n + \tclose(fd);\n + bad:\n -\tfor (i = 0; i < 3; i++)\n -\t\tunlink_or_warn(temp[i]);\n +\tdelete_tempfile(&tmp_o);\n"},{"id":"553539","messageId":"20260929051254.GA1100669@coredump.intra.peff.net","threadId":"66304","inReplyTo":"20260929051200.GA1100000@coredump.intra.peff.net","subject":"[PATCH v3 1/2] merge-ll: catch close() errors when writing external tempfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T05:12:54Z","receivedAt":"2026-09-29T05:12:56Z","isPatch":true,"body":"When writing out tempfiles for an external merge driver, we catch the\ncase that write() fails, but not the follow-up close(). This close()\nwould usually succeed, but the system could report a delayed write error\n(e.g., on a network file system).\n\nSince we're adding a new error message here, we'll also make the\nexisting one match it: mark it for translation and mention the actual\npath. The exact wording here was picked to match some existing\ntranslated messages.\n\nSigned-off-by: Jeff King \n---\nSince v2, this is hopefully written in a more obviously-correct way,\nrather than the short-circuit OR.\n\n merge-ll.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex ef5287dee8..62d402199d 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -181,8 +181,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n \txsnprintf(path, len, \".merge_file_XXXXXX\");\n \tfd = xmkstemp(path);\n \tif (write_in_full(fd, src->ptr, src->size) < 0)\n-\t\tdie_errno(\"unable to write temp-file\");\n-\tclose(fd);\n+\t\tdie_errno(_(\"unable to write %s\"), path);\n+\tif (close(fd) < 0)\n+\t\tdie_errno(_(\"unable to close %s\"), path);\n }\n \n /*\n-- \n2.56.0.rc2.338.gcaacf6bdf7\n\n"},{"id":"553540","messageId":"20260929051312.GB1100669@coredump.intra.peff.net","threadId":"66304","inReplyTo":"20260929051200.GA1100000@coredump.intra.peff.net","subject":"[PATCH v3 2/2] merge-ll: use tempfile API for external driver files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T05:13:12Z","receivedAt":"2026-09-29T05:13:14Z","isPatch":true,"body":"When there's a long(er) running merge driver helper, the user may just\ndecide to terminate it with Ctrl+C. That sends a signal to the driver\nprogram and to the whole process group as well, including the git merge\ncommand proper. Hence the cleanup code would not run and .merge_file_*\nfiles are left behind.\n\nWe can fix this by using the tempfile API, which auto-cleans files on\nsignal or other error. That covers the Ctrl+C case above, as well as any\nother incidental death (e.g., allocation error due to a gigantic\noutput).\n\nNote that there is one gotcha here. The current code uses short,\nrelative filenames for the tempfiles (like \".merge_file_abc123\"). But\nthe tempfile API stores and returns absolute paths. Because we run the\nmerge driver as a shell command, this can result in problems if the\nleading directories contain shell metacharacters (like our tests, which\nput a space in the trash directory name for exactly this purpose).\n\nIf we were starting from scratch, I'd say the correct solution here is\nto shell-quote the filenames we put in the command. But doing so isn't\nstrictly backwards compatible, because users might have their own shell\ncharacters. For example, if I configure a driver like this:\n\n [merge \"foo\"]\n driver = \"my-driver '%O' '%A' '%B'\"\n\nthen adding extra quoting will screw things up! Strictly speaking, this\nkind of quoting is wrong (it would fail if %A expanded to something with\na single-quote in it), but it is entirely harmless with the current\nvanilla relative paths. It doesn't seem worth breaking it.\n\nSo let's take the most conservative route, and just continue reporting\nthe relative paths.\n\nReported-by: Jean Delvare \nReported-by: Michal Koutný \nSigned-off-by: Jeff King \n---\n merge-ll.c | 54 ++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 34 insertions(+), 20 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 62d402199d..0eadbfba23 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -17,6 +17,7 @@\n #include \"quote.h\"\n #include \"strbuf.h\"\n #include \"gettext.h\"\n+#include \"tempfile.h\"\n \n struct ll_merge_driver;\n \n@@ -174,16 +175,28 @@ static struct ll_merge_driver ll_merge_drv[] = {\n \t{ \"union\", \"built-in union merge\", ll_union_merge },\n };\n \n-static void create_temp(mmfile_t *src, char *path, size_t len)\n+static struct tempfile *create_temp(mmfile_t *src)\n {\n-\tint fd;\n-\n-\txsnprintf(path, len, \".merge_file_XXXXXX\");\n-\tfd = xmkstemp(path);\n-\tif (write_in_full(fd, src->ptr, src->size) < 0)\n-\t\tdie_errno(_(\"unable to write %s\"), path);\n-\tif (close(fd) < 0)\n-\t\tdie_errno(_(\"unable to close %s\"), path);\n+\tstruct tempfile *t = xmks_tempfile(\".merge_file_XXXXXX\");\n+\tif (write_in_full(t->fd, src->ptr, src->size) < 0)\n+\t\tdie_errno(_(\"unable to write %s\"), get_tempfile_path(t));\n+\tif (close_tempfile_gently(t) < 0)\n+\t\tdie_errno(_(\"unable to close %s\"), get_tempfile_path(t));\n+\treturn t;\n+}\n+\n+static const char *temp_path_basename(struct tempfile *t)\n+{\n+\t/*\n+\t * basename() takes a non-const pointer because it can\n+\t * modify the input string to remove trailing directory\n+\t * separators. We know that we don't have any because\n+\t * this is a clean path generated from our vanilla\n+\t * tempfile template.\n+\t *\n+\t * So casting away the const here is safe, albeit gross.\n+\t */\n+\treturn basename((char *)get_tempfile_path(t));\n }\n \n /*\n@@ -198,11 +211,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n-\tchar temp[3][50];\n+\tstruct tempfile *tmp_o, *tmp_a, *tmp_b;\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tint status, fd, i;\n+\tint status, fd;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n \tassert(opts);\n@@ -212,19 +225,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tresult->ptr = NULL;\n \tresult->size = 0;\n-\tcreate_temp(orig, temp[0], sizeof(temp[0]));\n-\tcreate_temp(src1, temp[1], sizeof(temp[1]));\n-\tcreate_temp(src2, temp[2], sizeof(temp[2]));\n+\ttmp_o = create_temp(orig);\n+\ttmp_a = create_temp(src1);\n+\ttmp_b = create_temp(src2);\n \n \twhile (strbuf_expand_step(&cmd, &format)) {\n \t\tif (skip_prefix(format, \"%\", &format))\n \t\t\tstrbuf_addch(&cmd, '%');\n \t\telse if (skip_prefix(format, \"O\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[0]);\n+\t\t\tstrbuf_addstr(&cmd, temp_path_basename(tmp_o));\n \t\telse if (skip_prefix(format, \"A\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[1]);\n+\t\t\tstrbuf_addstr(&cmd, temp_path_basename(tmp_a));\n \t\telse if (skip_prefix(format, \"B\", &format))\n-\t\t\tstrbuf_addstr(&cmd, temp[2]);\n+\t\t\tstrbuf_addstr(&cmd, temp_path_basename(tmp_b));\n \t\telse if (skip_prefix(format, \"L\", &format))\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n@@ -242,7 +255,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n-\tfd = open(temp[1], O_RDONLY);\n+\tfd = open(get_tempfile_path(tmp_a), O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\n \tif (fstat(fd, &st))\n@@ -256,8 +269,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n close_bad:\n \tclose(fd);\n bad:\n-\tfor (i = 0; i < 3; i++)\n-\t\tunlink_or_warn(temp[i]);\n+\tdelete_tempfile(&tmp_o);\n+\tdelete_tempfile(&tmp_a);\n+\tdelete_tempfile(&tmp_b);\n \tstrbuf_release(&cmd);\n \tif (!status)\n \t\tret = LL_MERGE_OK;\n-- \n2.56.0.rc2.338.gcaacf6bdf7\n"},{"id":"553616","messageId":"xmqqcxtwhufq.fsf@gitster.g","threadId":"66304","inReplyTo":"20260929051312.GB1100669@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] merge-ll: use tempfile API for external driver files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-29T16:51:53Z","receivedAt":"2026-09-29T16:51:57Z","isPatch":true,"body":"Jeff King writes:\n\n> When there's a long(er) running merge driver helper, the user may just\n> decide to terminate it with Ctrl+C. That sends a signal to the driver\n> program and to the whole process group as well, including the git merge\n> command proper. Hence the cleanup code would not run and .merge_file_*\n> files are left behind.\n>\n> We can fix this by using the tempfile API, which auto-cleans files on\n> signal or other error. That covers the Ctrl+C case above, as well as any\n> other incidental death (e.g., allocation error due to a gigantic\n> output).\n>\n> Note that there is one gotcha here. The current code uses short,\n> relative filenames for the tempfiles (like \".merge_file_abc123\"). But\n> the tempfile API stores and returns absolute paths. Because we run the\n> merge driver as a shell command, this can result in problems if the\n> leading directories contain shell metacharacters (like our tests, which\n> put a space in the trash directory name for exactly this purpose).\n>\n> If we were starting from scratch, I'd say the correct solution here is\n> to shell-quote the filenames we put in the command. But doing so isn't\n> strictly backwards compatible, because users might have their own shell\n> characters. For example, if I configure a driver like this:\n\n\"own shell characters\" -> \"own shell quoting\"?\n\n>\n> [merge \"foo\"]\n> driver = \"my-driver '%O' '%A' '%B'\"\n>\n> then adding extra quoting will screw things up! Strictly speaking, this\n> kind of quoting is wrong (it would fail if %A expanded to something with\n> a single-quote in it), but it is entirely harmless with the current\n> vanilla relative paths. It doesn't seem worth breaking it.\n>\n> So let's take the most conservative route, and just continue reporting\n> the relative paths.\n\nVery well reasoned, and the implementation exactly matches the\ndesigned behaviour.\n\nWill replace. Let's mark it for 'next' (unless somebody notices\nwhat I overlooked, which is not a very high bar to cross).\n\nThanks.\n"},{"id":"553623","messageId":"20260929182537.GA1710046@coredump.intra.peff.net","threadId":"66304","inReplyTo":"xmqqcxtwhufq.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] merge-ll: use tempfile API for external driver files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T18:25:37Z","receivedAt":"2026-09-29T18:25:39Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 09:51:53AM -0700, Junio C Hamano wrote:\n\n> > If we were starting from scratch, I'd say the correct solution here is\n> > to shell-quote the filenames we put in the command. But doing so isn't\n> > strictly backwards compatible, because users might have their own shell\n> > characters. For example, if I configure a driver like this:\n> \n> \"own shell characters\" -> \"own shell quoting\"?\n\nHmm, yeah. I was thinking that our quoting could disrupt other shell\nmetacharacters they used. But I guess if it is only surrounding the\nfilenames we provide, only their quoting characters could matter. So if\nthey wrote:\n\n --option='%A'\n '--option=%A'\n --option=\"%A\"\n\nand so forth.\n\n-Peff\n"}]}