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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 28, 2014, 23:39 UTC
Message-ID: <20140528233940.GC12314@google.com>
In-Reply-To: <CAL=YDW=ruMzd=twadncjgFTh3yv=796cN72amJ4ep8a41tgmrA@mail.gmail.com>

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.

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