[PATCH 1/2] t5574: test porcelain output of atomic fetch

[PATCH 1/2] t5574: test porcelain output of atomic fetch

[Jiang Xin]

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

The test case "fetch porcelain output" checks output of the fetch
command. The error output must be empty with the follow assertion:

    test_must_be_empty stderr

Refactor this test case to run it twice. The first time will be run
using non-atomic fetch and the other time will be run using atomic
fetch. We can see that the above assertion fails for atomic get, as
shown below:

    ok 5 - fetch porcelain output  # TODO known breakage vanished
    not ok 6 - fetch porcelain output (atomic) # TODO known breakage

The failed test case had an error message with only the error prompt but
no message body, as follows:

    'stderr' is not empty, it contains:
    error:

In a later commit, we will fix this issue.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5574-fetch-output.sh | 96 ++++++++++++++++++++++++-----------------
 1 file changed, 57 insertions(+), 39 deletions(-)

diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 90e6dcb9a7..1397101629 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -61,9 +61,7 @@ test_expect_success 'fetch compact output' '
 	test_cmp expect actual
 '
 
-test_expect_success 'fetch porcelain output' '
-	test_when_finished "rm -rf porcelain" &&
-
+test_expect_success 'setup for fetch porcelain output' '
 	# Set up a bunch of references that we can use to demonstrate different
 	# kinds of flag symbols in the output format.
 	MAIN_OLD=$(git rev-parse HEAD) &&
@@ -74,15 +72,10 @@ test_expect_success 'fetch porcelain output' '
 	FORCE_UPDATED_OLD=$(git rev-parse HEAD) &&
 	git checkout main &&
 
-	# Clone and pre-seed the repositories. We fetch references into two
-	# namespaces so that we can test that rejected and force-updated
-	# references are reported properly.
-	refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
-	git clone . porcelain &&
-	git -C porcelain fetch origin $refspecs &&
+	# Backup to preseed.git
+	git clone --mirror . preseed.git &&
 
-	# Now that we have set up the client repositories we can change our
-	# local references.
+	# Continue changing our local references.
 	git branch new-branch &&
 	git branch -d deleted-branch &&
 	git checkout fast-forward &&
@@ -91,36 +84,61 @@ test_expect_success 'fetch porcelain output' '
 	git checkout force-updated &&
 	git reset --hard HEAD~ &&
 	test_commit --no-tag force-update-new &&
-	FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
-
-	cat >expect <<-EOF &&
-	- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
-	- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
-	! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/forced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
-	* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
-	EOF
-
-	# Execute a dry-run fetch first. We do this to assert that the dry-run
-	# and non-dry-run fetches produces the same output. Execution of the
-	# fetch is expected to fail as we have a rejected reference update.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --dry-run --prune origin $refspecs >actual &&
-	test_cmp expect actual &&
-
-	# And now we perform a non-dry-run fetch.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --prune origin $refspecs >actual 2>stderr &&
-	test_cmp expect actual &&
-	test_must_be_empty stderr
+	FORCE_UPDATED_NEW=$(git rev-parse HEAD)
 '
 
+for opt in off on
+do
+	case $opt in
+	on)
+		opt=--atomic
+		;;
+	off)
+		opt=
+		;;
+	esac
+	test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+		test_when_finished "rm -rf porcelain" &&
+
+		# Clone and pre-seed the repositories. We fetch references into two
+		# namespaces so that we can test that rejected and force-updated
+		# references are reported properly.
+		refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
+		git clone preseed.git porcelain &&
+		git -C porcelain fetch origin $opt $refspecs &&
+
+		cat >expect <<-EOF &&
+		- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
+		- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
+		! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/forced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
+		* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+		EOF
+
+		# Change the URL of the repository to fetch different references.
+		git -C porcelain remote set-url origin .. &&
+
+		# Execute a dry-run fetch first. We do this to assert that the dry-run
+		# and non-dry-run fetches produces the same output. Execution of the
+		# fetch is expected to fail as we have a rejected reference update.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --dry-run --prune origin $refspecs >actual &&
+		test_cmp expect actual &&
+
+		# And now we perform a non-dry-run fetch.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --prune origin $refspecs >actual 2>stderr &&
+		test_cmp expect actual &&
+		test_must_be_empty stderr
+	'
+done
+
 test_expect_success 'fetch porcelain with multiple remotes' '
 	test_when_finished "rm -rf porcelain" &&
 
