Thanks.
I have changed the transaction functions to die(BUG:) if the user
tries to call _update/_create/_delete on a failed transaction.
On Thu, May 29, 2014 at 10:41 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Ronnie Sahlberg wrote:
>> On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>
>>> Usually when ref_transaction_commit is called I can do
>>>
>>> struct strbuf err = STRBUF_INIT;
>>> if (ref_transaction_commit(..., &err))
>>> die("%s", err.buf);
>>>
>>> and I know that since ref_transaction_commit has returned a nonzero
>>> result, err.buf is populated with a sensible message that will
>>> describe what went wrong.
> [...]
>>> But the guarantee you are describing removes that property. It
>>> creates a case where ref_transaction_commit can return nonzero without
>>> updating err. So I get the following message:
>>>
>>> fatal:
>>>
>>> I don't think that's a good outcome.
>>
>> In this case "fatal:" can not happen.
>> This is no more subtle than most of the git core.
>>
>> I have changed this function to explicitly abort on _update failing
>> but I think this is making the api too restrictive.
>
> I don't want to push you toward making a change you think is wrong. I
> certainly don't own the codebase, and there are lots of other people
> (e.g., Michael, Junio, Jeff) to get advice from. So I guess I should
> try to address this.
>
> I'm not quite sure what you mean by too restrictive.
>
> a. Having API constraints that aren't enforced by the function makes
> using the API too fussy.
>
> I agree with that. That was something I liked about keeping track
> of the OPEN/CLOSED state of a transaction, which would let
> functions like _commit die() if someone is misusing the API so the
> problem gets detected early.
>
> b. Having to check the return value from _update() is too fussy.
>
> It certainly seems *possible* to have an API that doesn't require
> checking the return value, while still avoiding the usability
> problem I described in the quoted message above. For example:
>
> * _update() returns void and has no strbuf parameter
> * error handling happens by checking the error from _commit()
>
> That would score well on the scale described at
> http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
>
> An API where checking the return value is optional would be
> doable, too. For example:
>
> * _update() returns int and has a strbuf parameter
> * if the strbuf parameter is NULL, the caller is expected to
> wait for _commit() to check for errors, and a relevant
> message will be passed back then
> * if the strbuf parameter is non-NULL, then calling _commit()
> after an error is an API violation
>
> I don't understand the comment about no more subtle than most of git.
> Are you talking about the errno action at a distance you found in some
> functions? I thought we agreed that those were mistakes that accrue
> when people aim for a quick fix without thinking about maintainability
> and something git should have less of.
>
> 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