lockfile: do not rollback lock on failed close

Since the lockfile code is based on the tempfile code, it
has some of the same problems, including that close_lock_file()
erases the tempfile's filename buf, making it hard for the
caller to write a good error message.

In practice this comes up less for lockfiles than for
straight tempfiles, since we usually just report the
refname. But there is at least one buggy case in
write_ref_to_lockfile(). Besides, given the coupling between
the lockfile and tempfile modules, it's less confusing if
their close() functions have the same semantics.

Just as the previous commit did for close_tempfile(), let's
teach close_lock_file() and its wrapper close_ref() not to
rollback on error. And just as before, we'll give them new
"gently" names to catch any new callers that are added.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/lockfile.h b/lockfile.h
index dd4259bc402b4d573895a8e0ce37f53a0db04531..c45d2db324d3549e91fcd80e2159a80dbb6f472a 100644 (file)
--- a/lockfile.h
+++ b/lockfile.h
@@ -69,7 +69,7 @@
  *   `rollback_lock_file()`.
  *
  * * Close the file descriptor without removing or renaming the
- *   lockfile by calling `close_lock_file()`, and later call
+ *   lockfile by calling `close_lock_file_gently()`, and later call
  *   `commit_lock_file()`, `commit_lock_file_to()`,
  *   `rollback_lock_file()`, or `reopen_lock_file()`.
  *
@@ -85,7 +85,7 @@
  *
  * If you need to close the file descriptor you obtained from a
  * `hold_lock_file_for_*()` function yourself, do so by calling
- * `close_lock_file()`. See "tempfile.h" for more information.
+ * `close_lock_file_gently()`. See "tempfile.h" for more information.
  *
  *
  * Under the covers, a lockfile is just a tempfile with a few helper
@@ -104,8 +104,8 @@
  *
  * Similarly, `commit_lock_file`, `commit_lock_file_to`, and
  * `close_lock_file` return 0 on success. On failure they set `errno`
- * appropriately, do their best to roll back the lockfile, and return
- * -1.
+ * appropriately and return -1. The `commit` variants (but not `close`)
+ * do their best to delete the temporary file before returning.
  */
 
 #include "tempfile.h"
@@ -202,8 +202,9 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);
 /*
  * Associate a stdio stream with the lockfile (which must still be
  * open). Return `NULL` (*without* rolling back the lockfile) on
- * error. The stream is closed automatically when `close_lock_file()`
- * is called or when the file is committed or rolled back.
+ * error. The stream is closed automatically when
+ * `close_lock_file_gently()` is called or when the file is committed or
+ * rolled back.
  */
 static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
 {
@@ -241,28 +242,21 @@ extern char *get_locked_file_path(struct lock_file *lk);
  * lockfile over the file being locked. Return 0 upon success. On
  * failure to `close(2)`, return a negative value and roll back the
  * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,
- * or `rollback_lock_file()` should eventually be called if
- * `close_lock_file()` succeeds.
+ * or `rollback_lock_file()` should eventually be called.
  */
-static inline int close_lock_file(struct lock_file *lk)
+static inline int close_lock_file_gently(struct lock_file *lk)
 {
-       int ret = close_tempfile_gently(&lk->tempfile);
-       if (ret) {
-               int saved_errno = errno;
-               delete_tempfile(&lk->tempfile);
-               errno = saved_errno;
-       }
-       return ret;
+       return close_tempfile_gently(&lk->tempfile);
 }
 
 /*
- * Re-open a lockfile that has been closed using `close_lock_file()`
+ * Re-open a lockfile that has been closed using `close_lock_file_gently()`
  * but not yet committed or rolled back. This can be used to implement
  * a sequence of operations like the following:
  *
  * * Lock file.
  *
- * * Write new contents to lockfile, then `close_lock_file()` to
+ * * Write new contents to lockfile, then `close_lock_file_gently()` to
  *   cause the contents to be written to disk.
  *
  * * Pass the name of the lockfile to another program to allow it (and

diff --git a/read-cache.c b/read-cache.c
index 51686518e061b8fe60ddac620815412ba6724efa..c3be65f8b097263f1e0e53f95510b8518ef755c1 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -2345,7 +2345,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
        if (flags & COMMIT_LOCK)
                return commit_locked_index(lock);
        else if (flags & CLOSE_LOCK)
-               return close_lock_file(lock);
+               return close_lock_file_gently(lock);
        else
                return ret;
 }

diff --git a/refs/files-backend.c b/refs/files-backend.c
index fccbc24ac4ad3631b96a4796f4b1f89913fa0ea3..bc1899cd4a3976c15c520a4734a7284431fb8829 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1402,9 +1402,9 @@ static int files_rename_ref(struct ref_store *ref_store,
        return ret;
 }
 
-static int close_ref(struct ref_lock *lock)
+static int close_ref_gently(struct ref_lock *lock)
 {
-       if (close_lock_file(lock->lk))
+       if (close_lock_file_gently(lock->lk))
                return -1;
        return 0;
 }
@@ -1630,7 +1630,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
        fd = get_lock_file_fd(lock->lk);
        if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
            write_in_full(fd, &term, 1) != 1 ||
-           close_ref(lock) < 0) {
+           close_ref_gently(lock) < 0) {
                strbuf_addf(err,
                            "couldn't write '%s'", get_lock_file_path(lock->lk));
                unlock_ref(lock);
@@ -2372,7 +2372,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
                 * the lockfile is still open. Close it to
                 * free up the file descriptor:
                 */
-               if (close_ref(lock)) {
+               if (close_ref_gently(lock)) {
                        strbuf_addf(err, "couldn't close '%s.lock'",
                                    update->refname);
                        return TRANSACTION_GENERIC_ERROR;
@@ -2848,14 +2848,15 @@ static int files_reflog_expire(struct ref_store *ref_store,
                        !(type & REF_ISSYMREF) &&
                        !is_null_oid(&cb.last_kept_oid);
 
-               if (close_lock_file(&reflog_lock)) {
+               if (close_lock_file_gently(&reflog_lock)) {
                        status |= error("couldn't write %s: %s", log_file,
                                        strerror(errno));
+                       rollback_lock_file(&reflog_lock);
                } else if (update &&
                           (write_in_full(get_lock_file_fd(lock->lk),
                                oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
                            write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
-                           close_ref(lock) < 0)) {
+                           close_ref_gently(lock) < 0)) {
                        status |= error("couldn't write %s",
                                        get_lock_file_path(lock->lk));
                        rollback_lock_file(&reflog_lock);

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 412c85034fc3b8632f16d13c4d467ad41060ddeb..dca887c0fe2ebfac9d5a7f78a4ecf81d94308e36 100644 (file)
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -545,8 +545,9 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
                return -1;
        }
 
-       if (close_lock_file(&refs->lock)) {
+       if (close_lock_file_gently(&refs->lock)) {
                strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno));
+               rollback_lock_file(&refs->lock);
                return -1;
        }