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

On Fri, May 23, 2014 at 2:02 PM, Michael Haggerty [off-list ref] wrote:

On 05/23/2014 06:14 PM, Ronnie Sahlberg wrote:

quoted

On Fri, May 23, 2014 at 6:49 AM, Michael Haggerty [off-list ref] wrote:

quoted

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

We can add this later once we get the basic transaction stuff in.
It will make it easier to experiment with different types of error
reporting then.

Michael

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