path: root/tempfile.c
diff options
authorJeff King <>2017-09-05 12:14:30 (GMT)
committerJunio C Hamano <>2017-09-06 08:19:53 (GMT)
commit49bd0fc2220eef17d8f5fd3ee76e391d03df8a6d (patch)
treefb867abaa9885a795b7cc34ff870211ac522302f /tempfile.c
parent45c6b1ed24724f7f3041a60a4313df7d9c4b9909 (diff)
tempfile: do not delete tempfile on failed close
When close_tempfile() fails, we delete the tempfile and reset the fields of the tempfile struct. This makes it easier for callers to return without cleaning up, but it also makes this common pattern: if (close_tempfile(tempfile)) return error_errno("error closing %s", tempfile->filename.buf); wrong, because the "filename" field has been reset after the failed close. And it's not easy to fix, as in many cases we don't have another copy of the filename (e.g., if it was created via one of the mks_tempfile functions, and we just have the original template string). Let's drop the feature that a failed close automatically deletes the file. This puts the burden on the caller to do the deletion themselves, but this isn't that big a deal. Callers which do: if (write(...) || close_tempfile(...)) { delete_tempfile(...); return -1; } already had to call delete when the write() failed, and so aren't affected. Likewise, any caller which just calls die() in the error path is OK; we'll delete the tempfile during the atexit handler. Because this patch changes the semantics of close_tempfile() without changing its signature, all callers need to be manually checked and converted to the new scheme. This patch covers all in-tree callers, but there may be others for not-yet-merged topics. To catch these, we rename the function to close_tempfile_gently(), which will attract compile-time attention to new callers. (Technically the original could be considered "gentle" already in that it didn't die() on errors, but this one is even more so). Signed-off-by: Jeff King <> Signed-off-by: Junio C Hamano <>
Diffstat (limited to 'tempfile.c')
1 files changed, 12 insertions, 19 deletions
diff --git a/tempfile.c b/tempfile.c
index 6843710..c6740e5 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -30,13 +30,12 @@
* `fdopen_tempfile()` has been called on the object
* - `owner` holds the PID of the process that created the file
- * - Active, file closed (after successful `close_tempfile()`). Same
+ * - Active, file closed (after `close_tempfile_gently()`). Same
* as the previous state, except that the temporary file is closed,
* `fd` is -1, and `fp` is `NULL`.
- * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, a
- * failed attempt to create a temporary file, or a failed
- * `close_tempfile()`). In this state:
+ * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, or a
+ * failed attempt to create a temporary file). In this state:
* - `active` is unset
* - `filename` is empty (usually, though there are transitory
@@ -235,7 +234,7 @@ FILE *get_tempfile_fp(struct tempfile *tempfile)
return tempfile->fp;
-int close_tempfile(struct tempfile *tempfile)
+int close_tempfile_gently(struct tempfile *tempfile)
int fd = tempfile->fd;
FILE *fp = tempfile->fp;
@@ -258,14 +257,7 @@ int close_tempfile(struct tempfile *tempfile)
err = close(fd);
- if (err) {
- int save_errno = errno;
- delete_tempfile(tempfile);
- errno = save_errno;
- return -1;
- }
- return 0;
+ return err ? -1 : 0;
int reopen_tempfile(struct tempfile *tempfile)
@@ -283,8 +275,10 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)
if (!tempfile->active)
die("BUG: rename_tempfile called for inactive object");
- if (close_tempfile(tempfile))
+ if (close_tempfile_gently(tempfile)) {
+ delete_tempfile(tempfile);
return -1;
+ }
if (rename(tempfile->filename.buf, path)) {
int save_errno = errno;
@@ -303,9 +297,8 @@ void delete_tempfile(struct tempfile *tempfile)
if (!tempfile->active)
- if (!close_tempfile(tempfile)) {
- unlink_or_warn(tempfile->filename.buf);
- tempfile->active = 0;
- strbuf_reset(&tempfile->filename);
- }
+ close_tempfile_gently(tempfile);
+ unlink_or_warn(tempfile->filename.buf);
+ tempfile->active = 0;
+ strbuf_reset(&tempfile->filename);