# [PATCH] merge-ll: Cleanup merge driver temporaries after interrupt 22 messages from 2026-09-10 to 2026-09-29. Participants: Michal Koutný, Jeff King, Elijah Newren, Junio C Hamano. Thread: https://gitlist.dev/t/66304 ## Michal Koutný, 2026-09-10 15:06 Subject: [PATCH] merge-ll: Cleanup merge driver temporaries after interrupt Message-ID: <20260910150608.1867930-1-mkoutny@suse.com> ``` When there's a long(er) running merge driver helper, the user may just decide to terminate it with Ctrl+C. That sends a signal to the driver prog and to the whole process group as well, including the git merge command proper. Hence the cleanup code would not run and .merge_file_* files are left behind. Transfer the idiom [1] from editor.c where the (process group) signal delivery is approximated from the return code of the child process and do the cleanup before going for good. [1] Note: when the helper SIGINTs alone, it'd tear down the git-merge too. Reported-by: Jean Delvare Signed-off-by: Michal Koutný --- merge-ll.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index ef5287dee8..bee30fb5dd 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -17,6 +17,7 @@ #include "quote.h" #include "strbuf.h" #include "gettext.h" +#include "sigchain.h" struct ll_merge_driver; @@ -201,7 +202,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, struct strbuf cmd = STRBUF_INIT; const char *format = fn->cmdline; struct child_process child = CHILD_PROCESS_INIT; - int status, fd, i; + int status, fd, i, sig; struct stat st; enum ll_merge_result ret; assert(opts); @@ -240,7 +241,13 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, child.use_shell = 1; strvec_push(&child.args, cmd.buf); - status = run_command(&child); + status = -1; + if (start_command(&child) < 0) + goto bad; + sigchain_push(SIGINT, SIG_IGN); + sigchain_push(SIGQUIT, SIG_IGN); + status = finish_command(&child); + fd = open(temp[1], O_RDONLY); if (fd < 0) goto bad; @@ -262,9 +269,15 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, ret = LL_MERGE_OK; else if (status <= 128) ret = LL_MERGE_CONFLICT; - else + else { /* died due to a signal: WTERMSIG(status) + 128 */ + sig = status - 128; + sigchain_pop(SIGINT); + sigchain_pop(SIGQUIT); + if (sig == SIGINT || sig == SIGQUIT) + raise(sig); ret = LL_MERGE_ERROR; + } return ret; } -- 2.55.0 ``` ## Jeff King, 2026-09-10 16:22 Subject: Re: [PATCH] merge-ll: Cleanup merge driver temporaries after interrupt Message-ID: <20260910162242.GC251185@coredump.intra.peff.net> In-Reply-To: <20260910150608.1867930-1-mkoutny@suse.com> ``` On Thu, Sep 10, 2026 at 05:06:07PM +0200, Michal Koutný wrote: > When there's a long(er) running merge driver helper, the user may just > decide to terminate it with Ctrl+C. That sends a signal to the driver > prog and to the whole process group as well, including the git merge > command proper. Hence the cleanup code would not run and .merge_file_* > files are left behind. > > Transfer the idiom [1] from editor.c where the (process group) signal > delivery is approximated from the return code of the child process and > do the cleanup before going for good. We have a temporary-file cleanup handler that we install already, which handles signal propagation, atomicity, etc. It seems like it would be simpler to just use that. In the worst case we can just call register_tempfile() on each path, but I think this code could be taught to use the actual creation. Something like the patch below (only lightly tested). --- diff --git a/merge-ll.c b/merge-ll.c index ef5287dee8..d53f0fe4a6 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -17,6 +17,7 @@ #include "quote.h" #include "strbuf.h" #include "gettext.h" +#include "tempfile.h" struct ll_merge_driver; @@ -174,15 +175,30 @@ static struct ll_merge_driver ll_merge_drv[] = { { "union", "built-in union merge", ll_union_merge }, }; -static void create_temp(mmfile_t *src, char *path, size_t len) +static struct tempfile *create_temp(mmfile_t *src) { - int fd; - - xsnprintf(path, len, ".merge_file_XXXXXX"); - fd = xmkstemp(path); - if (write_in_full(fd, src->ptr, src->size) < 0) + struct tempfile *t = xmks_tempfile(".merge_file_XXXXXX"); + if (write_in_full(t->fd, src->ptr, src->size) < 0) die_errno("unable to write temp-file"); - close(fd); + close(t->fd); + return t; +} + +static const char *get_temp_path(struct tempfile *t) +{ + /* + * Tempfiles store the absolute path of the file, but + * we don't do any quoting against the shell, which + * can lead to problems if your path has spaces, etc, in it. + * Historically this was OK since we only provided relative + * paths which were fairly vanilla. + * + * We can work around it by going back to the relative path (since we + * know we created a tempfile in the cwd via create_temp() above). + * In the long run I think we ought to consider providing + * the absolute paths but correctly shell-quoting them. + */ + return basename(get_tempfile_path(t)); } /* @@ -197,11 +213,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, const struct ll_merge_options *opts, int marker_size) { - char temp[3][50]; + struct tempfile *tmp_o, *tmp_a, *tmp_b; struct strbuf cmd = STRBUF_INIT; const char *format = fn->cmdline; struct child_process child = CHILD_PROCESS_INIT; - int status, fd, i; + int status, fd; struct stat st; enum ll_merge_result ret; assert(opts); @@ -211,19 +227,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, result->ptr = NULL; result->size = 0; - create_temp(orig, temp[0], sizeof(temp[0])); - create_temp(src1, temp[1], sizeof(temp[1])); - create_temp(src2, temp[2], sizeof(temp[2])); + tmp_o = create_temp(orig); + tmp_a = create_temp(src1); + tmp_b = create_temp(src2); while (strbuf_expand_step(&cmd, &format)) { if (skip_prefix(format, "%", &format)) strbuf_addch(&cmd, '%'); else if (skip_prefix(format, "O", &format)) - strbuf_addstr(&cmd, temp[0]); + strbuf_addstr(&cmd, get_temp_path(tmp_o)); else if (skip_prefix(format, "A", &format)) - strbuf_addstr(&cmd, temp[1]); + strbuf_addstr(&cmd, get_temp_path(tmp_a)); else if (skip_prefix(format, "B", &format)) - strbuf_addstr(&cmd, temp[2]); + strbuf_addstr(&cmd, get_temp_path(tmp_b)); else if (skip_prefix(format, "L", &format)) strbuf_addf(&cmd, "%d", marker_size); else if (skip_prefix(format, "P", &format)) @@ -241,7 +257,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, child.use_shell = 1; strvec_push(&child.args, cmd.buf); status = run_command(&child); - fd = open(temp[1], O_RDONLY); + /* really feels like we could just use strbuf_read_file() here? */ + fd = open(get_tempfile_path(tmp_a), O_RDONLY); if (fd < 0) goto bad; if (fstat(fd, &st)) @@ -255,8 +272,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, close_bad: close(fd); bad: - for (i = 0; i < 3; i++) - unlink_or_warn(temp[i]); + delete_tempfile(&tmp_o); + delete_tempfile(&tmp_a); + delete_tempfile(&tmp_b); strbuf_release(&cmd); if (!status) ret = LL_MERGE_OK; ``` ## Michal Koutný, 2026-09-11 14:43 Subject: Re: [PATCH] merge-ll: Cleanup merge driver temporaries after interrupt Message-ID: In-Reply-To: <20260910162242.GC251185@coredump.intra.peff.net> ``` Hi. On Thu, Sep 10, 2026 at 12:22:42PM -0400, Jeff King wrote: > We have a temporary-file cleanup handler that we install already, which > handles signal propagation, atomicity, etc. It seems like it would be > simpler to just use that. That sounds like even a better idiom to achieve the goal. > > In the worst case we can just call register_tempfile() on each path, but > I think this code could be taught to use the actual creation. Something > like the patch below (only lightly tested). I've tested it and it works (cleans up both after SIGINT and regular termination). (There's only a warning about constness, one should not change the tempfile's path buffer. But here the ovewrite happens only if there were trialing dirseps, which they aren't as the filename is under control.) Do you want me to send your variant as v2 or will you? Thanks, Michal ``` ## Jeff King, 2026-09-11 17:10 Subject: [PATCH v2 0/3] merge-ll: Cleanup merge driver temporaries after Message-ID: <20260911171044.GA1609692@coredump.intra.peff.net> In-Reply-To: ``` On Fri, Sep 11, 2026 at 04:43:08PM +0200, Michal Koutný wrote: > > In the worst case we can just call register_tempfile() on each path, but > > I think this code could be taught to use the actual creation. Something > > like the patch below (only lightly tested). > > I've tested it and it works (cleans up both after SIGINT and regular > termination). Thanks for testing. I considered putting something in the test suite, but it gets ugly (we'd have the external driver pause, signal a fifo, then kill git-merge and it with SIGINT). I guess an alternative would be setting GIT_ALLOC_LIMIT to something low, and then generating a too-large output, which would cause xmalloc() to fail, which I believe would also fail. But then we're not really testing the signal handling. Hmm. I wonder if leaving the files could actually be a _feature_. If you completed the merge with the external tool but we barfed reading it back in, would it be useful to leave the file in place? It's possible, I suppose, but I think it is more likely to be a nuisance (and we already delete it for things like read() errors, just not anything that would cause us to die()). > (There's only a warning about constness, one should not change the > tempfile's path buffer. But here the ovewrite happens only if there were > trialing dirseps, which they aren't as the filename is under control.) Yeah, I've fixed it in this iteration, plus a few tweaks: - I did the strbuf cleanup I mentioned (patch 1) - we should be using close_tempfile_gently() instead of close() on the tempfiles so that they don't get double-closed when deleting - that made me notice a small error-checking bug in the original code, fixed in patch 2 > Do you want me to send your variant as v2 or will you? Here it is. I've labeled it v2, and I stole your commit message for the third patch. [1/3]: merge-ll: use strbuf to read back external merge result [2/3]: merge-ll: catch close() errors when writing external tempfiles [3/3]: merge-ll: use tempfile API for external driver files merge-ll.c | 68 +++++++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 31 deletions(-) -Peff ``` ## Jeff King, 2026-09-11 17:11 Subject: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result Message-ID: <20260911171124.GA1610200@coredump.intra.peff.net> In-Reply-To: <20260911171044.GA1609692@coredump.intra.peff.net> ``` After the external merge runs, we read the file back into a heap buffer. This ancient code does it by hand, but these days we can make the code shorter and less error prone by using strbuf_read_file(). It's not quite a one-liner replacement, because we have to copy the pointer and size into an mmbuffer_t. Two things to note there: 1. We can't just pass result->size to strbuf_detach(), since the former uses long instead of size_t (something that we'd ideally fix in the long run, but is way out of scope here). 2. We can leave result untouched on error; we zero it at the top of the function (confusingly we may still return LL_MERGE_OK and a NULL result if we hit an I/O error, but that is how the function has always behaved, and callers know to check for NULL). Signed-off-by: Jeff King --- Not strictly needed for the rest of the series, but it felt like a cleanup worth doing, and it conflicts textually. merge-ll.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index ef5287dee8..5b6af15e23 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -201,8 +201,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, struct strbuf cmd = STRBUF_INIT; const char *format = fn->cmdline; struct child_process child = CHILD_PROCESS_INIT; - int status, fd, i; - struct stat st; + int status, i; + struct strbuf result_buf = STRBUF_INIT; enum ll_merge_result ret; assert(opts); @@ -241,20 +241,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, child.use_shell = 1; strvec_push(&child.args, cmd.buf); status = run_command(&child); - fd = open(temp[1], O_RDONLY); - if (fd < 0) - goto bad; - if (fstat(fd, &st)) - goto close_bad; - result->size = st.st_size; - result->ptr = xmallocz(result->size); - if (read_in_full(fd, result->ptr, result->size) != result->size) { - FREE_AND_NULL(result->ptr); - result->size = 0; + + if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) { + result->size = result_buf.len; + result->ptr = strbuf_detach(&result_buf, NULL); } - close_bad: - close(fd); - bad: + for (i = 0; i < 3; i++) unlink_or_warn(temp[i]); strbuf_release(&cmd); -- 2.56.0.rc0.314.g7a874b6915 ``` ## Jeff King, 2026-09-11 17:11 Subject: [PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles Message-ID: <20260911171139.GB1610200@coredump.intra.peff.net> In-Reply-To: <20260911171044.GA1609692@coredump.intra.peff.net> ``` When writing out tempfiles for an external merge driver, we catch the case that write() fails, but not the follow-up close(). This close() would usually succeed, but the system could report a delayed write error (e.g., on a network file system). Signed-off-by: Jeff King --- merge-ll.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index 5b6af15e23..5a11a9613b 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -180,9 +180,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len) xsnprintf(path, len, ".merge_file_XXXXXX"); fd = xmkstemp(path); - if (write_in_full(fd, src->ptr, src->size) < 0) + if (write_in_full(fd, src->ptr, src->size) < 0 || + close(fd) < 0) die_errno("unable to write temp-file"); - close(fd); } /* -- 2.56.0.rc0.314.g7a874b6915 ``` ## Jeff King, 2026-09-11 17:13 Subject: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files Message-ID: <20260911171339.GC1610200@coredump.intra.peff.net> In-Reply-To: <20260911171044.GA1609692@coredump.intra.peff.net> ``` When there's a long(er) running merge driver helper, the user may just decide to terminate it with Ctrl+C. That sends a signal to the driver prog and to the whole process group as well, including the git merge command proper. Hence the cleanup code would not run and .merge_file_* files are left behind. We can fix this by using the tempfile API, which auto-cleans files on signal or other error. That covers the Ctrl+C case above, as well as any other incidental death (e.g., allocation error due to a gigantic output). Note that there is one gotcha here. The current code uses short, relative filenames for the tempfiles (like ".merge_file_abc123"). But the tempfile API stores and returns absolute paths. Because we run the merge driver as a shell command, this can result in problems if the leading directories contain shell metacharacters (like our tests, which put a space in the trash directory name for exactly this purpose). If we were starting from scratch, I'd say the correct solution here is to shell-quote the filenames we put in the command. But doing so isn't strictly backwards compatible, because users might have their own shell characters. For example, if I configure a driver like this: [merge "foo"] driver = "my-driver '%O' '%A' '%B'" then adding extra quoting will screw things up! Strictly speaking, this kind of quoting is wrong (it would fail if %A expanded to something with a single-quote in it), but it is entirely harmless with the current vanilla relative paths. It doesn't seem worth breaking it. So let's take the most conservative route, and just continue reporting the relative paths. Commit-message-stolen-from: Michal Koutný Reported-by: Jean Delvare Signed-off-by: Jeff King --- merge-ll.c | 50 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index 5a11a9613b..ec0f012b4f 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -17,6 +17,7 @@ #include "quote.h" #include "strbuf.h" #include "gettext.h" +#include "tempfile.h" struct ll_merge_driver; @@ -174,15 +175,27 @@ static struct ll_merge_driver ll_merge_drv[] = { { "union", "built-in union merge", ll_union_merge }, }; -static void create_temp(mmfile_t *src, char *path, size_t len) +static struct tempfile *create_temp(mmfile_t *src) { - int fd; - - xsnprintf(path, len, ".merge_file_XXXXXX"); - fd = xmkstemp(path); - if (write_in_full(fd, src->ptr, src->size) < 0 || - close(fd) < 0) + struct tempfile *t = xmks_tempfile(".merge_file_XXXXXX"); + if (write_in_full(t->fd, src->ptr, src->size) < 0 || + close_tempfile_gently(t) < 0) die_errno("unable to write temp-file"); + return t; +} + +static const char *temp_path_basename(struct tempfile *t) +{ + /* + * basename() takes a non-const pointer because it can + * modify the input string to remove trailing directory + * separators. We know that we don't have any because + * this is a clean path generated from our vanilla + * tempfile template. + * + * So casting away the const here is safe, albeit gross. + */ + return basename((char *)get_tempfile_path(t)); } /* @@ -197,11 +210,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, const struct ll_merge_options *opts, int marker_size) { - char temp[3][50]; + struct tempfile *tmp_o, *tmp_a, *tmp_b; struct strbuf cmd = STRBUF_INIT; const char *format = fn->cmdline; struct child_process child = CHILD_PROCESS_INIT; - int status, i; + int status; struct strbuf result_buf = STRBUF_INIT; enum ll_merge_result ret; assert(opts); @@ -211,19 +224,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, result->ptr = NULL; result->size = 0; - create_temp(orig, temp[0], sizeof(temp[0])); - create_temp(src1, temp[1], sizeof(temp[1])); - create_temp(src2, temp[2], sizeof(temp[2])); + tmp_o = create_temp(orig); + tmp_a = create_temp(src1); + tmp_b = create_temp(src2); while (strbuf_expand_step(&cmd, &format)) { if (skip_prefix(format, "%", &format)) strbuf_addch(&cmd, '%'); else if (skip_prefix(format, "O", &format)) - strbuf_addstr(&cmd, temp[0]); + strbuf_addstr(&cmd, temp_path_basename(tmp_o)); else if (skip_prefix(format, "A", &format)) - strbuf_addstr(&cmd, temp[1]); + strbuf_addstr(&cmd, temp_path_basename(tmp_a)); else if (skip_prefix(format, "B", &format)) - strbuf_addstr(&cmd, temp[2]); + strbuf_addstr(&cmd, temp_path_basename(tmp_b)); else if (skip_prefix(format, "L", &format)) strbuf_addf(&cmd, "%d", marker_size); else if (skip_prefix(format, "P", &format)) @@ -242,13 +255,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, strvec_push(&child.args, cmd.buf); status = run_command(&child); - if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) { + if (strbuf_read_file(&result_buf, get_tempfile_path(tmp_a), 0) >= 0) { result->size = result_buf.len; result->ptr = strbuf_detach(&result_buf, NULL); } - for (i = 0; i < 3; i++) - unlink_or_warn(temp[i]); + delete_tempfile(&tmp_o); + delete_tempfile(&tmp_a); + delete_tempfile(&tmp_b); strbuf_release(&cmd); if (!status) ret = LL_MERGE_OK; -- 2.56.0.rc0.314.g7a874b6915 ``` ## Elijah Newren, 2026-09-11 18:06 Subject: Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result Message-ID: In-Reply-To: <20260911171124.GA1610200@coredump.intra.peff.net> ``` On Fri, Sep 11, 2026 at 10:11 AM Jeff King wrote: > > After the external merge runs, we read the file back into a heap buffer. > This ancient code does it by hand, but these days we can make the code > shorter and less error prone by using strbuf_read_file(). > > It's not quite a one-liner replacement, because we have to copy the > pointer and size into an mmbuffer_t. Two things to note there: > > 1. We can't just pass result->size to strbuf_detach(), since the > former uses long instead of size_t (something that we'd ideally fix > in the long run, but is way out of scope here). > > 2. We can leave result untouched on error; we zero it at the top of > the function (confusingly we may still return LL_MERGE_OK and a > NULL result if we hit an I/O error, but that is how the function > has always behaved, and callers know to check for NULL). > > Signed-off-by: Jeff King > --- > Not strictly needed for the rest of the series, but it felt like a > cleanup worth doing, and it conflicts textually. > > merge-ll.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/merge-ll.c b/merge-ll.c > index ef5287dee8..5b6af15e23 100644 > --- a/merge-ll.c > +++ b/merge-ll.c > @@ -201,8 +201,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, > struct strbuf cmd = STRBUF_INIT; > const char *format = fn->cmdline; > struct child_process child = CHILD_PROCESS_INIT; > - int status, fd, i; > - struct stat st; > + int status, i; > + struct strbuf result_buf = STRBUF_INIT; > enum ll_merge_result ret; > assert(opts); > > @@ -241,20 +241,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, > child.use_shell = 1; > strvec_push(&child.args, cmd.buf); > status = run_command(&child); > - fd = open(temp[1], O_RDONLY); > - if (fd < 0) > - goto bad; > - if (fstat(fd, &st)) > - goto close_bad; > - result->size = st.st_size; > - result->ptr = xmallocz(result->size); > - if (read_in_full(fd, result->ptr, result->size) != result->size) { > - FREE_AND_NULL(result->ptr); > - result->size = 0; > + > + if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) { > + result->size = result_buf.len; > + result->ptr = strbuf_detach(&result_buf, NULL); I know the type mismatch is pre-existing, but the order makes the new behavior different. On LLP64, assuming the usual wraparound, a result of LONG_MAX + 101 narrows to the negative value LONG_MIN + 100 . The old code narrows before xmallocz() , so it requests an impossibly large allocation and dies. The new code allocates the actual buffer first, then records a negative size; callers converting that size back to size_t could read past the allocation. Would a simple fail-fast make sense? if (result_buf.len > LONG_MAX) die(_("external merge result is too large")); > } > - close_bad: > - close(fd); > - bad: > + > for (i = 0; i < 3; i++) > unlink_or_warn(temp[i]); > strbuf_release(&cmd); > -- > 2.56.0.rc0.314.g7a874b6915 Otherwise, looks nice. ``` ## Elijah Newren, 2026-09-11 18:06 Subject: Re: [PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles Message-ID: In-Reply-To: <20260911171139.GB1610200@coredump.intra.peff.net> ``` On Fri, Sep 11, 2026 at 10:11 AM Jeff King wrote: > > When writing out tempfiles for an external merge driver, we catch the > case that write() fails, but not the follow-up close(). This close() > would usually succeed, but the system could report a delayed write error > (e.g., on a network file system). > > Signed-off-by: Jeff King > --- > merge-ll.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/merge-ll.c b/merge-ll.c > index 5b6af15e23..5a11a9613b 100644 > --- a/merge-ll.c > +++ b/merge-ll.c > @@ -180,9 +180,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len) > > xsnprintf(path, len, ".merge_file_XXXXXX"); > fd = xmkstemp(path); > - if (write_in_full(fd, src->ptr, src->size) < 0) > + if (write_in_full(fd, src->ptr, src->size) < 0 || > + close(fd) < 0) > die_errno("unable to write temp-file"); > - close(fd); > } > > /* > -- > 2.56.0.rc0.314.g7a874b6915 I got tripped up at first on this patch; if write_in_full() < 0, then we won't explicitly close(), but since die will result in an implicit close, that's not a problem. Instead, the only thing that changes is we also die if close() fails. Looks good. ``` ## Elijah Newren, 2026-09-11 18:10 Subject: Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files Message-ID: In-Reply-To: <20260911171339.GC1610200@coredump.intra.peff.net> ``` On Fri, Sep 11, 2026 at 10:13 AM Jeff King wrote: > > When there's a long(er) running merge driver helper, the user may just > decide to terminate it with Ctrl+C. That sends a signal to the driver > prog and to the whole process group as well, including the git merge Minor nit: prog -> program ? or -> process ? Or maybe tweak whole sentence? : That sends a signal to the whole foreground process group, including both the driver and the git merge process. > command proper. Hence the cleanup code would not run and .merge_file_* > files are left behind. > > We can fix this by using the tempfile API, which auto-cleans files on > signal or other error. That covers the Ctrl+C case above, as well as any > other incidental death (e.g., allocation error due to a gigantic > output). > > Note that there is one gotcha here. The current code uses short, > relative filenames for the tempfiles (like ".merge_file_abc123"). But > the tempfile API stores and returns absolute paths. Because we run the > merge driver as a shell command, this can result in problems if the > leading directories contain shell metacharacters (like our tests, which > put a space in the trash directory name for exactly this purpose). > > If we were starting from scratch, I'd say the correct solution here is > to shell-quote the filenames we put in the command. But doing so isn't > strictly backwards compatible, because users might have their own shell > characters. For example, if I configure a driver like this: > > [merge "foo"] > driver = "my-driver '%O' '%A' '%B'" > > then adding extra quoting will screw things up! Strictly speaking, this > kind of quoting is wrong (it would fail if %A expanded to something with > a single-quote in it), but it is entirely harmless with the current > vanilla relative paths. It doesn't seem worth breaking it. > > So let's take the most conservative route, and just continue reporting > the relative paths. > > Commit-message-stolen-from: Michal Koutný :-) But maybe Commit-message-mostly-stolen-from? Much of your commit message is understandably about tempfile specifics, which the original didn't have. (Yeah, probably not important enough to bother changing; I'm just "thinking out loud" as I read...) > Reported-by: Jean Delvare > Signed-off-by: Jeff King > --- > merge-ll.c | 50 ++++++++++++++++++++++++++++++++------------------ > 1 file changed, 32 insertions(+), 18 deletions(-) > > diff --git a/merge-ll.c b/merge-ll.c > index 5a11a9613b..ec0f012b4f 100644 > --- a/merge-ll.c > +++ b/merge-ll.c > @@ -17,6 +17,7 @@ > #include "quote.h" > #include "strbuf.h" > #include "gettext.h" > +#include "tempfile.h" > > struct ll_merge_driver; > > @@ -174,15 +175,27 @@ static struct ll_merge_driver ll_merge_drv[] = { > { "union", "built-in union merge", ll_union_merge }, > }; > > -static void create_temp(mmfile_t *src, char *path, size_t len) > +static struct tempfile *create_temp(mmfile_t *src) > { > - int fd; > - > - xsnprintf(path, len, ".merge_file_XXXXXX"); > - fd = xmkstemp(path); > - if (write_in_full(fd, src->ptr, src->size) < 0 || > - close(fd) < 0) > + struct tempfile *t = xmks_tempfile(".merge_file_XXXXXX"); > + if (write_in_full(t->fd, src->ptr, src->size) < 0 || > + close_tempfile_gently(t) < 0) > die_errno("unable to write temp-file"); > + return t; > +} > + > +static const char *temp_path_basename(struct tempfile *t) > +{ > + /* > + * basename() takes a non-const pointer because it can > + * modify the input string to remove trailing directory > + * separators. We know that we don't have any because > + * this is a clean path generated from our vanilla > + * tempfile template. > + * > + * So casting away the const here is safe, albeit gross. > + */ > + return basename((char *)get_tempfile_path(t)); Thanks for the comment. > } > > /* > @@ -197,11 +210,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, > const struct ll_merge_options *opts, > int marker_size) > { > - char temp[3][50]; > + struct tempfile *tmp_o, *tmp_a, *tmp_b; > struct strbuf cmd = STRBUF_INIT; > const char *format = fn->cmdline; > struct child_process child = CHILD_PROCESS_INIT; > - int status, i; > + int status; > struct strbuf result_buf = STRBUF_INIT; > enum ll_merge_result ret; > assert(opts); > @@ -211,19 +224,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, > > result->ptr = NULL; > result->size = 0; > - create_temp(orig, temp[0], sizeof(temp[0])); > - create_temp(src1, temp[1], sizeof(temp[1])); > - create_temp(src2, temp[2], sizeof(temp[2])); > + tmp_o = create_temp(orig); > + tmp_a = create_temp(src1); > + tmp_b = create_temp(src2); > > while (strbuf_expand_step(&cmd, &format)) { > if (skip_prefix(format, "%", &format)) > strbuf_addch(&cmd, '%'); > else if (skip_prefix(format, "O", &format)) > - strbuf_addstr(&cmd, temp[0]); > + strbuf_addstr(&cmd, temp_path_basename(tmp_o)); > else if (skip_prefix(format, "A", &format)) > - strbuf_addstr(&cmd, temp[1]); > + strbuf_addstr(&cmd, temp_path_basename(tmp_a)); > else if (skip_prefix(format, "B", &format)) > - strbuf_addstr(&cmd, temp[2]); > + strbuf_addstr(&cmd, temp_path_basename(tmp_b)); > else if (skip_prefix(format, "L", &format)) > strbuf_addf(&cmd, "%d", marker_size); > else if (skip_prefix(format, "P", &format)) > @@ -242,13 +255,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, > strvec_push(&child.args, cmd.buf); > status = run_command(&child); > > - if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) { > + if (strbuf_read_file(&result_buf, get_tempfile_path(tmp_a), 0) >= 0) { > result->size = result_buf.len; > result->ptr = strbuf_detach(&result_buf, NULL); > } > > - for (i = 0; i < 3; i++) > - unlink_or_warn(temp[i]); > + delete_tempfile(&tmp_o); > + delete_tempfile(&tmp_a); > + delete_tempfile(&tmp_b); > strbuf_release(&cmd); > if (!status) > ret = LL_MERGE_OK; > -- > 2.56.0.rc0.314.g7a874b6915 Looks good to me. ``` ## Junio C Hamano, 2026-09-11 18:32 Subject: Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result Message-ID: In-Reply-To: ``` Elijah Newren writes: > The old code narrows before xmallocz() , so it requests an impossibly > large allocation and dies. The new code allocates the actual buffer > first, then records a negative size; callers converting that size back > to size_t could read past the allocation. > > Would a simple fail-fast make sense? > > if (result_buf.len > LONG_MAX) > die(_("external merge result is too large")); Intereting find. That does sound sensible. ``` ## Michal Koutný, 2026-09-14 13:23 Subject: Re: [PATCH v2 0/3] merge-ll: Cleanup merge driver temporaries after Message-ID: In-Reply-To: <20260911171044.GA1609692@coredump.intra.peff.net> ``` On Fri, Sep 11, 2026 at 01:10:44PM -0400, Jeff King wrote: > On Fri, Sep 11, 2026 at 04:43:08PM +0200, Michal Koutný wrote: > > > > In the worst case we can just call register_tempfile() on each path, but > > > I think this code could be taught to use the actual creation. Something > > > like the patch below (only lightly tested). > > > > I've tested it and it works (cleans up both after SIGINT and regular > > termination). > > Thanks for testing. I considered putting something in the test suite, > but it gets ugly (we'd have the external driver pause, signal a fifo, > then kill git-merge and it with SIGINT). I guess an alternative would be > setting GIT_ALLOC_LIMIT to something low, and then generating a > too-large output, which would cause xmalloc() to fail, which I believe > would also fail. But then we're not really testing the signal handling. > Hmm. I wonder if leaving the files could actually be a _feature_. If you > completed the merge with the external tool but we barfed reading it back > in, would it be useful to leave the file in place? It's possible, I > suppose, but I think it is more likely to be a nuisance (and we already > delete it for things like read() errors, just not anything that would > cause us to die()). From the user perspective, this is unnecessary. (Potentially useful for debugging the merge tool.) For the former, the whole merge can retried (after restoring state), the latter is quite rare and can be worked around easily when the merge tool is under development. 0.02€, Michal ``` ## Michal Koutný, 2026-09-14 13:24 Subject: Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files Message-ID: In-Reply-To: ``` On Fri, Sep 11, 2026 at 11:10:03AM -0700, Elijah Newren wrote: > But maybe Commit-message-mostly-stolen-from? Much of your commit > message is understandably about tempfile specifics, which the original > didn't have. It's also OK, if you just add me to the Reported-by: chain ;-) Michal ``` ## Jeff King, 2026-09-14 16:53 Subject: Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result Message-ID: <20260914165350.GA32247@peff.net> In-Reply-To: ``` On Fri, Sep 11, 2026 at 11:06:33AM -0700, Elijah Newren wrote: > > - result->size = st.st_size; > > - result->ptr = xmallocz(result->size); > > - if (read_in_full(fd, result->ptr, result->size) != result->size) { > > - FREE_AND_NULL(result->ptr); > > - result->size = 0; > > + > > + if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) { > > + result->size = result_buf.len; > > + result->ptr = strbuf_detach(&result_buf, NULL); > > I know the type mismatch is pre-existing, but the order makes the new > behavior different. On LLP64, assuming the usual wraparound, a result > of LONG_MAX + 101 narrows to the negative value LONG_MIN + 100 . > > The old code narrows before xmallocz() , so it requests an impossibly > large allocation and dies. The new code allocates the actual buffer > first, then records a negative size; callers converting that size back > to size_t could read past the allocation. Hmm, yeah. I noticed the possible truncation, but reasoned that it was roughly the same before and after (the only difference being that we know would actually have the full buffer, just a truncated size). But you're right that negative values introduce their own distinct type of confusion. > Would a simple fail-fast make sense? > > if (result_buf.len > LONG_MAX) > die(_("external merge result is too large")); Yeah. I think we should be doing that even with the current code, as it's possible for us to silently truncate a merge result (e.g., wrapping beyond 4GB goes back to 0). There's a similar case in read_mmfile(). There we actually bother to use xsize_t() to catch _some_ problems, but of course we are using "long" and not "size_t" in the mmfile, so it's still subject to truncation. We can't just use read_mmfile() here, because there is an artificial distinction between mmfile_t and mmbuffer_t, even though they hold the exact same members (IIRC, one is for "output"). But possibly we can use it and just assign the members, which is no worse than what we have to do with the strbuf. I'll plan to add a check like the one above here and in read_mmfile(), and then look at re-working this cleanup to use that function. I'll probably also peel this off of the other patches. It's really two separate topics: this file-read cleanup, and the tempfile-deletion improvement that started the thread. -Peff ``` ## Jeff King, 2026-09-14 16:56 Subject: Re: [PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles Message-ID: <20260914165654.GB32247@peff.net> In-Reply-To: ``` On Fri, Sep 11, 2026 at 11:06:43AM -0700, Elijah Newren wrote: > > - if (write_in_full(fd, src->ptr, src->size) < 0) > > + if (write_in_full(fd, src->ptr, src->size) < 0 || > > + close(fd) < 0) > > die_errno("unable to write temp-file"); > > - close(fd); > > } > > I got tripped up at first on this patch; if write_in_full() < 0, then > we won't explicitly close(), but since die will result in an implicit > close, that's not a problem. > > Instead, the only thing that changes is we also die if close() fails. Yeah, this is a subtle mistake that we've had to fix before. Doing: if (write_in_full(fd, ...) || close(fd)) return error(...); is a hard-to-spot leak. It's not present here because we're calling die() instead of returning, but maybe it is worth writing it out to set a good example, like: if (write_in_full(...)) die_errno("unable to write"); if (close(...)) die_errno("unable to close"); Since I'm re-rolling anyway. -Peff ``` ## Jeff King, 2026-09-14 16:57 Subject: Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files Message-ID: <20260914165741.GC32247@peff.net> In-Reply-To: ``` On Mon, Sep 14, 2026 at 03:24:33PM +0200, Michal Koutný wrote: > On Fri, Sep 11, 2026 at 11:10:03AM -0700, Elijah Newren wrote: > > But maybe Commit-message-mostly-stolen-from? Much of your commit > > message is understandably about tempfile specifics, which the original > > didn't have. > > It's also OK, if you just add me to the Reported-by: chain ;-) Thanks, I wanted to make sure I credited you but wasn't sure how. I'll just do that in the re-roll. :) -Peff ``` ## Jeff King, 2026-09-14 16:59 Subject: Re: [PATCH v2 0/3] merge-ll: Cleanup merge driver temporaries after Message-ID: <20260914165903.GD32247@peff.net> In-Reply-To: ``` On Mon, Sep 14, 2026 at 03:23:28PM +0200, Michal Koutný wrote: > > Hmm. I wonder if leaving the files could actually be a _feature_. If you > > completed the merge with the external tool but we barfed reading it back > > in, would it be useful to leave the file in place? It's possible, I > > suppose, but I think it is more likely to be a nuisance (and we already > > delete it for things like read() errors, just not anything that would > > cause us to die()). > > From the user perspective, this is unnecessary. (Potentially useful for > debugging the merge tool.) > For the former, the whole merge can retried (after restoring state), the > latter is quite rare and can be worked around easily when the merge tool is > under development. I was more wondering if a user would be frustrated that they spent 30 minutes doing a really complicated merge in the tool, and then that output was lost. I'd guess it's pretty rare, though. -Peff ``` ## Jeff King, 2026-09-29 05:12 Subject: [PATCH v3 0/2] merge-ll: Cleanup merge driver temporaries after signal Message-ID: <20260929051200.GA1100000@coredump.intra.peff.net> In-Reply-To: <20260911171044.GA1609692@coredump.intra.peff.net> ``` Here's a revised version of the series to switch merge-ll to use tempfile structs. Sorry, I got derailed a bit by travel. I dropped the v2 cleanup patch to use strbuf_read() for now. It was not strictly related and I think there's a bit of a rabbit hole that extends even beyond this function. That might become its own series later. Beyond that, this is mostly the same as v2. I tweaked the error-checking for close() in the first patch so that it's more obviously correct (and can produce a slightly more informative message). The range diff is below, though it's IMHO not very informative. The drop of the cleanup patch a lot of uninteresting textual ripples. [1/2]: merge-ll: catch close() errors when writing external tempfiles [2/2]: merge-ll: use tempfile API for external driver files merge-ll.c | 51 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 18 deletions(-) 1: 62b4ac5ae0 < -: ---------- merge-ll: use strbuf to read back external merge result 2: 020e3bfcbd < -: ---------- merge-ll: catch close() errors when writing external tempfiles -: ---------- > 1: c6a4b3146d merge-ll: catch close() errors when writing external tempfiles 3: 914fafcd88 ! 2: b85e169cb3 merge-ll: use tempfile API for external driver files @@ Commit message When there's a long(er) running merge driver helper, the user may just decide to terminate it with Ctrl+C. That sends a signal to the driver - prog and to the whole process group as well, including the git merge + program and to the whole process group as well, including the git merge command proper. Hence the cleanup code would not run and .merge_file_* files are left behind. @@ Commit message So let's take the most conservative route, and just continue reporting the relative paths. - Commit-message-stolen-from: Michal Koutný Reported-by: Jean Delvare + Reported-by: Michal Koutný Signed-off-by: Jeff King ## merge-ll.c ## @@ merge-ll.c: static struct ll_merge_driver ll_merge_drv[] = { - - xsnprintf(path, len, ".merge_file_XXXXXX"); - fd = xmkstemp(path); -- if (write_in_full(fd, src->ptr, src->size) < 0 || -- close(fd) < 0) +- if (write_in_full(fd, src->ptr, src->size) < 0) +- die_errno(_("unable to write %s"), path); +- if (close(fd) < 0) +- die_errno(_("unable to close %s"), path); + struct tempfile *t = xmks_tempfile(".merge_file_XXXXXX"); -+ if (write_in_full(t->fd, src->ptr, src->size) < 0 || -+ close_tempfile_gently(t) < 0) - die_errno("unable to write temp-file"); ++ if (write_in_full(t->fd, src->ptr, src->size) < 0) ++ die_errno(_("unable to write %s"), get_tempfile_path(t)); ++ if (close_tempfile_gently(t) < 0) ++ die_errno(_("unable to close %s"), get_tempfile_path(t)); + return t; +} + @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive struct strbuf cmd = STRBUF_INIT; const char *format = fn->cmdline; struct child_process child = CHILD_PROCESS_INIT; -- int status, i; -+ int status; - struct strbuf result_buf = STRBUF_INIT; +- int status, fd, i; ++ int status, fd; + struct stat st; enum ll_merge_result ret; assert(opts); @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive strbuf_addf(&cmd, "%d", marker_size); else if (skip_prefix(format, "P", &format)) @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, + child.use_shell = 1; strvec_push(&child.args, cmd.buf); status = run_command(&child); - -- if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) { -+ if (strbuf_read_file(&result_buf, get_tempfile_path(tmp_a), 0) >= 0) { - result->size = result_buf.len; - result->ptr = strbuf_detach(&result_buf, NULL); - } - +- fd = open(temp[1], O_RDONLY); ++ fd = open(get_tempfile_path(tmp_a), O_RDONLY); + if (fd < 0) + goto bad; + if (fstat(fd, &st)) +@@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, + close_bad: + close(fd); + bad: - for (i = 0; i < 3; i++) - unlink_or_warn(temp[i]); + delete_tempfile(&tmp_o); ``` ## Jeff King, 2026-09-29 05:12 Subject: [PATCH v3 1/2] merge-ll: catch close() errors when writing external tempfiles Message-ID: <20260929051254.GA1100669@coredump.intra.peff.net> In-Reply-To: <20260929051200.GA1100000@coredump.intra.peff.net> ``` When writing out tempfiles for an external merge driver, we catch the case that write() fails, but not the follow-up close(). This close() would usually succeed, but the system could report a delayed write error (e.g., on a network file system). Since we're adding a new error message here, we'll also make the existing one match it: mark it for translation and mention the actual path. The exact wording here was picked to match some existing translated messages. Signed-off-by: Jeff King --- Since v2, this is hopefully written in a more obviously-correct way, rather than the short-circuit OR. merge-ll.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index ef5287dee8..62d402199d 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -181,8 +181,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len) xsnprintf(path, len, ".merge_file_XXXXXX"); fd = xmkstemp(path); if (write_in_full(fd, src->ptr, src->size) < 0) - die_errno("unable to write temp-file"); - close(fd); + die_errno(_("unable to write %s"), path); + if (close(fd) < 0) + die_errno(_("unable to close %s"), path); } /* -- 2.56.0.rc2.338.gcaacf6bdf7 ``` ## Jeff King, 2026-09-29 05:13 Subject: [PATCH v3 2/2] merge-ll: use tempfile API for external driver files Message-ID: <20260929051312.GB1100669@coredump.intra.peff.net> In-Reply-To: <20260929051200.GA1100000@coredump.intra.peff.net> ``` When there's a long(er) running merge driver helper, the user may just decide to terminate it with Ctrl+C. That sends a signal to the driver program and to the whole process group as well, including the git merge command proper. Hence the cleanup code would not run and .merge_file_* files are left behind. We can fix this by using the tempfile API, which auto-cleans files on signal or other error. That covers the Ctrl+C case above, as well as any other incidental death (e.g., allocation error due to a gigantic output). Note that there is one gotcha here. The current code uses short, relative filenames for the tempfiles (like ".merge_file_abc123"). But the tempfile API stores and returns absolute paths. Because we run the merge driver as a shell command, this can result in problems if the leading directories contain shell metacharacters (like our tests, which put a space in the trash directory name for exactly this purpose). If we were starting from scratch, I'd say the correct solution here is to shell-quote the filenames we put in the command. But doing so isn't strictly backwards compatible, because users might have their own shell characters. For example, if I configure a driver like this: [merge "foo"] driver = "my-driver '%O' '%A' '%B'" then adding extra quoting will screw things up! Strictly speaking, this kind of quoting is wrong (it would fail if %A expanded to something with a single-quote in it), but it is entirely harmless with the current vanilla relative paths. It doesn't seem worth breaking it. So let's take the most conservative route, and just continue reporting the relative paths. Reported-by: Jean Delvare Reported-by: Michal Koutný Signed-off-by: Jeff King --- merge-ll.c | 54 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index 62d402199d..0eadbfba23 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -17,6 +17,7 @@ #include "quote.h" #include "strbuf.h" #include "gettext.h" +#include "tempfile.h" struct ll_merge_driver; @@ -174,16 +175,28 @@ static struct ll_merge_driver ll_merge_drv[] = { { "union", "built-in union merge", ll_union_merge }, }; -static void create_temp(mmfile_t *src, char *path, size_t len) +static struct tempfile *create_temp(mmfile_t *src) { - int fd; - - xsnprintf(path, len, ".merge_file_XXXXXX"); - fd = xmkstemp(path); - if (write_in_full(fd, src->ptr, src->size) < 0) - die_errno(_("unable to write %s"), path); - if (close(fd) < 0) - die_errno(_("unable to close %s"), path); + struct tempfile *t = xmks_tempfile(".merge_file_XXXXXX"); + if (write_in_full(t->fd, src->ptr, src->size) < 0) + die_errno(_("unable to write %s"), get_tempfile_path(t)); + if (close_tempfile_gently(t) < 0) + die_errno(_("unable to close %s"), get_tempfile_path(t)); + return t; +} + +static const char *temp_path_basename(struct tempfile *t) +{ + /* + * basename() takes a non-const pointer because it can + * modify the input string to remove trailing directory + * separators. We know that we don't have any because + * this is a clean path generated from our vanilla + * tempfile template. + * + * So casting away the const here is safe, albeit gross. + */ + return basename((char *)get_tempfile_path(t)); } /* @@ -198,11 +211,11 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, const struct ll_merge_options *opts, int marker_size) { - char temp[3][50]; + struct tempfile *tmp_o, *tmp_a, *tmp_b; struct strbuf cmd = STRBUF_INIT; const char *format = fn->cmdline; struct child_process child = CHILD_PROCESS_INIT; - int status, fd, i; + int status, fd; struct stat st; enum ll_merge_result ret; assert(opts); @@ -212,19 +225,19 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, result->ptr = NULL; result->size = 0; - create_temp(orig, temp[0], sizeof(temp[0])); - create_temp(src1, temp[1], sizeof(temp[1])); - create_temp(src2, temp[2], sizeof(temp[2])); + tmp_o = create_temp(orig); + tmp_a = create_temp(src1); + tmp_b = create_temp(src2); while (strbuf_expand_step(&cmd, &format)) { if (skip_prefix(format, "%", &format)) strbuf_addch(&cmd, '%'); else if (skip_prefix(format, "O", &format)) - strbuf_addstr(&cmd, temp[0]); + strbuf_addstr(&cmd, temp_path_basename(tmp_o)); else if (skip_prefix(format, "A", &format)) - strbuf_addstr(&cmd, temp[1]); + strbuf_addstr(&cmd, temp_path_basename(tmp_a)); else if (skip_prefix(format, "B", &format)) - strbuf_addstr(&cmd, temp[2]); + strbuf_addstr(&cmd, temp_path_basename(tmp_b)); else if (skip_prefix(format, "L", &format)) strbuf_addf(&cmd, "%d", marker_size); else if (skip_prefix(format, "P", &format)) @@ -242,7 +255,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, child.use_shell = 1; strvec_push(&child.args, cmd.buf); status = run_command(&child); - fd = open(temp[1], O_RDONLY); + fd = open(get_tempfile_path(tmp_a), O_RDONLY); if (fd < 0) goto bad; if (fstat(fd, &st)) @@ -256,8 +269,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, close_bad: close(fd); bad: - for (i = 0; i < 3; i++) - unlink_or_warn(temp[i]); + delete_tempfile(&tmp_o); + delete_tempfile(&tmp_a); + delete_tempfile(&tmp_b); strbuf_release(&cmd); if (!status) ret = LL_MERGE_OK; -- 2.56.0.rc2.338.gcaacf6bdf7 ``` ## Junio C Hamano, 2026-09-29 16:51 Subject: Re: [PATCH v3 2/2] merge-ll: use tempfile API for external driver files Message-ID: In-Reply-To: <20260929051312.GB1100669@coredump.intra.peff.net> ``` Jeff King writes: > When there's a long(er) running merge driver helper, the user may just > decide to terminate it with Ctrl+C. That sends a signal to the driver > program and to the whole process group as well, including the git merge > command proper. Hence the cleanup code would not run and .merge_file_* > files are left behind. > > We can fix this by using the tempfile API, which auto-cleans files on > signal or other error. That covers the Ctrl+C case above, as well as any > other incidental death (e.g., allocation error due to a gigantic > output). > > Note that there is one gotcha here. The current code uses short, > relative filenames for the tempfiles (like ".merge_file_abc123"). But > the tempfile API stores and returns absolute paths. Because we run the > merge driver as a shell command, this can result in problems if the > leading directories contain shell metacharacters (like our tests, which > put a space in the trash directory name for exactly this purpose). > > If we were starting from scratch, I'd say the correct solution here is > to shell-quote the filenames we put in the command. But doing so isn't > strictly backwards compatible, because users might have their own shell > characters. For example, if I configure a driver like this: "own shell characters" -> "own shell quoting"? > > [merge "foo"] > driver = "my-driver '%O' '%A' '%B'" > > then adding extra quoting will screw things up! Strictly speaking, this > kind of quoting is wrong (it would fail if %A expanded to something with > a single-quote in it), but it is entirely harmless with the current > vanilla relative paths. It doesn't seem worth breaking it. > > So let's take the most conservative route, and just continue reporting > the relative paths. Very well reasoned, and the implementation exactly matches the designed behaviour. Will replace. Let's mark it for 'next' (unless somebody notices what I overlooked, which is not a very high bar to cross). Thanks. ``` ## Jeff King, 2026-09-29 18:25 Subject: Re: [PATCH v3 2/2] merge-ll: use tempfile API for external driver files Message-ID: <20260929182537.GA1710046@coredump.intra.peff.net> In-Reply-To: ``` On Tue, Sep 29, 2026 at 09:51:53AM -0700, Junio C Hamano wrote: > > If we were starting from scratch, I'd say the correct solution here is > > to shell-quote the filenames we put in the command. But doing so isn't > > strictly backwards compatible, because users might have their own shell > > characters. For example, if I configure a driver like this: > > "own shell characters" -> "own shell quoting"? Hmm, yeah. I was thinking that our quoting could disrupt other shell metacharacters they used. But I guess if it is only surrounding the filenames we provide, only their quoting characters could matter. So if they wrote: --option='%A' '--option=%A' --option="%A" and so forth. -Peff ```