Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch

[Jiang Xin <worldhello.net@gmail.com>]

On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,
> >       }
> >
> >  cleanup:
> > -     if (retcode && transaction) {
> > -             ref_transaction_abort(transaction, &err);
> > +     if (retcode && transaction && ref_transaction_abort(transaction, &err))
> >               error("%s", err.buf);
> > -     }
>
> Right. We already call `error()` in all cases where `err` was populated
> before we `goto cleanup;`, so calling it unconditionally a second time
> here is wrong.
>
> That being said, `ref_transaction_abort()` will end up calling the
> respective backend's implementation of `transaction_abort`, and for the
> files backend it actually ignores `err` completely. So if the abort
> fails, we would still end up calling `error()` with an empty string.

The transaction_abort implementations of the two builtin refs backends
will not use "err“ because they never fail (always return 0). Some one
may want to implement their own refs backend which may use the "err"
variable in their "transaction_abort". So follow the pattern as
update-ref.c and files-backend.c to call ref_transaction_abort() is
safe.

> Furthermore, it can happen that `transaction_commit` fails, writes to
> the buffer and then prints the error. If the abort now fails as well, we
> would end up printing the error message twice.

The abort never fails so error message from transaction_commit() will
not reach the code.

--
Jiang Xin