Re: [PATCH v20 20/48] refs.c: change ref_transaction_create to do error checking and return status

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

I'm in my next attempt to get through your patch series.  Sorry for the
long hiatus.

Patches 1-19 look OK aside from a minor typo that I just reported.

See below for a comment on this patch.

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Do basic error checking in ref_transaction_create() and make it return
> non-zero on error. Update all callers to check the result of
> ref_transaction_create(). There are currently no conditions in _create that
> will return error but there will be in the future. Add an err argument that
> will be updated on failure.
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/update-ref.c |  4 +++-
>  refs.c               | 18 +++++++++++------
>  refs.h               | 55 +++++++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 63 insertions(+), 14 deletions(-)
> 
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 3067b11..41121fa 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
>  	if (*next != line_termination)
>  		die("create %s: extra input: %s", refname, next);
>  
> -	ref_transaction_create(transaction, refname, new_sha1, update_flags);
> +	if (ref_transaction_create(transaction, refname, new_sha1,
> +				   update_flags, &err))
> +		die("%s", err.buf);
>  
>  	update_flags = 0;
>  	free(refname);
> diff --git a/refs.c b/refs.c
> index 3f05e88..c49f1c6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3449,18 +3449,24 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  	return 0;
>  }
>  
> -void ref_transaction_create(struct ref_transaction *transaction,
> -			    const char *refname,
> -			    const unsigned char *new_sha1,
> -			    int flags)
> +int ref_transaction_create(struct ref_transaction *transaction,
> +			   const char *refname,
> +			   const unsigned char *new_sha1,
> +			   int flags,
> +			   struct strbuf *err)
>  {
> -	struct ref_update *update = add_update(transaction, refname);
> +	struct ref_update *update;
> +
> +	if (!new_sha1 || is_null_sha1(new_sha1))
> +		die("BUG: create ref with null new_sha1");
> +
> +	update = add_update(transaction, refname);
>  
> -	assert(!is_null_sha1(new_sha1));
>  	hashcpy(update->new_sha1, new_sha1);
>  	hashclr(update->old_sha1);
>  	update->flags = flags;
>  	update->have_old = 1;
> +	return 0;
>  }
>  
>  void ref_transaction_delete(struct ref_transaction *transaction,
> diff --git a/refs.h b/refs.h
> index c5376ce..33b4383 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -10,6 +10,45 @@ struct ref_lock {
>  	int force_write;
>  };
>  
> +/*
> + * A ref_transaction represents a collection of ref updates
> + * that should succeed or fail together.
> + *
> + * Calling sequence
> + * ----------------
> + * - Allocate and initialize a `struct ref_transaction` by calling
> + *   `ref_transaction_begin()`.
> + *
> + * - List intended ref updates by calling functions like
> + *   `ref_transaction_update()` and `ref_transaction_create()`.
> + *
> + * - Call `ref_transaction_commit()` to execute the transaction.
> + *   If this succeeds, the ref updates will have taken place and
> + *   the transaction cannot be rolled back.
> + *
> + * - At any time call `ref_transaction_free()` to discard the
> + *   transaction and free associated resources.  In particular,
> + *   this rolls back the transaction if it has not been
> + *   successfully committed.
> + *
> + * Error handling
> + * --------------
> + *
> + * On error, transaction functions append a message about what
> + * went wrong to the 'err' argument.  The message mentions what
> + * ref was being updated (if any) when the error occurred so it
> + * can be passed to 'die' or 'error' as-is.
> + *
> + * The message is appended to err without first clearing err.
> + * This allows the caller to prepare preamble text to the generated
> + * error message:
> + *
> + *     strbuf_addf(&err, "Error while doing foo-bar: ");
> + *     if (ref_transaction_update(..., &err)) {
> + *         ret = error("%s", err.buf);
> + *         goto cleanup;
> + *     }
> + */

I don't have a problem with the API, but I think the idiom suggested in
the comment above is a bit silly.  Surely one would do the following
instead:

    if (ref_transaction_update(..., &err)) {
        ret = error("Error while doing foo-bar: %s", err.buf);
        goto cleanup;
    }

I think it would also be helpful to document whether the error string
that is appended to the strbuf is terminated with a LF.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu