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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 12/05/2014 01:23 AM, 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.

This reminder is important (and forgettable) enough that I will add a
comment within the function explaining it.

> Should updates to the HEAD reflog acquire HEAD.lock?  (They don't
> currently.)

Yes, they should; good catch. I assume that you are referring to the
code at the bottom of write_ref_sha1()? Or did you find a problem in
this patch series?

If the former, then I propose that we address this bug in a separate
patch series.

> [...]
>> --- 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.

For now it is only used within this function, so I will move it into the
function as you suggest. (As you know, it does need to remain static,
because of the way the lock_file module takes over ownership of these
objects.)

>> +
>>  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;
> 	}

Thanks; will add.

> (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;
> 	}
> 
> )

Thanks for the heads-up. The compiler will complain when the branches
are merged, and hopefully the fix will be obvious.

>> +		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.

That sounds reasonable, but it is not manifestly obvious given that at
least one caller of fdopen_lock_file() (in fast-import.c) tries to
recover if fdopen_lock_file() fails. Let's address this in a separate
patch series if that is OK with you. For now I will add explicit
error-reporting code here before "goto 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).

Thanks; will fix.

> commit_lock_file() can take care of the close_lock_file automatically.

The existing code is a tiny bit safer: first make sure both files can be
written, *then* rename each of them into place. If either write fails,
then both files will get rolled back. But if we switch to using
commit_lock_file(), then a failure when writing the reference would
leave the reflog updated but the reference rolled back.

> [...]
>> @@ -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.

I will improve the error message.

Thanks for your detailed review!
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu