Re: [PATCH v3 2/2] merge-ll: use tempfile API for external driver files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 29, 2026, 16:51 UTC
- Message-ID
- <xmqqcxtwhufq.fsf@gitster.g>
- In-Reply-To
- <20260929051312.GB1100669@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> 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:
"own shell characters" -> "own shell quoting"?
Show 11 quoted lines
> > [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.
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.