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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html