From: Jeff King <peff@peff.net>
Date: Tue, 18 Nov 2025 09:46:21 GMT
Subject: Re: [PATCH] wrapper: simplify xmkstemp()
Message-ID: <20251118094621.GB530545@coredump.intra.peff.net>
In-Reply-To: <xmqqbjl0iax6.fsf@gitster.g>

On Mon, Nov 17, 2025 at 01:52:53PM -0800, Junio C Hamano wrote:

> >  int xmkstemp(char *filename_template)
> >  {
> > -	int fd;
> > -	char origtemplate[PATH_MAX];
> > -	strlcpy(origtemplate, filename_template, sizeof(origtemplate));
> > -
> > -	fd = mkstemp(filename_template);
> > -	if (fd < 0) {
> > -		int saved_errno = errno;
> > -		const char *nonrelative_template;
> > -
> > -		if (strlen(filename_template) != strlen(origtemplate))
> > -			filename_template = origtemplate;
> > -
> > -		nonrelative_template = absolute_path(filename_template);
> > -		errno = saved_errno;
> > -		die_errno("Unable to create temporary file '%s'",
> > -			nonrelative_template);
> > -	}
> > -	return fd;
> > +	return xmkstemp_mode(filename_template, 0600);
> >  }
> 
> A patch that loses lines is nice.  My curiosity wonders what the
> strlen() comparison in the original was about, but let's not waste
> our brain cycles to code that we no longer use ;-).  xmkstemp_mode()
> checks if our git_mkstemp_mode() cleared the template[0] as a sign
> to restore the origtemplate, and uses the template that was munged
> by git_mkstemp_mode() and used to attempt opening it, which seems
> very sensible.

I agree that we do not need to worry about code that is going away,
but...it made me wonder if the reason for the strlen() applied equally
to git_mkstemp_mode(). I.e., if it has had a small bug for a long time,
and we are now about to expose it to a wider audience.

Looks like it comes from f7be59b477 (xmkstemp(): avoid showing truncated
template more carefully, 2012-12-18), where some implementations of
mkstemp() would truncate "foo/bar.XXXXX" as "foo\0". But since we are
now always using our own function, we know it truncates at the very
start. So we do not need to worry about that hack.


I also wondered if we ever use mkstemp() at all after this patch. If
not, we might want to declare it off-limits. Not because it is evil, but
because our own implementation is more predictable (and we can drop the
compat wrappers for mingw). It looks like there is one more call in
entry.c's open_output_fd(), but arguably that should be calling
xmkstemp() or git_mkstemp_mode(). But that's out of scope for this patch
(I just thought I might nerd-snipe René into looking at it).

-Peff