Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Good points.



On Fri, Apr 25, 2014 at 3:10 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Ronnie Sahlberg wrote:
>
>> Let ref_transaction_commit return an optional error string that describes
>> the transaction failure.  Start by returning the same error as update_ref_lock
>> returns, modulo the initial error:/fatal: preamble.
>
> s/returns/prints/?

Done, and then was deleted when I reworded the message.

>
>> This will make it easier for callers to craft better error messages when
>> a transaction call fails.
>
> Interesting.  Can you give an example?  What kind of behavior are we
> expecting in callers other than die()-ing or cleaning up and then
> die()-ing?

I was thinking a bit too far ahead. You could in theory keep logging multiple
lock failures during the _commit() and then when the transaction fails
and returns
it will have appended a list of all refs that failed and not just the
first ref that failed.


>
> I like this more than having the caller pass in a flag/callback/etc to
> decide how noisy to be and whether to gracefully accept errors or exit.
> So it seems like an improvement, but may always returning error()
> would be better --- more context would help in clarifying this.
>
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction *transaction,
>>   * Commit all of the changes that have been queued in transaction, as
>>   * atomically as possible.  Return a nonzero value if there is a
>>   * problem.  The ref_transaction is freed by this function.
>> + * If error is non-NULL it will return an error string that describes
>> + * why a commit failed. This string must be free()ed by the caller.
>>   */
>>  int ref_transaction_commit(struct ref_transaction *transaction,
>> -                        const char *msg, enum action_on_err onerr);
>> +                        const char *msg, char **err,
>> +                        enum action_on_err onerr);
>
> Is the idea that if I pass in a pointer &err then
> ref_transaction_commit will take the action described by onerr *and*
> write its error message to err?

Temporarily, yes.
Shortly after this patch I remove the onerr argument completely.
But I want to keep the "pass error back to caller" and "get rid of
onerr" as two separate patches.
I think it is easier to follow the flow of changes if they are done in
two separate steps.

>
> Probably squashing with patch 07 would make this easier to read (and
> wouldn't require changing any messages at that point).

See above.

>
> [...]
>> --- a/refs.c
>> +++ b/refs.c
> [...]
>> @@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                              update->flags,
>>                                              &update->type, onerr);
>>               if (!update->lock) {
>> +                     if (err) {
>> +                             const char *str = "Cannot lock the ref '%s'.";
>> +                             *err = xmalloc(PATH_MAX + 24);
>> +                             snprintf(*err, PATH_MAX + 24, str,
>> +                                      update->refname);
>> +                     }
>
> Might be clearer to use a helper similar to path.c::mkpathdup
>
>         char *aprintf(const char *fmt, ...)
>         {
>                 char *result;
>                 struct strbuf sb = STRBUF_INIT;
>                 va_list args;
>
>                 va_start(args, fmt);
>                 strbuf_vaddf(&sb, fmt, args);
>                 va_end(args);
>
>                 return strbuf_detach(&sb);
>         }
>
> or to have the caller pass in a pointer to strbuf instead of char *.

strbuf as argument is probably the right thing to do. I am doing that change.

>
> The rest looks good to me.
>
> 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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]