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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/diff.c b/diff.c
index fdb974014b4266ae5d4c6e6735097321bc29ed24..20f68ec3892cb51d87fdae629361e6cc1f80e948 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -3739,7 +3739,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
                size = buf.len;
        }
        if (write_in_full(fd, blob, size) != size ||
-           close_tempfile(&temp->tempfile))
+           close_tempfile_gently(&temp->tempfile))
                die_errno("unable to write temp-file");
        temp->name = get_tempfile_path(&temp->tempfile);
        oid_to_hex_r(temp->hex, oid);

diff --git a/gpg-interface.c b/gpg-interface.c
index 05ca6ecbfd915a31fe468053560300e1f4873cc3..4ea2597ff4cb304c82419eb515b07a04673b7f5f 100644 (file)
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -210,7 +210,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
        if (fd < 0)
                return error_errno(_("could not create temporary file"));
        if (write_in_full(fd, signature, signature_size) < 0 ||
-           close_tempfile(&temp) < 0) {
+           close_tempfile_gently(&temp) < 0) {
                error_errno(_("failed writing detached signature to '%s'"),
                            temp.filename.buf);
                delete_tempfile(&temp);

diff --git a/lockfile.h b/lockfile.h
index 572064939c718281b56abda7757bb21d35f42de3..dd4259bc402b4d573895a8e0ce37f53a0db04531 100644 (file)
--- a/lockfile.h
+++ b/lockfile.h
@@ -246,7 +246,13 @@ extern char *get_locked_file_path(struct lock_file *lk);
  */
 static inline int close_lock_file(struct lock_file *lk)
 {
-       return close_tempfile(&lk->tempfile);
+       int ret = close_tempfile_gently(&lk->tempfile);
+       if (ret) {
+               int saved_errno = errno;
+               delete_tempfile(&lk->tempfile);
+               errno = saved_errno;
+       }
+       return ret;
 }
 
 /*

diff --git a/read-cache.c b/read-cache.c
index 40da87ea71f088114c5a40893850cc88efee6953..51686518e061b8fe60ddac620815412ba6724efa 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -2309,8 +2309,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 
        if (ce_flush(&c, newfd, istate->sha1))
                return -1;
-       if (close_tempfile(tempfile))
-               return error(_("could not close '%s'"), tempfile->filename.buf);
+       if (close_tempfile_gently(tempfile)) {
+               error(_("could not close '%s'"), tempfile->filename.buf);
+               delete_tempfile(tempfile);
+               return -1;
+       }
        if (stat(tempfile->filename.buf, &st))
                return -1;
        istate->timestamp.sec = (unsigned int)st.st_mtime;

diff --git a/shallow.c b/shallow.c
index f49e6ae122b6af2a06a7b4bb80b60f9f285950a5..36216febb613969ef5e72b889de65db586ce949d 100644 (file)
--- a/shallow.c
+++ b/shallow.c
@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
                fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
 
                if (write_in_full(fd, sb.buf, sb.len) != sb.len ||
-                   close_tempfile(&temp) < 0)
+                   close_tempfile_gently(&temp) < 0)
                        die_errno("failed to write to %s",
                                  get_tempfile_path(&temp));
                strbuf_release(&sb);

diff --git a/tempfile.c b/tempfile.c
index 68437106703470f39a6079aaad7d3bb407ac3389..c6740e562cb95e960365a34a63930077103c59e4 100644 (file)
--- 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)
                return;
 
-       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);
 }

diff --git a/tempfile.h b/tempfile.h
index 2f0038decd5b6d00b55fa03ec8988a3810d1784f..d854dcdd3eb513c3198a7fb5c58a7a7d0b08a4d8 100644 (file)
--- a/tempfile.h
+++ b/tempfile.h
@@ -47,7 +47,7 @@
  *   control of the file.
  *
  * * Close the file descriptor without removing or renaming the
- *   temporary file by calling `close_tempfile()`, and later call
+ *   temporary file by calling `close_tempfile_gently()`, and later call
  *   `delete_tempfile()` or `rename_tempfile()`.
  *
  * Even after the temporary file is renamed or deleted, the `tempfile`
@@ -59,7 +59,7 @@
  * and remove the temporary file.
  *
  * If you need to close the file descriptor yourself, do so by calling
- * `close_tempfile()`. You should never call `close(2)` or `fclose(3)`
+ * `close_tempfile_gently()`. You should never call `close(2)` or `fclose(3)`
  * yourself, otherwise the `struct tempfile` structure would still
  * think that the file descriptor needs to be closed, and a later
  * cleanup would result in duplicate calls to `close(2)`. Worse yet,
@@ -74,9 +74,10 @@
  * `create_tempfile()` returns a file descriptor on success or -1 on
  * failure. On errors, `errno` describes the reason for failure.
  *
- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile()`
- * return 0 on success. On failure they set `errno` appropriately, do
- * their best to delete the temporary file, and return -1.
+ * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`
+ * return 0 on success. On failure they set `errno` appropriately and return
+ * -1. `delete` and `rename` (but not `close`) do their best to delete the
+ * temporary file before returning.
  */
 
 struct tempfile {
@@ -203,7 +204,7 @@ static inline int xmks_tempfile(struct tempfile *tempfile,
 /*
  * Associate a stdio stream with the temporary file (which must still
  * be open). Return `NULL` (*without* deleting the file) on error. The
- * stream is closed automatically when `close_tempfile()` is called or
+ * stream is closed automatically when `close_tempfile_gently()` is called or
  * when the file is deleted or renamed.
  */
 extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);
@@ -226,20 +227,20 @@ extern FILE *get_tempfile_fp(struct tempfile *tempfile);
  * If the temporary file is still open, close it (and the file pointer
  * too, if it has been opened using `fdopen_tempfile()`) without
  * deleting the file. Return 0 upon success. On failure to `close(2)`,
- * return a negative value and delete the file. Usually
- * `delete_tempfile()` or `rename_tempfile()` should eventually be
- * called if `close_tempfile()` succeeds.
+ * return a negative value. Usually `delete_tempfile()` or `rename_tempfile()`
+ * should eventually be called regardless of whether `close_tempfile_gently()`
+ * succeeds.
  */
-extern int close_tempfile(struct tempfile *tempfile);
+extern int close_tempfile_gently(struct tempfile *tempfile);
 
 /*
  * Re-open a temporary file that has been closed using
- * `close_tempfile()` but not yet deleted or renamed. This can be used
+ * `close_tempfile_gently()` but not yet deleted or renamed. This can be used
  * to implement a sequence of operations like the following:
  *
  * * Create temporary file.
  *
- * * Write new contents to file, then `close_tempfile()` to cause the
+ * * Write new contents to file, then `close_tempfile_gently()` to cause the
  *   contents to be written to disk.
  *
  * * Pass the name of the temporary file to another program to allow