Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags

> Ronnie Sahlberg wrote:
>> On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder <jrnieder@gmail.com> 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