threads / patch / 60402

patch, 2 partst5574: test porcelain output of atomic fetch

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

## tl;dr

19 messages between Oct 19, 2023 and Dec 18, 2023. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

Jiang Xin· Oct 19, 2023, 14:34 UTC · lore
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(-)
Show changes to t/t5574-fetch-output.sh +57 −39
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
Jiang Xin· Oct 19, 2023, 14:34 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 2 files +2 −4

builtin/fetch.c, t/t5574-fetch-output.sh

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
Patrick Steinhardt· Oct 23, 2023, 08:27 UTC · re: Jiang Xin · lore

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

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
> 
Jiang Xin· Oct 23, 2023, 09:16 UTC · re: Patrick Steinhardt · lore

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

On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
Show 20 quoted lines
>
> 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

Patrick Steinhardt· Oct 23, 2023, 10:07 UTC · re: Jiang Xin · lore

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

On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
Show 35 quoted lines
> 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
Jiang Xin· Oct 23, 2023, 23:20 UTC · re: Patrick Steinhardt · lore

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

On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:
Show 43 quoted lines
>
> 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.

Show 5 quoted lines
> 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
Patrick Steinhardt· Oct 25, 2023, 08:21 UTC · re: Jiang Xin · lore

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

On Tue, Oct 24, 2023 at 07:20:08AM +0800, Jiang Xin wrote:
Show 49 quoted lines
> 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
Junio C Hamano· Oct 24, 2023, 18:16 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
> 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.
Jiang Xin· Dec 14, 2023, 12:33 UTC · re: Jiang Xin · lore

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

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
Jiang Xin· Dec 14, 2023, 12:33 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to t/t5574-fetch-output.sh +58 −39
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
Patrick Steinhardt· Dec 15, 2023, 09:56 UTC · re: Jiang Xin · lore

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

On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
[snip]
Show 45 quoted lines
> @@ -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
Jiang Xin· Dec 15, 2023, 11:16 UTC · re: Patrick Steinhardt · lore

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

On Fri, Dec 15, 2023 at 5:56 PM Patrick Steinhardt <ps@pks.im> wrote:
Show 53 quoted lines
>
> 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.
Junio C Hamano· Dec 15, 2023, 16:47 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 5 quoted lines
>> 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.

Jiang Xin· Dec 14, 2023, 12:33 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 2 files +2 −4

builtin/fetch.c, t/t5574-fetch-output.sh

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
Patrick Steinhardt· Dec 15, 2023, 09:56 UTC · re: Jiang Xin · lore

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

On Thu, Dec 14, 2023 at 08:33:12PM +0800, Jiang Xin wrote:
Show 12 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).
> 
> 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:
Show changes to builtin/fetch.c +6 −3
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
> 
Jiang Xin· Dec 17, 2023, 14:11 UTC · re: Jiang Xin · lore

[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(-)
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
Jiang Xin· Dec 17, 2023, 14:11 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to t/t5574-fetch-output.sh +50 −39
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
Jiang Xin· Dec 17, 2023, 14:11 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 2 files +10 −6

builtin/fetch.c, t/t5574-fetch-output.sh

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
Patrick Steinhardt· Dec 18, 2023, 08:14 UTC · re: Jiang Xin · lore

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

On Sun, Dec 17, 2023 at 10:11:32PM +0800, Jiang Xin wrote:
Show 6 quoted lines
> 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

← back to recent threads