On Tue, Oct 24, 2023 at 07:20:08AM +0800, Jiang Xin wrote:

On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt [off-list ref] wrote:

quoted

On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:

quoted

On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt [off-list ref] wrote:

quoted

On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:

quoted

@@ -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.

quoted

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