From: Ronnie Sahlberg <sahlberg@google.com>
Date: Thu, 29 May 2014 16:10:41 GMT
Subject: Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags
Message-ID: <CAL=YDW=WmNObkTO_uybTToeMKGGQf5NC0oFvy_pMrsg+ehpzog@mail.gmail.com>
In-Reply-To: <20140528233940.GC12314@google.com>

On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> I rely on the fact that if the transaction has failed then it is safe
>> to call ref_transaction_commit since it is guaranteed to return an
>> error too.
>
> Yes, I am saying that behavior for ref_transaction_commit is weird.
>
> 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.
>
> That's true even if there's a bug elsewhere in code I didn't write
> (e.g., someone forgot to check the return value from
> ref_transaction_update).
>
> 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.


>
> Sure, if I am well acquainted with the API, I can make sure to use the
> same strbuf for all transaction API calls.  But that would result in
> strange behavior, too: if multiple _update calls fail, then I get
> concatenated messages.
>
> Okay, I can make sure to do at most one failing _update, before
> calling _commit and printing the error.  But at that point, what is
> the advantage over normal exception handling, where the error gets
> handled at the _update call site?
>
> Jonathan