Re: [PATCH v2 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:00:43
On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:
Change fast_forward_to() to check if locking the ref failed, print a nice error message and bail out early. The old code did not check if ref_lock was NULL and relied on the fact that the write_ref_sha1() would safely detect this condition and set the
s/the write_ref_sha1()/write_ref_sha1()/
return variable ret to indicate an error. While that is safe, it makes the code harder to read for two reasons: * Inconsistency. Almost all other places we do check the lock for NULL explicitely, so the naive reader is confused "why don't we check here".
s/explicitely/explicitly/ s/here"/here?"/
* And relying on write_ref_sha1() to detect and return an error for when a previous lock_any_ref_for_update() feels obfuscated.
s/feels/failed feels/ maybe?
This change should not change any functionality or logic aside from adding an extra error message when this condition is triggered. (write_ref_sha1() returns an error silently for this condition)
You need a period inside the parentheses.
quoted hunk ↗ jump to hunk
Signed-off-by: Ronnie Sahlberg <redacted> --- sequencer.c | 4 ++++ 1 file changed, 4 insertions(+)diff --git a/sequencer.c b/sequencer.c index bde5f04..0a80c58 100644 --- a/sequencer.c +++ b/sequencer.c@@ -281,8 +281,12 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, exit(1); /* the callee should have complained already */ ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from, 0, NULL); + if (!ref_lock) + return error(_("Failed to lock HEAD during fast_forward_to"));
This error message can be emitted to the user in the normal course of things (i.e., it is not a bug). So the message should make sense to the user. Is "fast_forward_to" a user-facing term that the user will understand? I suspect that you took it from the name of the function, which is *not* meaningful to a user. But unfortunately I'm not familiar enough with the sequencer to be able to suggest a better error message.
+ strbuf_addf(&sb, "%s: fast-forward", action_name(opts)); ret = write_ref_sha1(ref_lock, to, sb.buf); + strbuf_release(&sb); return ret; }
Michael -- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/