Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file

On Thu, Dec 04, 2014 at 06:51:36PM -0800, ronnie sahlberg wrote:

On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty [off-list ref]
wrote:

quoted

We don't actually need the locking functionality, because we already
hold the lock on the reference itself,

No. You do need the lock.
The ref is locked only during transaction_commit()

Michael is saying, that you never want to modify the reflog, if you're not holding
the lock on the corresponding ref. Or in other words, you may modify the reflog, if
the corresponding ref is held. This used to be this way back then. 

If you don't want to lock the reflog file and instead rely on the lock on
the ref itself you will need to
rework your patches so that the lock on the ref is taken already during,
for example, transaction_update_ref() instead.

But without doing those changes and moving the ref locking from _commit()
to _update_ref() you will risk reflog corruption/surprises
if two operations collide and both rewrite the reflog without any lock held.

quoted

which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.

For example:

* Correctly handle the case that the reflog lock file already exists
  for some reason or cannot be opened.

* Correctly clean up the lockfile if the program dies.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/reflog.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index a282e60..d344d45 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c

@@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname,

const unsigned char *sha1, int
        return 0;
 }

+static struct lock_file reflog_lock;
+
 static int expire_reflog(const char *refname, const unsigned char *sha1,
void *cb_data)
 {
        struct cmd_reflog_expire_cb *cmd = cb_data;
        struct expire_reflog_cb cb;
        struct ref_lock *lock;
-       char *log_file, *newlog_path = NULL;
+       char *log_file;
        struct commit *tip_commit;
        struct commit_list *tips;
        int status = 0;

@@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const

unsigned char *sha1, void *c
                unlock_ref(lock);
                return 0;
        }
+
        log_file = git_pathdup("logs/%s", refname);
        if (!cmd->dry_run) {
-               newlog_path = git_pathdup("logs/%s.lock", refname);
-               cb.newlog = fopen(newlog_path, "w");
+               if (hold_lock_file_for_update(&reflog_lock, log_file, 0) <
0)
+                       goto failure;
+               cb.newlog = fdopen_lock_file(&reflog_lock, "w");
+               if (!cb.newlog)
+                       goto failure;
        }

        cb.cmd = cmd;

@@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const

unsigned char *sha1, void *c
        }

        if (cb.newlog) {
-               if (fclose(cb.newlog)) {
-                       status |= error("%s: %s", strerror(errno),
-                                       newlog_path);
-                       unlink(newlog_path);
+               if (close_lock_file(&reflog_lock)) {
+                       status |= error("Couldn't write %s: %s", log_file,
+                                       strerror(errno));
                } else if (cmd->updateref &&
                        (write_in_full(lock->lock_fd,
                                sha1_to_hex(cb.last_kept_sha1), 40) != 40
||

@@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const

unsigned char *sha1, void *c
                         close_ref(lock) < 0)) {
                        status |= error("Couldn't write %s",
                                        lock->lk->filename.buf);
-                       unlink(newlog_path);
-               } else if (rename(newlog_path, log_file)) {
-                       status |= error("cannot rename %s to %s",
-                                       newlog_path, log_file);
-                       unlink(newlog_path);
+                       rollback_lock_file(&reflog_lock);
+               } else if (commit_lock_file(&reflog_lock)) {
+                       status |= error("cannot rename %s.lock to %s",
+                                       log_file, log_file);
                } else if (cmd->updateref && commit_ref(lock)) {
                        status |= error("Couldn't set %s", lock->ref_name);
-               } else {
-                       adjust_shared_perm(log_file);
                }
        }
-       free(newlog_path);
        free(log_file);
        unlock_ref(lock);
        return status;
+
+ failure:
+       rollback_lock_file(&reflog_lock);
+       free(log_file);
+       unlock_ref(lock);
+       return -1;
 }

 static int collect_reflog(const char *ref, const unsigned char *sha1, int
unused, void *cb_data)
--
2.1.3