From 010845157ae748bc35af84cfedf0803379c6e02b Mon Sep 17 00:00:00 2001 From: Martin Ågren Date: Wed, 9 May 2018 22:55:36 +0200 Subject: refs.c: do not die if locking fails in `write_pseudoref()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we could not take the lock, we add an error to the `strbuf err` and return. However, this code is dead. The reason is that we take the lock using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle error-handling to actually kick in. We could instead just drop the dead code and die here. But everything is prepared for gently propagating the error, so let's do that instead. There is similar dead code in `delete_pseudoref()`, but let's save that for the next patch. While at it, make the lock non-static. (Placing `struct lock_file`s on the stack used to be a bad idea, because the temp- and lockfile-machinery would keep a pointer into the struct. But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can safely have lockfiles on the stack.) Reviewed-by: Stefan Beller Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- refs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 20ba82b434..6ad1efb600 100644 --- a/refs.c +++ b/refs.c @@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid, { const char *filename; int fd; - static struct lock_file lock; + struct lock_file lock = LOCK_INIT; struct strbuf buf = STRBUF_INIT; int ret = -1; @@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid, strbuf_addf(&buf, "%s\n", oid_to_hex(oid)); filename = git_path("%s", pseudoref); - fd = hold_lock_file_for_update_timeout(&lock, filename, - LOCK_DIE_ON_ERROR, + fd = hold_lock_file_for_update_timeout(&lock, filename, 0, get_files_ref_lock_timeout_ms()); if (fd < 0) { strbuf_addf(err, "could not open '%s' for writing: %s", -- cgit v1.3 From 3c6fad4a3fcc9a01dd3d9678360907271ad85920 Mon Sep 17 00:00:00 2001 From: Martin Ågren Date: Wed, 9 May 2018 22:55:37 +0200 Subject: refs.c: do not die if locking fails in `delete_pseudoref()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After taking the lock we check whether we got it and die otherwise. But since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have died. Considering the choice between dropping the dead code and dropping the flag, let's go for option number three: Drop the flag, write an error instead of dying, then return -1. This function already returns -1 for another error, so the caller (or rather, its callers) should be able to handle this. There is some inconsistency around how we handle errors in this function and elsewhere in this file, but let's take this small step towards gentle error-reporting now and leave the rest for another time. While at it, make the lock non-static and reduce its scope. (Placing `struct lock_file`s on the stack used to be a bad idea, because the temp- and lockfile-machinery would keep a pointer into the struct. But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can safely have lockfiles on the stack.) Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- refs.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 6ad1efb600..f657f4de84 100644 --- a/refs.c +++ b/refs.c @@ -689,20 +689,23 @@ done: static int delete_pseudoref(const char *pseudoref, const struct object_id *old_oid) { - static struct lock_file lock; const char *filename; filename = git_path("%s", pseudoref); if (old_oid && !is_null_oid(old_oid)) { + struct lock_file lock = LOCK_INIT; int fd; struct object_id actual_old_oid; fd = hold_lock_file_for_update_timeout( - &lock, filename, LOCK_DIE_ON_ERROR, + &lock, filename, 0, get_files_ref_lock_timeout_ms()); - if (fd < 0) - die_errno(_("Could not open '%s' for writing"), filename); + if (fd < 0) { + error_errno(_("could not open '%s' for writing"), + filename); + return -1; + } if (read_ref(pseudoref, &actual_old_oid)) die("could not read ref '%s'", pseudoref); if (oidcmp(&actual_old_oid, old_oid)) { -- cgit v1.3