Re: [PATCH v11 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:

> --- a/refs.c
> +++ b/refs.c
[...]
> @@ -3385,6 +3408,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  {
>  	struct ref_update *update;
>  
> +	if (transaction->state != REF_TRANSACTION_OPEN)
> +		return -1;

I still think this is a step in the wrong direction.  It means that
instead of being able to do

	if (ref_transaction_update(..., &err))
		die("%s", err.buf);

and be certain that err.buf has a meaningful message, now I have to
worry that if the state were not REF_TRANSACTION_OPEN (e.g., due to
someone else's code forgetting to handle an error) then the result
would be a plain

	fatal:

The behavior would be much easier to debug if the code were

	if (transaction->state != REF_TRANSACTION_OPEN)
		die("BUG: update on transaction that is not open");

since then the symptom would be

	fatal: BUG: update on transaction that is not open

which is easier to work with.

If a non-OPEN state were a normal, recoverable error, then it could
append a message to the *err argument.  But there's no message that
could be put there that would be meaningful to the end-user.  So I
suspect it's not a normal, recoverable error.

If we want to collect errors to make _commit() later fail with a
message like '38 refs failed to update' or something (or a
string_list, if the API is to change that way in the future), then
_update() should not fail.  It can record information about what is
wrong with this update in the transaction and succeed so the caller
knows to keep going and collect other updates before the _commit()
that will fail.

Of course it's easily possible I'm missing something.  That's just how
I see it now.

Does that make sense?

Jonathan