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(-)

On Thu, Sep 10, 2026 at 05:06:07PM +0200, Michal Koutný 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.

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).

---

Hi.

On Thu, Sep 10, 2026 at 12:22:42PM -0400, Jeff King <peff@peff.net> wrote:

That sounds like even a better idiom to achieve the goal.

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

On Fri, Sep 11, 2026 at 04:43:08PM +0200, Michal Koutný wrote:

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()).

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

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

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(-)

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(-)

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(-)

On Fri, Sep 11, 2026 at 10:11 AM Jeff King <peff@peff.net> wrote:

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"));

Otherwise, looks nice.

On Fri, Sep 11, 2026 at 10:11 AM Jeff King <peff@peff.net> wrote:

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.

On Fri, Sep 11, 2026 at 10:13 AM Jeff King <peff@peff.net> wrote:

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.

:-)

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...)

Thanks for the comment.

Looks good to me.

Elijah Newren <newren@gmail.com> writes:

Intereting find.  That does sound sensible.

On Fri, Sep 11, 2026 at 01:10:44PM -0400, Jeff King <peff@peff.net> wrote:

From the user perspective, this is unnecessary. (Potentially useful for debugging the merge tool.) For the former, the whole merge can retried (after restoring state), the latter is quite rare and can be worked around easily when the merge tool is under development.

0.02€, Michal

On Fri, Sep 11, 2026 at 11:10:03AM -0700, Elijah Newren <newren@gmail.com> wrote:

It's also OK, if you just add me to the Reported-by: chain ;-)

Michal

On Fri, Sep 11, 2026 at 11:06:33AM -0700, Elijah Newren wrote:

Hmm, yeah. I noticed the possible truncation, but reasoned that it was roughly the same before and after (the only difference being that we know would actually have the full buffer, just a truncated size). But you're right that negative values introduce their own distinct type of confusion.

Yeah. I think we should be doing that even with the current code, as it's possible for us to silently truncate a merge result (e.g., wrapping beyond 4GB goes back to 0).

There's a similar case in read_mmfile(). There we actually bother to use xsize_t() to catch _some_ problems, but of course we are using "long" and not "size_t" in the mmfile, so it's still subject to truncation.

We can't just use read_mmfile() here, because there is an artificial distinction between mmfile_t and mmbuffer_t, even though they hold the exact same members (IIRC, one is for "output"). But possibly we can use it and just assign the members, which is no worse than what we have to do with the strbuf.

I'll plan to add a check like the one above here and in read_mmfile(), and then look at re-working this cleanup to use that function. I'll probably also peel this off of the other patches. It's really two separate topics: this file-read cleanup, and the tempfile-deletion improvement that started the thread.

-Peff

On Fri, Sep 11, 2026 at 11:06:43AM -0700, Elijah Newren wrote:

Yeah, this is a subtle mistake that we've had to fix before. Doing:

  if (write_in_full(fd, ...) || close(fd))
	return error(...);

is a hard-to-spot leak. It's not present here because we're calling die() instead of returning, but maybe it is worth writing it out to set a good example, like:

  if (write_in_full(...))
	die_errno("unable to write");
  if (close(...))
	die_errno("unable to close");

Since I'm re-rolling anyway.

-Peff

On Mon, Sep 14, 2026 at 03:24:33PM +0200, Michal Koutný wrote:

Thanks, I wanted to make sure I credited you but wasn't sure how. I'll just do that in the re-roll. :)

-Peff

On Mon, Sep 14, 2026 at 03:23:28PM +0200, Michal Koutný wrote:

I was more wondering if a user would be frustrated that they spent 30 minutes doing a really complicated merge in the tool, and then that output was lost. I'd guess it's pretty rare, though.

-Peff

Here's a revised version of the series to switch merge-ll to use tempfile structs. Sorry, I got derailed a bit by travel.

I dropped the v2 cleanup patch to use strbuf_read() for now. It was not strictly related and I think there's a bit of a rabbit hole that extends even beyond this function. That might become its own series later.

Beyond that, this is mostly the same as v2. I tweaked the error-checking for close() in the first patch so that it's more obviously correct (and can produce a slightly more informative message).

The range diff is below, though it's IMHO not very informative. The drop of the cleanup patch a lot of uninteresting textual ripples.

  [1/2]: merge-ll: catch close() errors when writing external tempfiles
  [2/2]: merge-ll: use tempfile API for external driver files

 merge-ll.c | 51 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 18 deletions(-)

1:  62b4ac5ae0 < -:  ---------- merge-ll: use strbuf to read back external merge result
2:  020e3bfcbd < -:  ---------- merge-ll: catch close() errors when writing external tempfiles
-:  ---------- > 1:  c6a4b3146d merge-ll: catch close() errors when writing external tempfiles
3:  914fafcd88 ! 2:  b85e169cb3 merge-ll: use tempfile API for external driver files
    @@ Commit message
     
         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
    +    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.
     
    @@ Commit message
         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>
    +    Reported-by: Michal Koutný <mkoutny@suse.com>
         Signed-off-by: Jeff King <peff@peff.net>
     
      ## merge-ll.c ##
    @@ merge-ll.c: static struct ll_merge_driver ll_merge_drv[] = {
     -
     -	xsnprintf(path, len, ".merge_file_XXXXXX");
     -	fd = xmkstemp(path);
    --	if (write_in_full(fd, src->ptr, src->size) < 0 ||
    --	    close(fd) < 0)
    +-	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);
     +	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");
    ++	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;
     +}
     +
    @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive
      	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;
    +-	int status, fd, i;
    ++	int status, fd;
    + 	struct stat st;
      	enum ll_merge_result ret;
      	assert(opts);
     @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
    @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive
      			strbuf_addf(&cmd, "%d", marker_size);
      		else if (skip_prefix(format, "P", &format))
     @@ merge-ll.c: 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);
    - 
    --	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);
    - 	}
    - 
    +-	fd = open(temp[1], O_RDONLY);
    ++	fd = open(get_tempfile_path(tmp_a), O_RDONLY);
    + 	if (fd < 0)
    + 		goto bad;
    + 	if (fstat(fd, &st))
    +@@ merge-ll.c: 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);

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 <peff@peff.net>
---
Since v2, this is hopefully written in a more obviously-correct way,
rather than the short-circuit OR.

 merge-ll.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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 <jdelvare@suse.de>
Reported-by: Michal Koutný <mkoutny@suse.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c | 54 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

Jeff King <peff@peff.net> writes:

"own shell characters" -> "own shell quoting"?

Very well reasoned, and the implementation exactly matches the designed behaviour.

Will replace. Let's mark it for 'next' (unless somebody notices what I overlooked, which is not a very high bar to cross).

Thanks.

On Tue, Sep 29, 2026 at 09:51:53AM -0700, Junio C Hamano wrote:

Hmm, yeah. I was thinking that our quoting could disrupt other shell metacharacters they used. But I guess if it is only surrounding the filenames we provide, only their quoting characters could matter. So if they wrote:

  --option='%A'
  '--option=%A'
  --option="%A"

and so forth.

-Peff