Am 15.07.22 um 18:38 schrieb Junio C Hamano: > Johannes Sixt <j6t@xxxxxxxx> writes: > >>> diff --git a/compat/mingw.c b/compat/mingw.c >>> index 2607de93af..b5502997e2 100644 >>> --- a/compat/mingw.c >>> +++ b/compat/mingw.c >>> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template) >>> >>> int mkstemp(char *template) >>> { >>> - char *filename = mktemp(template); >>> - if (!filename) >>> - return -1; >>> - return open(filename, O_RDWR | O_CREAT, 0600); >>> + return git_mkstemp_mode(template, 0600); >>> } >>> >>> int gettimeofday(struct timeval *tv, void *tz) >> >> I hate such an obvious layering violation. But we have already a ton of >> them elsewhere (calling xmalloc from compat/, for example), so this is >> just a rant, not an objection. > > There is intended behaviour difference between xmalloc() and > malloc() that justifies your "layering violation" observation. A > low level library replacement implemented in compat/ should not call > die() when it fails to obtain memory and instead report an error. > > But it is unclear git_mkstemp_mode() and some others in wrapper.c > fall into the same category. With the posted implementation above, > the end result is how the platform that lack working mkstemp() would > implement it. We'd have a problem if git_mkstemp_mode() depended on mkstemp(). It doesn't now, but if it is ever turned into a mkstemp() wrapper then it would fall flat on its face, but only on Windows. > If we were to do something to make it "cleaner", I suspect that the > above implementatoin of mkstemp() can be moved out of compat/mingw.c > and made reusable by _anybody_ who lack mkstemp(), just like we ship > memmem() emulation for anybody who lack it in contrib/memmem.c All other platforms seem to have a native mkstemp(). There may still be interest in using our own if the native implementation is inferior, e.g. uses a weak RNG and/or doesn't retry. We could also convert the handful of mkstemp() callers (two if we ignore reftable/) to use git_mkstemp_mode(). One of them is quite tempting for the code deduplication aspect alone (see below).. René --- wrapper.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/wrapper.c b/wrapper.c index 1c3c970080..77ca923040 100644 --- a/wrapper.c +++ b/wrapper.c @@ -439,24 +439,7 @@ FILE *fopen_or_warn(const char *path, const char *mode) 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); } /* Adapted from libiberty's mkstemp.c. */ -- 2.37.0