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

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.

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