From: Elijah Newren Date: Fri, 11 Sep 2026 18:10:03 GMT Subject: Re: [PATCH v2 3/3] merge-ll: use tempfile API for external driver files Message-ID: In-Reply-To: <20260911171339.GC1610200@coredump.intra.peff.net> On Fri, Sep 11, 2026 at 10:13 AM Jeff King 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ý :-) 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 > Signed-off-by: Jeff King > --- > 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.