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

[Patrick Steinhardt <ps@pks.im>]

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

On Thu, Dec 14, 2023 at 08:33:12PM +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).
> 
> In function do_fetch(), a failure message is already shown before the
> retcode is set, so we should not call additional error() at the end of
> this function.
> 
> We can remove the redundant error() function, because we know that
> the function ref_transaction_abort() never fails.

Okay, so this still suffers from the same issue as discussed in the
thread at <ZTYue-3gAS1aGXNa@tanuki>, but now it's documented in the
commit message. I'm still not convinced that is a good argument to say
that the function never fails, and if it ever would it would populate
the error message. Especially now where there's churn to introduce the
new reftable backend this could change any time.

For the record, I'm proposing to do something like the following:

diff --git a/builtin/fetch.c b/builtin/fetch.c
index fd134ba74d..80b8bc549d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1651,7 +1651,7 @@ static int do_fetch(struct transport *transport,
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
 		if (!transaction) {
-			retcode = error("%s", err.buf);
+			retcode = -1;
 			goto cleanup;
 		}
 	}
@@ -1711,7 +1711,6 @@ static int do_fetch(struct transport *transport,
 
 		retcode = ref_transaction_commit(transaction, &err);
 		if (retcode) {
-			error("%s", err.buf);
 			ref_transaction_free(transaction);
 			transaction = NULL;
 			goto cleanup;
@@ -1775,9 +1774,13 @@ static int do_fetch(struct transport *transport,
 	}
 
 cleanup:
+	if (retcode && err.len)
+		error("%s", err.buf);
 	if (retcode && transaction) {
+		strbuf_reset(&err);
 		ref_transaction_abort(transaction, &err);
-		error("%s", err.buf);
+		if (err.len)
+			error("%s", err.buf);
 	}
 
 	display_state_release(&display_state);

This would both fix the issue you observed, but also fixes issues in
case the ref backend failed without writing an error message to the
buffer. It also fixes issues if there were multiple failures, where we'd
print the initial error printed to the buffer twice.

I know this is mostly solidifying us against potential future changes,
but if it's comparatively easy like this I don't see much of a reason
against it.

Patrick

> While we can find a
> common pattern for calling ref_transaction_abort() by running command
> "git grep -A1 ref_transaction_abort", e.g.:
> 
>     if (ref_transaction_abort(transaction, &error))
>         error("abort: %s", error.buf);
> 
> We can fix this issue follow this pattern, and the test case "fetch
> porcelain output (atomic)" in t5574 will also be fixed. If in the future
> we decide that we don't need to check the return value of the function
> ref_transaction_abort(), this change can be fixed along with it.
> 
> 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);
> -	}
>  
>  	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 bc747efefc..8d01e36b3d 100755
> --- a/t/t5574-fetch-output.sh
> +++ b/t/t5574-fetch-output.sh
> @@ -98,7 +98,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.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
> 

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