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

[Jeff King <peff@peff.net>]

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;