-- 
2.42.0.411.g813d9a9188

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

[Jiang Xin]

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);
-	}
 
 	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

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

[Patrick Steinhardt]

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

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

[Jiang Xin]

On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > @@ -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.

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

--
Jiang Xin

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

[Patrick Steinhardt]

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

On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
> On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > > @@ -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.
> 
> > 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.

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

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

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

[Jiang Xin]

On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
> > On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > > > @@ -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.
> >
> > > 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

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

[Patrick Steinhardt]

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

On Tue, Oct 24, 2023 at 07:20:08AM +0800, Jiang Xin wrote:
> On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
> > > On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > >
> > > > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > > > > @@ -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.
> > >
> > > > 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.

Isn't that assuming quite a lot about that future code though? It
assumes both:

    - That the code knows to always populate the error in the first
      place. Otherwise we may end up with the same empty error message
      that you aim to fix.

    - That the code will know to overwrite it instead of appending to
      it. Otherwise we may end up printing previous errors a second
      time.

Both of these assumptions may not hold. Current code that does write to
the error buffer for example will always append to it, not overwrite its
preexisting contents. And it's likely that future code will do the same.

Patrick

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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

Sounds reasonable.  Thanks for a good review.

[PATCH v2 0/2] jx/fetch-atomic-error-message-fix

[Jiang Xin]

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

# Changes since v1:

1. Add a "test_commit ..." command in test case of t5574, so we can run
   test cases 4-6 individually.

2. Improve commit logs.


# range-diff v1...v2

1:  8c85f83e66 ! 1:  210191917b t5574: test porcelain output of atomic fetch
    @@ Commit message
     
             test_must_be_empty stderr
     
    -    Refactor this test case to run it twice. The first time will be run
    -    using non-atomic fetch and the other time will be run using atomic
    -    fetch. We can see that the above assertion fails for atomic get, as
    -    shown below:
    +    But this assertion fails if using atomic fetch. Refactor this test case
    +    to use different fetch options by splitting it into three test cases.
     
    +      1. "setup for fetch porcelain output".
    +
    +      2. "fetch porcelain output": for non-atomic fetch.
    +
    +      3. "fetch porcelain output (atomic)": for atomic fetch.
    +
    +    Add new command "test_commit ..." in the first test case, so that if we
    +    run these test cases individually (--run=4-6), "git rev-parse HEAD~"
    +    command will work properly. Run the above test cases, we can find that
    +    one test case has a known breakage, as shown below:
    +
    +        ok 4 - setup for fetch porcelain output
             ok 5 - fetch porcelain output  # TODO known breakage vanished
             not ok 6 - fetch porcelain output (atomic) # TODO known breakage
     
    -    The failed test case had an error message with only the error prompt but
    +    The failed test case has an error message with only the error prompt but
         no message body, as follows:
     
             'stderr' is not empty, it contains:
    @@ Commit message
         In a later commit, we will fix this issue.
     
         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 compact output' '
     +test_expect_success 'setup for fetch porcelain output' '
      	# Set up a bunch of references that we can use to demonstrate different
      	# kinds of flag symbols in the output format.
    ++	test_commit commit-for-porcelain-output &&
      	MAIN_OLD=$(git rev-parse HEAD) &&
    + 	git branch "fast-forward" &&
    + 	git branch "deleted-branch" &&
     @@ t/t5574-fetch-output.sh: test_expect_success 'fetch porcelain output' '
      	FORCE_UPDATED_OLD=$(git rev-parse HEAD) &&
      	git checkout main &&
