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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html