Re: [PATCH] wrapper: simplify xmkstemp()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 18, 2025, 23:08 UTC
- Message-ID
- <xmqqqztvc51s.fsf@gitster.g>
- In-Reply-To
- <3b1cb53a-6427-4626-a768-1961e25514f8@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 13 quoted lines
> 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.
Show 57 quoted lines
> --- >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)));