Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 23, 2023, 08:27 UTC
- Message-ID
- <ZTYue-3gAS1aGXNa@tanuki>
- In-Reply-To
- <ced46baeb1c18b416b4b4cc947f498bea2910b1b.1697725898.git.zhiyou.jx@alibaba-inc.com>
On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
Show 34 quoted lines
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> If an error occurs during an atomic fetch, a redundant error message
> will appear at the end of do_fetch(). It was introduced in b3a804663c
> (fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).
>
> Instead of displaying the error message unconditionally, the final error
> output should follow the pattern in update-ref.c and files-backend.c as
> follows:
>
> if (ref_transaction_abort(transaction, &error))
> error("abort: %s", error.buf);
>
> This will fix the test case "fetch porcelain output (atomic)" in t5574.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> builtin/fetch.c | 4 +---
> t/t5574-fetch-output.sh | 2 +-
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index fd134ba74d..01a573cf8d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -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. 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.
I wonder whether we should fix this by unifying all calls to `error()` to only happen in the cleanup block, and only iff the buffer length is non-zero?
Patrick
Show 18 quoted lines
> display_state_release(&display_state);
> close_fetch_head(&fetch_head);
> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
> index 1397101629..3c72fc693f 100755
> --- a/t/t5574-fetch-output.sh
> +++ b/t/t5574-fetch-output.sh
> @@ -97,7 +97,7 @@ do
> opt=
> ;;
> esac
> - test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
> + test_expect_success "fetch porcelain output ${opt:+(atomic)}" '
> test_when_finished "rm -rf porcelain" &&
>
> # Clone and pre-seed the repositories. We fetch references into two
> --
> 2.42.0.411.g813d9a9188
>