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

From: Jonathan Nieder <[email-masked]>
Date: Wed, 28 May 2014 16:39:40 -0700

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