From 83a3069a3895de81fea720ffa6a3e47f9400fe04 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:33 -0400 Subject: lockfile: do not rollback lock on failed close Since the lockfile code is based on the tempfile code, it has some of the same problems, including that close_lock_file() erases the tempfile's filename buf, making it hard for the caller to write a good error message. In practice this comes up less for lockfiles than for straight tempfiles, since we usually just report the refname. But there is at least one buggy case in write_ref_to_lockfile(). Besides, given the coupling between the lockfile and tempfile modules, it's less confusing if their close() functions have the same semantics. Just as the previous commit did for close_tempfile(), let's teach close_lock_file() and its wrapper close_ref() not to rollback on error. And just as before, we'll give them new "gently" names to catch any new callers that are added. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 13 +++++++------ refs/packed-backend.c | 3 ++- 2 files changed, 9 insertions(+), 7 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index fccbc24ac4..bc1899cd4a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1402,9 +1402,9 @@ static int files_rename_ref(struct ref_store *ref_store, return ret; } -static int close_ref(struct ref_lock *lock) +static int close_ref_gently(struct ref_lock *lock) { - if (close_lock_file(lock->lk)) + if (close_lock_file_gently(lock->lk)) return -1; return 0; } @@ -1630,7 +1630,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock, fd = get_lock_file_fd(lock->lk); if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || write_in_full(fd, &term, 1) != 1 || - close_ref(lock) < 0) { + close_ref_gently(lock) < 0) { strbuf_addf(err, "couldn't write '%s'", get_lock_file_path(lock->lk)); unlock_ref(lock); @@ -2372,7 +2372,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, * the lockfile is still open. Close it to * free up the file descriptor: */ - if (close_ref(lock)) { + if (close_ref_gently(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); return TRANSACTION_GENERIC_ERROR; @@ -2848,14 +2848,15 @@ static int files_reflog_expire(struct ref_store *ref_store, !(type & REF_ISSYMREF) && !is_null_oid(&cb.last_kept_oid); - if (close_lock_file(&reflog_lock)) { + if (close_lock_file_gently(&reflog_lock)) { status |= error("couldn't write %s: %s", log_file, strerror(errno)); + rollback_lock_file(&reflog_lock); } else if (update && (write_in_full(get_lock_file_fd(lock->lk), oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 || - close_ref(lock) < 0)) { + close_ref_gently(lock) < 0)) { status |= error("couldn't write %s", get_lock_file_path(lock->lk)); rollback_lock_file(&reflog_lock); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 412c85034f..dca887c0fe 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -545,8 +545,9 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err) return -1; } - if (close_lock_file(&refs->lock)) { + if (close_lock_file_gently(&refs->lock)) { strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno)); + rollback_lock_file(&refs->lock); return -1; } -- cgit v1.2.3