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

[Patrick Steinhardt <ps@pks.im>]

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

On Tue, Oct 24, 2023 at 07:20:08AM +0800, Jiang Xin wrote:
> On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > 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.
> 
> If in the future ref_transaction_abort() fails for some reason, the
> err variable will be filled with the error message and the previous
> error message will be discarded, no duplication will occur. So I think
> use this fix is OK.

Isn't that assuming quite a lot about that future code though? It
assumes both:

    - That the code knows to always populate the error in the first
      place. Otherwise we may end up with the same empty error message
      that you aim to fix.

    - That the code will know to overwrite it instead of appending to
      it. Otherwise we may end up printing previous errors a second
      time.

Both of these assumptions may not hold. Current code that does write to
the error buffer for example will always append to it, not overwrite its
preexisting contents. And it's likely that future code will do the same.

Patrick

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