Good ideas.
Applied, thanks.
On Fri, Apr 25, 2014 at 3:36 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Ronnie Sahlberg wrote:
>
>> Call ref_transaction_commit with QUIET_ON_ERR and use the error string
>> that is returned to print a better log message if/after the transaction
>> fails.
>
> Ah, so that's how the transition to a better API happens. Makes sense.
>
> (A mention of QUIET_ON_ERR in the patch that introduces the &err
> parameter could help, or feel free to ignore these comments, since it's
> all well by the end of the series.)
>
>> Update the tests to reflect that the log message is now slightly different
>> fatal: update_ref failed: Cannot lock the ref 'some ref'
>> versus from the previous
>> fatal: Cannot lock the ref 'some ref'
>
> Makes sense as a demo of what the new code allows, but is this error
> message better? The use of 'git update-ref' is an implementation
> detail that the user doesn't need to know about.
>
> I think I'd prefer the result of plain die("%s", err), even though
> that's a no-op.
>
> [...]
>> +++ b/builtin/update-ref.c
> [...]
>> @@ -359,17 +360,18 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>> die("Refusing to perform update with empty message.");
>>
>> if (read_stdin) {
>> - int ret;
>> transaction = ref_transaction_begin();
>> -
>> + if (!transaction)
>> + die("failed to update refs");
>
> This can't happen (xcalloc is defined to die() on malloc failure).
> If you want to protect against it anyway, die("BUG: ...")?
>
> Thanks,
> Jonathan
--
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