Subject: Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

From: Jonathan Nieder <[email-masked]>
Date: Wed, 14 May 2014 17:04:09 -0700

Ronnie Sahlberg wrote:

> Do basic error checking in ref_transaction_create() and make it return
> status. 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.

Same concerns as with _update:

[...]
> +++ b/builtin/update-ref.c
> @@ -225,7 +225,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))
> +		die("failed transaction create for %s", refname);

If it were ever triggered, the message

	error: some bad thing
	fatal: failed transaction create for refs/heads/master

looks overly verbose and unclear.  Something like

	fatal: cannot create ref refs/heads/master: some bad thing

might work better.  It's hard to tell without an example in mind.

[...]
> @@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction *transaction,
[...]
> -	assert(!is_null_sha1(new_sha1));
> +	if (!new_sha1 || is_null_sha1(new_sha1))
> +		die("create ref with null new_sha1");

One less 'assert' is nice. :)

As with _update, the message should start with "BUG:" to make it clear
to users and translators that this should never happen, even with
malformed user input.  That gets corrected in patch 28 but it's
clearer to include it from the start.