From: Ronnie Sahlberg <sahlberg@google.com>
Date: Mon, 28 Apr 2014 15:11:48 GMT
Subject: Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message
Message-ID: <CAL=YDWnD74eZ8teXsY1RfWj9ZTvmNmhYa1+3SiDBo+gCRTqf7w@mail.gmail.com>
In-Reply-To: <20140425223620.GD9218@google.com>

Good ideas.
Applied, thanks.

On Fri, Apr 25, 2014 at 3:36 PM, Jonathan Nieder <jrnieder@gmail.com> 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