2:  d3184a9d0f ! 2:  6fb83a0000 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).
     
    -    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:
    +    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. 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);
     
    -    This will fix the test case "fetch porcelain output (atomic)" in t5574.
    +    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 ##
     @@ builtin/fetch.c: static int do_fetch(struct transport *transport,

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

 builtin/fetch.c         |  4 +-
 t/t5574-fetch-output.sh | 97 ++++++++++++++++++++++++-----------------
 2 files changed, 59 insertions(+), 42 deletions(-)

-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

[PATCH v2 1/2] t5574: test porcelain output of atomic fetch

[Jiang Xin]

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

The test case "fetch porcelain output" checks output of the fetch
command. The error output must be empty with the follow assertion:

    test_must_be_empty stderr

But this assertion fails if using atomic fetch. Refactor this test case
to use different fetch options by splitting it into three test cases.

  1. "setup for fetch porcelain output".

  2. "fetch porcelain output": for non-atomic fetch.

  3. "fetch porcelain output (atomic)": for atomic fetch.

Add new command "test_commit ..." in the first test case, so that if we
run these test cases individually (--run=4-6), "git rev-parse HEAD~"
command will work properly. Run the above test cases, we can find that
one test case has a known breakage, as shown below:

    ok 4 - setup for fetch porcelain output
    ok 5 - fetch porcelain output  # TODO known breakage vanished
    not ok 6 - fetch porcelain output (atomic) # TODO known breakage

The failed test case has an error message with only the error prompt but
no message body, as follows:

    'stderr' is not empty, it contains:
    error:

In a later commit, we will fix this issue.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5574-fetch-output.sh | 97 ++++++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 39 deletions(-)

diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 90e6dcb9a7..bc747efefc 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -61,11 +61,10 @@ test_expect_success 'fetch compact output' '
 	test_cmp expect actual
 '
 
-test_expect_success 'fetch porcelain output' '
-	test_when_finished "rm -rf porcelain" &&
-
+test_expect_success 'setup for fetch porcelain output' '
 	# Set up a bunch of references that we can use to demonstrate different
 	# kinds of flag symbols in the output format.
+	test_commit commit-for-porcelain-output &&
 	MAIN_OLD=$(git rev-parse HEAD) &&
 	git branch "fast-forward" &&
 	git branch "deleted-branch" &&
@@ -74,15 +73,10 @@ test_expect_success 'fetch porcelain output' '
 	FORCE_UPDATED_OLD=$(git rev-parse HEAD) &&
 	git checkout main &&
 
-	# Clone and pre-seed the repositories. We fetch references into two
-	# namespaces so that we can test that rejected and force-updated
-	# references are reported properly.
-	refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
-	git clone . porcelain &&
-	git -C porcelain fetch origin $refspecs &&
+	# Backup to preseed.git
+	git clone --mirror . preseed.git &&
 
-	# Now that we have set up the client repositories we can change our
-	# local references.
+	# Continue changing our local references.
 	git branch new-branch &&
 	git branch -d deleted-branch &&
 	git checkout fast-forward &&
@@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '
 	git checkout force-updated &&
 	git reset --hard HEAD~ &&
 	test_commit --no-tag force-update-new &&
-	FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
-
-	cat >expect <<-EOF &&
-	- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
-	- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
-	! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/forced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
-	* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
-	EOF
-
-	# Execute a dry-run fetch first. We do this to assert that the dry-run
-	# and non-dry-run fetches produces the same output. Execution of the
-	# fetch is expected to fail as we have a rejected reference update.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --dry-run --prune origin $refspecs >actual &&
-	test_cmp expect actual &&
-
-	# And now we perform a non-dry-run fetch.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --prune origin $refspecs >actual 2>stderr &&
-	test_cmp expect actual &&
-	test_must_be_empty stderr
+	FORCE_UPDATED_NEW=$(git rev-parse HEAD)
 '
 
+for opt in off on
+do
+	case $opt in
+	on)
+		opt=--atomic
+		;;
+	off)
+		opt=
+		;;
+	esac
+	test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+		test_when_finished "rm -rf porcelain" &&
+
+		# Clone and pre-seed the repositories. We fetch references into two
+		# namespaces so that we can test that rejected and force-updated
+		# references are reported properly.
+		refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
+		git clone preseed.git porcelain &&
+		git -C porcelain fetch origin $opt $refspecs &&
+
+		cat >expect <<-EOF &&
+		- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
+		- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
+		! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/forced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
+		* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+		EOF
+
+		# Change the URL of the repository to fetch different references.
+		git -C porcelain remote set-url origin .. &&
+
+		# Execute a dry-run fetch first. We do this to assert that the dry-run
+		# and non-dry-run fetches produces the same output. Execution of the
+		# fetch is expected to fail as we have a rejected reference update.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --dry-run --prune origin $refspecs >actual &&
+		test_cmp expect actual &&
+
+		# And now we perform a non-dry-run fetch.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --prune origin $refspecs >actual 2>stderr &&
+		test_cmp expect actual &&
+		test_must_be_empty stderr
+	'
+done
+
 test_expect_success 'fetch porcelain with multiple remotes' '
 	test_when_finished "rm -rf porcelain" &&
 
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch

[Patrick Steinhardt]

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

