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

On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

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:

Done.

[...]

quoted

+++ 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

I changed it to :
   die("cannot create ref '%s'", refname);

But it would still mean you would have
         error: some bad thing
         fatal: cannot create 'refs/heads/master'


To make it better we have to wait until the end of the second patch
series, ref-transactions-next
where we will have an err argument to _update/_create/_delete too and
thus we can do this from update-ref.c :

   if (transaction_create_sha1(transaction, refname, new_sha1,
       update_flags, msg, &err))
   die("%s", err.buf);

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

[...]

quoted

@@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction *transaction,

[...]

quoted

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