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

[Patrick Steinhardt <ps@pks.im>]

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

On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> 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

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

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