Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 25, 2014, 21:32 UTC
Message-ID: <20140425213210.GN15516@google.com>
In-Reply-To: <1397763987-4453-3-git-send-email-sahlberg@google.com>

Hi,

Ronnie Sahlberg wrote:

> Update ref_transaction_update() do some basic error checking and return
> true on error. Update all callers to check ref_transaction_update() for error.

Micronit: nonzero, not true.  (true tends to mean '1' while here we
have the usual error return of -1.  It's kind of annoying that C
doesn't have a nice way to distinguish between the usual int return of
0 for success and the usual bool return of true for success.)

Looks like a good change.  Some tiny nitpicks below.

[...]

Show 8 quoted lines

> --- a/refs.h
> +++ b/refs.h
> @@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
>   * that the reference should have had before the update, or zeros if
>   * it must not have existed beforehand.
>   */
> -void ref_transaction_update(struct ref_transaction *transaction,
> +int ref_transaction_update(struct ref_transaction *transaction,

The comment above the prototype doesn't tell me:

When should the caller expect ref_transaction_update to return an error? What does an error mean: is it always a sign of a bug in the caller, or can it be due to some other problem? What guarantees does the caller have about the state after an error --- is it just "Things will be in a sane state so you can free resources and exit", or will the ref_transaction_update() have been essentially a no-op allowing the caller to continue?

[...]

Show 18 quoted lines

> --- a/refs.c
> +++ b/refs.c
> @@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
>  	return update;
>  }
>  
> -void ref_transaction_update(struct ref_transaction *transaction,
> +int ref_transaction_update(struct ref_transaction *transaction,
>  			    const char *refname,
>  			    const unsigned char *new_sha1,
>  			    const unsigned char *old_sha1,
>  			    int flags, int have_old)
>  {
> -	struct ref_update *update = add_update(transaction, refname);
> +	struct ref_update *update;
> +
> +	if (have_old && !old_sha1)
> +		return error("have_old is true but old_sha1 is NULL");

I agree with Michael that the error message should start with "BUG:" so humans encountering this know to contact the list instead of blaming themselves.

Returning error instead of die-ing seems like a nice thing that make it easier to make git valgrind-clean some day. Others might disagree with me about whether that's worthwhile, but I think it's a good change. :)

[...]

Show 11 quoted lines

> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
>  	if (*next != line_termination)
>  		die("update %s: extra input: %s", refname, next);
>  
> -	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> -			       update_flags, have_old);
> +	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +				   update_flags, have_old))
> +		die("failed transaction update for %s", refname);

ref_transaction_update already printed an error, but of course that's no guarantee that bugs in ref_transaction_update will not cause it to fail without printing a message in the future. And the extra context for the error might be nice (but why not print refname in the message from ref_transaction_update instead?).

Is the plan for ref_transaction_update to be able to fail for other reasons some day? What is the contract --- do we need a human-readable, translatable message here, or is a "this can't happen" BUG message fine?

I'd be fine with

		die("BUG: failed transa...

or

		/* ref_transaction_update already printed a message */
		exit(128)

with a slight preference for the former, for what it's worth.

[...]

Show 9 quoted lines

> @@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
>  	if (*next != line_termination)
>  		die("verify %s: extra input: %s", refname, next);
>  
> -	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> -			       update_flags, have_old);
> +	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +			       update_flags, have_old))
> +		die("failed transaction update for %s", refname);

Likewise.

Thanks, Jonathan