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

On 12/05/2014 01:23 AM, Jonathan Nieder wrote:

Michael Haggerty wrote:

quoted

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.

[...]

quoted

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

quoted

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

quoted

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

[...]

quoted

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

[...]

quoted

@@ -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