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 04:23:31PM -0800, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
> > We don't actually need the locking functionality, because we already
> > hold the lock on the reference itself, 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.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.
> 
> Should updates to the HEAD reflog acquire HEAD.lock?  (They don't
> currently.)
> 
> [...]
> > --- 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;
> 
> If this lockfile is only used in that one function, it can be declared
> inside the function.
> 
> If it is meant to be used throughout the 'git reflog' command, then it
> can go near the top of the file.
> 

After the series completes, this lock is only used in reflog_expire.
So I'd rather move it inside the function? Then we could run the reflog_expire
function in parallel for different locks in theory?

> > +
> >  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;
> 
> hold_lock_file_for_update doesn't print a message.  Code to print one
> looks like
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> 		unable_to_lock_message(log_file, errno, &err);
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> (A patch in flight changes that to
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> )
> 
> > +		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> > +		if (!cb.newlog)
> > +			goto failure;
> 
> Hm.  lockfile.c::fdopen_lock_file ought to use xfdopen to make this
> case impossible.  And xfdopen should use try_to_free_routine() and
> try again on failure.
> 
> [...]
> > @@ -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));
> 
> Style nit: error messages usually start with a lowercase letter
> (though I realize nearby examples are already inconsistent).
> 
> commit_lock_file() can take care of the close_lock_file automatically.
> 
> [...]
> > @@ -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);
> 
> Most callers say "unable to commit reflog '%s'", log_file to hedge their
> bets in case the close failed (which may be what you were avoiding
> above.
> 
> errno is meaningful when commit_lock_file fails, making a more
> detailed diagnosis from strerror(errno) possible.
> 
> Thanks,
> Jonathan