From: Jeff King <peff@peff•net>
To: Junio C Hamano <gitster@pobox•com>
Cc: "René Scharfe" <l.s.r@web•de>, "Git List" <git@vger•kernel.org>
Subject: Re: [PATCH] wrapper: simplify xmkstemp()
Date: Thu, 20 Nov 2025 03:23:28 -0500 [thread overview]
Message-ID: <20251120082328.GD1283645@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqqztvc51s.fsf@gitster.g>
On Tue, Nov 18, 2025 at 03:08:31PM -0800, Junio C Hamano wrote:
> 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().
> > 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
next prev parent reply other threads:[~2025-11-20 8:23 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-17 19:42 [PATCH] wrapper: simplify xmkstemp() René Scharfe
2025-11-17 21:52 ` Junio C Hamano
2025-11-18 9:46 ` Jeff King
2025-11-18 22:29 ` René Scharfe
2025-11-18 23:08 ` Junio C Hamano
2025-11-20 8:23 ` Jeff King [this message]
2025-11-20 14:39 ` Junio C Hamano
2025-11-22 13:29 ` René Scharfe
2025-11-22 13:24 ` René Scharfe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251120082328.GD1283645@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=l.s.r@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox