Re: [PATCH] merge-ll: Cleanup merge driver temporaries after interrupt

From

Jeff King <peff@peff.net>

Date

Sep 10, 2026, 16:22 UTC

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.

---

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;