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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
> 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.

With that reasoning we could get rid of the error handling of abort
completely as it's known not to fail. But only because it does not fail
right now doesn't mean that it won't in the future, as the infra for it
to fail is all in place. And in case it ever does the current code will
run into the bug I described.

So in my opinion, we should either refactor the code to clarify that
this cannot fail indeed. Or do the right thing and handle the error case
correctly, which right now we don't.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]