From 25f7f0906642be73bf4360487481b1d960b9d142 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Sep 2026 13:11:24 -0400 Subject: [PATCH 1/3] merge-ll: use strbuf to read back external merge result 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 Signed-off-by: Junio C Hamano --- merge-ll.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index fafe2c9197..b9d52c423f 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); From 5c86262d4d3c09996df93c458dc7129fce7187a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Sep 2026 13:11:39 -0400 Subject: [PATCH 2/3] 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). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- merge-ll.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index b9d52c423f..96b6b49bc0 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); } /* From 2aaf1866f47da7cc0d577186ea9ad2350156d614 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Sep 2026 13:13:39 -0400 Subject: [PATCH 3/3] 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 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 Signed-off-by: Junio C Hamano --- merge-ll.c | 50 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/merge-ll.c b/merge-ll.c index 96b6b49bc0..df57eff823 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;