From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 18 Nov 2025 23:08:31 GMT
Subject: Re: [PATCH] wrapper: simplify xmkstemp()
Message-ID: <xmqqqztvc51s.fsf@gitster.g>
In-Reply-To: <3b1cb53a-6427-4626-a768-1961e25514f8@web.de>

René Scharfe <l.s.r@web.de> writes:

> On 11/18/25 10:46 AM, Jeff King wrote:
>> 
>> 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).
> Thought about it before, but couldn't bring myself to ban mkstemp(3).
> Its only faults are lack of features (mode setting and suffix support)
> and not being available on Windows, but apart from that it does its
> job as advertised.  Which means ... it doesn't cut it for us.  Hmm.

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.

> --- >8 ---
> Subject: [PATCH] stop using mkstemp(3)
>
> mkstemp(3) works fine if you don't need custom permissions, a specific
> filename suffix or to run it on Windows.  For those cases we have a
> custom implementation around git_mkstemps_mode().  Use it for the base
> case as well, for consistency across platforms.
>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  compat/mingw-posix.h | 1 -
>  compat/mingw.c       | 5 -----
>  git-compat-util.h    | 2 ++
>  3 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
> index 631a208684..57915119c6 100644
> --- a/compat/mingw-posix.h
> +++ b/compat/mingw-posix.h
> @@ -185,7 +185,6 @@ char *mingw_locate_in_PATH(const char *cmd);
>  
>  int pipe(int filedes[2]);
>  unsigned int sleep (unsigned int seconds);
> -int mkstemp(char *template);
>  int gettimeofday(struct timeval *tv, void *tz);
>  #ifndef __MINGW64_VERSION_MAJOR
>  struct tm *gmtime_r(const time_t *timep, struct tm *result);
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 736a07a028..dc3da7c6d5 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1174,11 +1174,6 @@ char *mingw_mktemp(char *template)
>  	return template;
>  }
>  
> -int mkstemp(char *template)
> -{
> -	return git_mkstemp_mode(template, 0600);
> -}
> -
>  int gettimeofday(struct timeval *tv, void *tz UNUSED)
>  {
>  	FILETIME ft;
> 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)
> +
>  /* General helper functions */
>  NORETURN void usage(const char *err);
>  NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));