On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
[snip]
> @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '
>  	git checkout force-updated &&
>  	git reset --hard HEAD~ &&
>  	test_commit --no-tag force-update-new &&
> -	FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
> -
> -	cat >expect <<-EOF &&
> -	- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
> -	- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
> -	  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
> -	! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
> -	* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
> -	  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
> -	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
> -	* $ZERO_OID $MAIN_OLD refs/forced/new-branch
> -	  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
> -	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
> -	* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
> -	EOF
> -
> -	# Execute a dry-run fetch first. We do this to assert that the dry-run
> -	# and non-dry-run fetches produces the same output. Execution of the
> -	# fetch is expected to fail as we have a rejected reference update.
> -	test_must_fail git -C porcelain fetch \
> -		--porcelain --dry-run --prune origin $refspecs >actual &&
> -	test_cmp expect actual &&
> -
> -	# And now we perform a non-dry-run fetch.
> -	test_must_fail git -C porcelain fetch \
> -		--porcelain --prune origin $refspecs >actual 2>stderr &&
> -	test_cmp expect actual &&
> -	test_must_be_empty stderr
> +	FORCE_UPDATED_NEW=$(git rev-parse HEAD)
>  '
>  
> +for opt in off on
> +do
> +	case $opt in
> +	on)
> +		opt=--atomic
> +		;;
> +	off)
> +		opt=
> +		;;
> +	esac

Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
this case statement. Not sure whether this is worth a reroll though,
probably not.

Patrick

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

Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch

[Jiang Xin]

On Fri, Dec 15, 2023 at 5:56 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> [snip]
> > @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '
> >       git checkout force-updated &&
> >       git reset --hard HEAD~ &&
> >       test_commit --no-tag force-update-new &&
> > -     FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
> > -
> > -     cat >expect <<-EOF &&
> > -     - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
> > -     - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
> > -     ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
> > -     * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
> > -     + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
> > -     * $ZERO_OID $MAIN_OLD refs/forced/new-branch
> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
> > -     + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
> > -     * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
> > -     EOF
> > -
> > -     # Execute a dry-run fetch first. We do this to assert that the dry-run
> > -     # and non-dry-run fetches produces the same output. Execution of the
> > -     # fetch is expected to fail as we have a rejected reference update.
> > -     test_must_fail git -C porcelain fetch \
> > -             --porcelain --dry-run --prune origin $refspecs >actual &&
> > -     test_cmp expect actual &&
> > -
> > -     # And now we perform a non-dry-run fetch.
> > -     test_must_fail git -C porcelain fetch \
> > -             --porcelain --prune origin $refspecs >actual 2>stderr &&
> > -     test_cmp expect actual &&
> > -     test_must_be_empty stderr
> > +     FORCE_UPDATED_NEW=$(git rev-parse HEAD)
> >  '
> >
> > +for opt in off on
> > +do
> > +     case $opt in
> > +     on)
> > +             opt=--atomic
> > +             ;;
> > +     off)
> > +             opt=
> > +             ;;
> > +     esac
>
> Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
> this case statement. Not sure whether this is worth a reroll though,
> probably not.

Yes, your code is much simpler.

Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

>> Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
>> this case statement. Not sure whether this is worth a reroll though,
>> probably not.
>
> Yes, your code is much simpler.

Yup, thanks for careful and helpful reviews, Patrick, on both
patches.  And of course, thanks, Jiang, for working on them.

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

[Jiang Xin]

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

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

[Patrick Steinhardt]

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

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

[Jiang Xin]

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

-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

[PATCH v3 1/2] t5574: test porcelain output of atomic fetch

[Jiang Xin]

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

The test case "fetch porcelain output" checks output of the fetch
command. The error output must be empty with the follow assertion:

    test_must_be_empty stderr

But this assertion fails if using atomic fetch. Refactor this test case
to use different fetch options by splitting it into three test cases.

  1. "setup for fetch porcelain output".

  2. "fetch porcelain output": for non-atomic fetch.

  3. "fetch porcelain output (atomic)": for atomic fetch.

Add new command "test_commit ..." in the first test case, so that if we
run these test cases individually (--run=4-6), "git rev-parse HEAD~"
command will work properly. Run the above test cases, we can find that
one test case has a known breakage, as shown below:

    ok 4 - setup for fetch porcelain output
    ok 5 - fetch porcelain output  # TODO known breakage vanished
    not ok 6 - fetch porcelain output (atomic) # TODO known breakage

