Re: [PATCH] wrapper: simplify xmkstemp()

From: Jeff King <peff@peff.net>
Date: Nov 20, 2025, 08:23 UTC
Message-ID: <20251120082328.GD1283645@coredump.intra.peff.net>
In-Reply-To: <xmqqqztvc51s.fsf@gitster.g>

On Tue, Nov 18, 2025 at 03:08:31PM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> When somebody asks:
> 
>     On this and that platforms, mkstemp() is natively available.
>     Why are we using git_mkstemp_mode() instead?
> 
> after seeing this patch, I am tempted to say "Why not?"  Are there
> legitimate answers to my "What not?"
> 
>  - the platform native one could be more performant?
>  - the platform native one could be more secure?
>  - using the platform native one, we can lose out custom code?
> 
> None of the ones I can come up with offhand sound very legitimate.
> 
> One upside might be that doing so would make the behaviour more
> predictable, in that even on a platform with native mkstemp(), we
> would use the same implementation as what we use on Windows.  But
> I do not know how much upside it is in practice, either.

I think predictability cuts both ways. The system mkstemp() will behave more like it does on the rest of that platform, but maybe less like Git on other platforms. Using Git's implementation will be consistent across platforms, but maybe inconsistent with the rest of the current platform.

I think the "consistent with the rest of the current platform" ship may have already sailed, though. We already use our custom git_mkstemp_mode() on every platform for most tempfiles. And now even those few xmkstemp() calls will do so (after René's first patch).

My suggestion was mostly: if we are going to use custom code at all, then let's at least do so always, and not ever use the system mkstemp().

Show 9 quoted lines

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 398e0fac4f..0e6bd266cc 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -446,6 +446,8 @@ static inline int git_has_dir_sep(const char *path)
> >  
> >  #include "wrapper.h"
> >  
> > +#define mkstemp(template) git_mkstemp_mode((template), 0600)

So this patch implements what I was thinking, though I probably would have made it more explicit: add mkstemp() to the banned list (not because it's evil but because it's unportable) and force callers to use git_mkstemp_mode() explicitly.

-Peff