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

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

[Michal Koutný]

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 <jdelvare@suse.de>
Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 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

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

[Jeff King]

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;

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

Hi.

On Thu, Sep 10, 2026 at 12:22:42PM -0400, Jeff King <peff@peff.net> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

[PATCH v2 0/3] merge-ll: Cleanup merge driver temporaries after

[Jeff King]

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

[PATCH v2 1/3] merge-ll: use strbuf to read back external merge result

[Jeff King]

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 <peff@peff.net>
---
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

[PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH v2 3/3] merge-ll: use tempfile API for external driver files

[Jeff King]

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ý <mkoutny@suse.com>
Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 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

Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result

[Elijah Newren]

On Fri, Sep 11, 2026 at 10:11 AM Jeff King <peff@peff.net> 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 <peff@peff.net>
> ---
> 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.

Re: [PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles

[Elijah Newren]

On Fri, Sep 11, 2026 at 10:11 AM Jeff King <peff@peff.net> 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 <peff@peff.net>
> ---
>  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.

Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files

[Elijah Newren]

On Fri, Sep 11, 2026 at 10:13 AM Jeff King <peff@peff.net> 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ý <mkoutny@suse.com>

:-)

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 <jdelvare@suse.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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.

Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result

[Junio C Hamano]

Elijah Newren <newren@gmail.com> 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.