[PATCH v3 0/2] fix fetch atomic error message

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

# Changes since v2:

Changed the patches with help from Patrick.

# range-diff v2...v3

1:  0b9865f1df ! 1:  0a6c53de7c t5574: test porcelain output of atomic fetch
    @@ Commit message
     
         In a later commit, we will fix this issue.
     
    +    Helped-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## t/t5574-fetch-output.sh ##
     @@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '
    @@ t/t5574-fetch-output.sh: test_expect_success 'fetch porcelain output' '
     +	FORCE_UPDATED_NEW=$(git rev-parse HEAD)
      '
      
    -+for opt in off on
    ++for opt in "" "--atomic"
     +do
    -+	case $opt in
    -+	on)
    -+		opt=--atomic
    -+		;;
    -+	off)
    -+		opt=
    -+		;;
    -+	esac
     +	test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
     +		test_when_finished "rm -rf porcelain" &&
     +
2:  e10fa198dd ! 2:  a8a7658fb2 fetch: no redundant error message for atomic fetch
    @@ Commit 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.
    +    Because a failure message is displayed before setting retcode in the
    +    function do_fetch(), calling error() on the err message at the end of
    +    this function may result in redundant or empty error message to be
    +    displayed.
     
         We can remove the redundant error() function, because we know that
         the function ref_transaction_abort() never fails. While we can find a
    @@ Commit message
             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.
    +    Following this pattern, we can tolerate the return value of the function
    +    ref_transaction_abort() being changed in the future. We also delay the
    +    output of the err message to the end of do_fetch() to reduce redundant
    +    code. With these changes, the test case "fetch porcelain output
    +    (atomic)" in t5574 will also be fixed.
     
    +    Helped-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## builtin/fetch.c ##
    +@@ builtin/fetch.c: 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;
    + 		}
    + 	}
    +@@ builtin/fetch.c: 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;
     @@ builtin/fetch.c: 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);
    --	}
    +-		error("%s", err.buf);
    ++	if (retcode) {
    ++		if (err.len) {
    ++			error("%s", err.buf);
    ++			strbuf_reset(&err);
    ++		}
    ++		if (transaction && ref_transaction_abort(transaction, &err) &&
    ++		    err.len)
    ++			error("%s", err.buf);
    + 	}
      
      	display_state_release(&display_state);

Jiang Xin (2):
  t5574: test porcelain output of atomic fetch
  fetch: no redundant error message for atomic fetch

 builtin/fetch.c         | 14 ++++---
 t/t5574-fetch-output.sh | 89 +++++++++++++++++++++++------------------
 2 files changed, 59 insertions(+), 44 deletions(-)