The failed test case has an error message with only the error prompt but
no message body, as follows:

    'stderr' is not empty, it contains:
    error:

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 | 89 +++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 90e6dcb9a7..b579364c47 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -61,11 +61,10 @@ test_expect_success 'fetch compact output' '
 	test_cmp expect actual
 '
 
-test_expect_success 'fetch porcelain output' '
-	test_when_finished "rm -rf porcelain" &&
-
+test_expect_success 'setup for fetch porcelain output' '
 	# Set up a bunch of references that we can use to demonstrate different
 	# kinds of flag symbols in the output format.
+	test_commit commit-for-porcelain-output &&
 	MAIN_OLD=$(git rev-parse HEAD) &&
 	git branch "fast-forward" &&
 	git branch "deleted-branch" &&
@@ -74,15 +73,10 @@ test_expect_success 'fetch porcelain output' '
 	FORCE_UPDATED_OLD=$(git rev-parse HEAD) &&
 	git checkout main &&
 
-	# Clone and pre-seed the repositories. We fetch references into two
-	# namespaces so that we can test that rejected and force-updated
-	# references are reported properly.
-	refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
-	git clone . porcelain &&
-	git -C porcelain fetch origin $refspecs &&
+	# Backup to preseed.git
+	git clone --mirror . preseed.git &&
 
-	# Now that we have set up the client repositories we can change our
-	# local references.
+	# Continue changing our local references.
 	git branch new-branch &&
 	git branch -d deleted-branch &&
 	git checkout fast-forward &&
@@ -91,36 +85,53 @@ test_expect_success 'fetch porcelain output' '
 	git checkout force-updated &&
 	git reset --hard HEAD~ &&
 	test_commit --no-tag force-update-new &&
-	FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
-
-	cat >expect <<-EOF &&
-	- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
-	- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
-	! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/forced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
-	* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
-	EOF
-
-	# Execute a dry-run fetch first. We do this to assert that the dry-run
-	# and non-dry-run fetches produces the same output. Execution of the
-	# fetch is expected to fail as we have a rejected reference update.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --dry-run --prune origin $refspecs >actual &&
-	test_cmp expect actual &&
-
-	# And now we perform a non-dry-run fetch.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --prune origin $refspecs >actual 2>stderr &&
-	test_cmp expect actual &&
-	test_must_be_empty stderr
+	FORCE_UPDATED_NEW=$(git rev-parse HEAD)
 '
 
+for opt in "" "--atomic"
+do
+	test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+		test_when_finished "rm -rf porcelain" &&
+
+		# Clone and pre-seed the repositories. We fetch references into two
+		# namespaces so that we can test that rejected and force-updated
+		# references are reported properly.
+		refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
+		git clone preseed.git porcelain &&
+		git -C porcelain fetch origin $opt $refspecs &&
+
+		cat >expect <<-EOF &&
+		- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
+		- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
+		! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/forced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
+		* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+		EOF
+
+		# Change the URL of the repository to fetch different references.
+		git -C porcelain remote set-url origin .. &&
+
+		# Execute a dry-run fetch first. We do this to assert that the dry-run
+		# and non-dry-run fetches produces the same output. Execution of the
+		# fetch is expected to fail as we have a rejected reference update.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --dry-run --prune origin $refspecs >actual &&
+		test_cmp expect actual &&
+
+		# And now we perform a non-dry-run fetch.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --prune origin $refspecs >actual 2>stderr &&
+		test_cmp expect actual &&
+		test_must_be_empty stderr
+	'
+done
+
 test_expect_success 'fetch porcelain with multiple remotes' '
 	test_when_finished "rm -rf porcelain" &&
 
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

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

[Jiang Xin]

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

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

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         | 14 +++++++++-----
 t/t5574-fetch-output.sh |  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index fd134ba74d..a284b970ef 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,14 @@ static int do_fetch(struct transport *transport,
 	}
 
 cleanup:
-	if (retcode && transaction) {
-		ref_transaction_abort(transaction, &err);
-		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);
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index b579364c47..1400ef14cd 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -90,7 +90,7 @@ test_expect_success 'setup for fetch porcelain output' '
 
 for opt in "" "--atomic"
 do
-	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

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

[Patrick Steinhardt]

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

On Sun, Dec 17, 2023 at 10:11:32PM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> 
> # Changes since v2:
> 
> Changed the patches with help from Patrick.
> 

Thanks, this version looks good to me!

Patrick

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