From 3ddcf99d04409500df1be1b1619e541448e42026 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 29 Sep 2026 01:12:54 -0400 Subject: [PATCH 1/2] merge-ll: catch close() errors when writing external tempfiles 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 Signed-off-by: Junio C Hamano --- merge-ll.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index fafe2c9197..01bd70e183 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); } /* From 5efbff499c2946041ac2950ac248bac9028a16a4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 29 Sep 2026 01:13:12 -0400 Subject: [PATCH 2/2] merge-ll: use tempfile API for external driver files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- merge-ll.c | 52 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index 01bd70e183..b45598c60f 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; + 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; +} - 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); +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;