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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 29, 2014, 17:41 UTC
Message-ID: <20140529174106.GE12314@google.com>
In-Reply-To: <CAL=YDW=WmNObkTO_uybTToeMKGGQf5NC0oFvy_pMrsg+ehpzog@mail.gmail.com>

Ronnie Sahlberg wrote:

> On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 9 quoted lines

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

[...]

Show 13 quoted lines

>> 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