Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs

[Michael Haggerty <mhagger@alum.mit.edu>]

On 05/23/2014 06:14 PM, Ronnie Sahlberg wrote:
> On Fri, May 23, 2014 at 6:49 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> When I combine these two lines of thought, it suggests to me that we
>> could do a better job of supporting *both* use cases.  What if the
>> transaction object contained not an err strbuf but a string_list?  If an
>> error occurs while building up the transaction, a message would be added
>> to the string list and the function would return an error status.  The
>> caller can monitor errors while it is building up the transaction and
>> abort immediately if it wants, or it can ignore the return values and
>> let the error messages accumulate in the string list.  When the caller
>> attempts the commit, it would notice that the transaction failed, and at
>> that time the caller could emit *all* of the accumulated error messages
>> by reading them out of the string list; e.g.,
>>
>>     Error fetching from $REMOTE:   <- this is generated by caller
>>         $ERR[0]    <- these come from the error string list,
>>         $ERR[1]       printed with indentation by caller
>>         $ERR[2]
>>         $ERR[3]
>>
>> This style would have another advantage: we might have some back ends
>> for which transactions have a high overhead.  Such a back end would
>> probably choose not to do any checks while the transaction is being
>> built up, e.g., to avoid a round-trip to a database.  When commit() is
>> called, it would learn about all of the errors at once.  (1) It would
>> need a way to return all of the errors to the caller.  (2) It would be
>> nice for the caller to be able to treat such a back end the same as it
>> treats a back end that is able to report errors immediately.  It seems
>> to me that having a way to report multiple errors at the same time would
>> solve both problems nicely.
> 
> Inretesting.
> That would mean changing all functions to take a string_list provided
> by the caller instead of a strbuf.
> And then have _update/_create/_delete do actual work instead of
> bailing out after the first error.
> 
> Users that want to check for error and log after each call to
> _update/_create/_delete could do so and
> just use the last entry added to the string list or otherwise they
> could just wait until _commit time and if it fails log
> all the strings.

I still think we should consider storing the err string_list within the
transaction object; otherwise it's annoying to have to pass that
parameter around everywhere.  And if there were also a policy that any
caller that checks and reports any error should report *all* of the
accumulated errors and abort the transaction, then we don't have to
worry about error messages being output multiple times or zero times.

It might be nice to have a printf-style helper function like

    ref_transaction_perror(transaction, fmt, ...)

(or maybe ref_transaction_die()) that outputs the accumulated errors
with msg as a header (like my example output above), to make error
handling easy and uniform.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/