Re: [PATCH v2 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to

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

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.

> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  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/