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


> * 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 <sahlb...@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 Haggerty
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to