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

[Stefan Beller <sbeller@google.com>]

On Thu, Dec 04, 2014 at 06:51:36PM -0800, ronnie sahlberg wrote:
> On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty <mhagger@alum.mit.edu>
> wrote:
> 
> > 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.
> 
> 
> 
> 
> 
> > 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 <mhagger@alum.mit.edu>
> > ---
> >  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
> >
> >