{"thread":{"id":"60402","subject":"[PATCH 1/2] t5574: test porcelain output of atomic fetch","startedAt":"2023-10-19T14:34:45Z","lastAt":"2023-12-18T08:14:57Z","messageCount":19,"participants":["Jiang Xin","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"483487","messageId":"38b0b22038399265407f7fc5f126f471dcc6f1a3.1697725898.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":null,"subject":"[PATCH 1/2] t5574: test porcelain output of atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-19T14:34:32Z","receivedAt":"2023-10-19T14:34:45Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nThe test case \"fetch porcelain output\" checks output of the fetch\ncommand. The error output must be empty with the follow assertion:\n\n    test_must_be_empty stderr\n\nRefactor this test case to run it twice. The first time will be run\nusing non-atomic fetch and the other time will be run using atomic\nfetch. We can see that the above assertion fails for atomic get, as\nshown below:\n\n    ok 5 - fetch porcelain output  # TODO known breakage vanished\n    not ok 6 - fetch porcelain output (atomic) # TODO known breakage\n\nThe failed test case had an error message with only the error prompt but\nno message body, as follows:\n\n    'stderr' is not empty, it contains:\n    error:\n\nIn a later commit, we will fix this issue.\n\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n t/t5574-fetch-output.sh | 96 ++++++++++++++++++++++++-----------------\n 1 file changed, 57 insertions(+), 39 deletions(-)\n\ndiff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\nindex 90e6dcb9a7..1397101629 100755\n--- a/t/t5574-fetch-output.sh\n+++ b/t/t5574-fetch-output.sh\n@@ -61,9 +61,7 @@ test_expect_success 'fetch compact output' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'fetch porcelain output' '\n-\ttest_when_finished \"rm -rf porcelain\" &&\n-\n+test_expect_success 'setup for fetch porcelain output' '\n \t# Set up a bunch of references that we can use to demonstrate different\n \t# kinds of flag symbols in the output format.\n \tMAIN_OLD=$(git rev-parse HEAD) &&\n@@ -74,15 +72,10 @@ test_expect_success 'fetch porcelain output' '\n \tFORCE_UPDATED_OLD=$(git rev-parse HEAD) &&\n \tgit checkout main &&\n \n-\t# Clone and pre-seed the repositories. We fetch references into two\n-\t# namespaces so that we can test that rejected and force-updated\n-\t# references are reported properly.\n-\trefspecs=\"refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*\" &&\n-\tgit clone . porcelain &&\n-\tgit -C porcelain fetch origin $refspecs &&\n+\t# Backup to preseed.git\n+\tgit clone --mirror . preseed.git &&\n \n-\t# Now that we have set up the client repositories we can change our\n-\t# local references.\n+\t# Continue changing our local references.\n \tgit branch new-branch &&\n \tgit branch -d deleted-branch &&\n \tgit checkout fast-forward &&\n@@ -91,36 +84,61 @@ test_expect_success 'fetch porcelain output' '\n \tgit checkout force-updated &&\n \tgit reset --hard HEAD~ &&\n \ttest_commit --no-tag force-update-new &&\n-\tFORCE_UPDATED_NEW=$(git rev-parse HEAD) &&\n-\n-\tcat >expect <<-EOF &&\n-\t- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n-\t- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n-\t! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n-\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/forced/new-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n-\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n-\tEOF\n-\n-\t# Execute a dry-run fetch first. We do this to assert that the dry-run\n-\t# and non-dry-run fetches produces the same output. Execution of the\n-\t# fetch is expected to fail as we have a rejected reference update.\n-\ttest_must_fail git -C porcelain fetch \\\n-\t\t--porcelain --dry-run --prune origin $refspecs >actual &&\n-\ttest_cmp expect actual &&\n-\n-\t# And now we perform a non-dry-run fetch.\n-\ttest_must_fail git -C porcelain fetch \\\n-\t\t--porcelain --prune origin $refspecs >actual 2>stderr &&\n-\ttest_cmp expect actual &&\n-\ttest_must_be_empty stderr\n+\tFORCE_UPDATED_NEW=$(git rev-parse HEAD)\n '\n \n+for opt in off on\n+do\n+\tcase $opt in\n+\ton)\n+\t\topt=--atomic\n+\t\t;;\n+\toff)\n+\t\topt=\n+\t\t;;\n+\tesac\n+\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n+\t\ttest_when_finished \"rm -rf porcelain\" &&\n+\n+\t\t# Clone and pre-seed the repositories. We fetch references into two\n+\t\t# namespaces so that we can test that rejected and force-updated\n+\t\t# references are reported properly.\n+\t\trefspecs=\"refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*\" &&\n+\t\tgit clone preseed.git porcelain &&\n+\t\tgit -C porcelain fetch origin $opt $refspecs &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\t- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n+\t\t- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n+\t\t! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n+\t\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/forced/new-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n+\t\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n+\t\tEOF\n+\n+\t\t# Change the URL of the repository to fetch different references.\n+\t\tgit -C porcelain remote set-url origin .. &&\n+\n+\t\t# Execute a dry-run fetch first. We do this to assert that the dry-run\n+\t\t# and non-dry-run fetches produces the same output. Execution of the\n+\t\t# fetch is expected to fail as we have a rejected reference update.\n+\t\ttest_must_fail git -C porcelain fetch $opt \\\n+\t\t\t--porcelain --dry-run --prune origin $refspecs >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# And now we perform a non-dry-run fetch.\n+\t\ttest_must_fail git -C porcelain fetch $opt \\\n+\t\t\t--porcelain --prune origin $refspecs >actual 2>stderr &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty stderr\n+\t'\n+done\n+\n test_expect_success 'fetch porcelain with multiple remotes' '\n \ttest_when_finished \"rm -rf porcelain\" &&\n \n-- \n2.42.0.411.g813d9a9188\n\n"},{"id":"483488","messageId":"ced46baeb1c18b416b4b4cc947f498bea2910b1b.1697725898.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":"38b0b22038399265407f7fc5f126f471dcc6f1a3.1697725898.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH 2/2] fetch: no redundant error message for atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-19T14:34:33Z","receivedAt":"2023-10-19T14:34:46Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nIf an error occurs during an atomic fetch, a redundant error message\nwill appear at the end of do_fetch(). It was introduced in b3a804663c\n(fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).\n\nInstead of displaying the error message unconditionally, the final error\noutput should follow the pattern in update-ref.c and files-backend.c as\nfollows:\n\n    if (ref_transaction_abort(transaction, &error))\n        error(\"abort: %s\", error.buf);\n\nThis will fix the test case \"fetch porcelain output (atomic)\" in t5574.\n\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n builtin/fetch.c         | 4 +---\n t/t5574-fetch-output.sh | 2 +-\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fd134ba74d..01a573cf8d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n-\tif (retcode && transaction) {\n-\t\tref_transaction_abort(transaction, &err);\n+\tif (retcode && transaction && ref_transaction_abort(transaction, &err))\n \t\terror(\"%s\", err.buf);\n-\t}\n \n \tdisplay_state_release(&display_state);\n \tclose_fetch_head(&fetch_head);\ndiff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\nindex 1397101629..3c72fc693f 100755\n--- a/t/t5574-fetch-output.sh\n+++ b/t/t5574-fetch-output.sh\n@@ -97,7 +97,7 @@ do\n \t\topt=\n \t\t;;\n \tesac\n-\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n+\ttest_expect_success \"fetch porcelain output ${opt:+(atomic)}\" '\n \t\ttest_when_finished \"rm -rf porcelain\" &&\n \n \t\t# Clone and pre-seed the repositories. We fetch references into two\n-- \n2.42.0.411.g813d9a9188\n\n"},{"id":"483652","messageId":"ZTYue-3gAS1aGXNa@tanuki","threadId":"60402","inReplyTo":"ced46baeb1c18b416b4b4cc947f498bea2910b1b.1697725898.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-23T08:27:39Z","receivedAt":"2023-10-23T08:27:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:\n> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> \n> If an error occurs during an atomic fetch, a redundant error message\n> will appear at the end of do_fetch(). It was introduced in b3a804663c\n> (fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).\n> \n> Instead of displaying the error message unconditionally, the final error\n> output should follow the pattern in update-ref.c and files-backend.c as\n> follows:\n> \n>     if (ref_transaction_abort(transaction, &error))\n>         error(\"abort: %s\", error.buf);\n> \n> This will fix the test case \"fetch porcelain output (atomic)\" in t5574.\n> \n> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> ---\n>  builtin/fetch.c         | 4 +---\n>  t/t5574-fetch-output.sh | 2 +-\n>  2 files changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index fd134ba74d..01a573cf8d 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n>  \t}\n>  \n>  cleanup:\n> -\tif (retcode && transaction) {\n> -\t\tref_transaction_abort(transaction, &err);\n> +\tif (retcode && transaction && ref_transaction_abort(transaction, &err))\n>  \t\terror(\"%s\", err.buf);\n> -\t}\n\nRight. We already call `error()` in all cases where `err` was populated\nbefore we `goto cleanup;`, so calling it unconditionally a second time\nhere is wrong.\n\nThat being said, `ref_transaction_abort()` will end up calling the\nrespective backend's implementation of `transaction_abort`, and for the\nfiles backend it actually ignores `err` completely. So if the abort\nfails, we would still end up calling `error()` with an empty string.\nFurthermore, it can happen that `transaction_commit` fails, writes to\nthe buffer and then prints the error. If the abort now fails as well, we\nwould end up printing the error message twice.\n\nI wonder whether we should fix this by unifying all calls to `error()`\nto only happen in the cleanup block, and only iff the buffer length is\nnon-zero?\n\nPatrick\n\n>  \tdisplay_state_release(&display_state);\n>  \tclose_fetch_head(&fetch_head);\n> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\n> index 1397101629..3c72fc693f 100755\n> --- a/t/t5574-fetch-output.sh\n> +++ b/t/t5574-fetch-output.sh\n> @@ -97,7 +97,7 @@ do\n>  \t\topt=\n>  \t\t;;\n>  \tesac\n> -\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n> +\ttest_expect_success \"fetch porcelain output ${opt:+(atomic)}\" '\n>  \t\ttest_when_finished \"rm -rf porcelain\" &&\n>  \n>  \t\t# Clone and pre-seed the repositories. We fetch references into two\n> -- \n> 2.42.0.411.g813d9a9188\n> \n"},{"id":"483653","messageId":"CANYiYbEJ_mHdsPM3-huDPFktSWFhrpoz7Cvf000JSfZM2cco9w@mail.gmail.com","threadId":"60402","inReplyTo":"ZTYue-3gAS1aGXNa@tanuki","subject":"Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-23T09:16:20Z","receivedAt":"2023-10-23T09:16:35Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:\n> > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n> >       }\n> >\n> >  cleanup:\n> > -     if (retcode && transaction) {\n> > -             ref_transaction_abort(transaction, &err);\n> > +     if (retcode && transaction && ref_transaction_abort(transaction, &err))\n> >               error(\"%s\", err.buf);\n> > -     }\n>\n> Right. We already call `error()` in all cases where `err` was populated\n> before we `goto cleanup;`, so calling it unconditionally a second time\n> here is wrong.\n>\n> That being said, `ref_transaction_abort()` will end up calling the\n> respective backend's implementation of `transaction_abort`, and for the\n> files backend it actually ignores `err` completely. So if the abort\n> fails, we would still end up calling `error()` with an empty string.\n\nThe transaction_abort implementations of the two builtin refs backends\nwill not use \"err“ because they never fail (always return 0). Some one\nmay want to implement their own refs backend which may use the \"err\"\nvariable in their \"transaction_abort\". So follow the pattern as\nupdate-ref.c and files-backend.c to call ref_transaction_abort() is\nsafe.\n\n> Furthermore, it can happen that `transaction_commit` fails, writes to\n> the buffer and then prints the error. If the abort now fails as well, we\n> would end up printing the error message twice.\n\nThe abort never fails so error message from transaction_commit() will\nnot reach the code.\n\n--\nJiang Xin\n"},{"id":"483658","messageId":"ZTZF3AbNNuGpy38l@tanuki","threadId":"60402","inReplyTo":"CANYiYbEJ_mHdsPM3-huDPFktSWFhrpoz7Cvf000JSfZM2cco9w@mail.gmail.com","subject":"Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-23T10:07:24Z","receivedAt":"2023-10-23T10:07:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:\n> On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:\n> > > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n> > >       }\n> > >\n> > >  cleanup:\n> > > -     if (retcode && transaction) {\n> > > -             ref_transaction_abort(transaction, &err);\n> > > +     if (retcode && transaction && ref_transaction_abort(transaction, &err))\n> > >               error(\"%s\", err.buf);\n> > > -     }\n> >\n> > Right. We already call `error()` in all cases where `err` was populated\n> > before we `goto cleanup;`, so calling it unconditionally a second time\n> > here is wrong.\n> >\n> > That being said, `ref_transaction_abort()` will end up calling the\n> > respective backend's implementation of `transaction_abort`, and for the\n> > files backend it actually ignores `err` completely. So if the abort\n> > fails, we would still end up calling `error()` with an empty string.\n> \n> The transaction_abort implementations of the two builtin refs backends\n> will not use \"err“ because they never fail (always return 0). Some one\n> may want to implement their own refs backend which may use the \"err\"\n> variable in their \"transaction_abort\". So follow the pattern as\n> update-ref.c and files-backend.c to call ref_transaction_abort() is\n> safe.\n> \n> > Furthermore, it can happen that `transaction_commit` fails, writes to\n> > the buffer and then prints the error. If the abort now fails as well, we\n> > would end up printing the error message twice.\n> \n> The abort never fails so error message from transaction_commit() will\n> not reach the code.\n\nWith that reasoning we could get rid of the error handling of abort\ncompletely as it's known not to fail. But only because it does not fail\nright now doesn't mean that it won't in the future, as the infra for it\nto fail is all in place. And in case it ever does the current code will\nrun into the bug I described.\n\nSo in my opinion, we should either refactor the code to clarify that\nthis cannot fail indeed. Or do the right thing and handle the error case\ncorrectly, which right now we don't.\n\nPatrick\n"},{"id":"483750","messageId":"CANYiYbG0YFc4Hg=e+0db4NBgM2QwOLpjHjfp8WaoObNxR-=euA@mail.gmail.com","threadId":"60402","inReplyTo":"ZTZF3AbNNuGpy38l@tanuki","subject":"Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-23T23:20:08Z","receivedAt":"2023-10-23T23:20:23Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:\n> > On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:\n> > >\n> > > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:\n> > > > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n> > > >       }\n> > > >\n> > > >  cleanup:\n> > > > -     if (retcode && transaction) {\n> > > > -             ref_transaction_abort(transaction, &err);\n> > > > +     if (retcode && transaction && ref_transaction_abort(transaction, &err))\n> > > >               error(\"%s\", err.buf);\n> > > > -     }\n> > >\n> > > Right. We already call `error()` in all cases where `err` was populated\n> > > before we `goto cleanup;`, so calling it unconditionally a second time\n> > > here is wrong.\n> > >\n> > > That being said, `ref_transaction_abort()` will end up calling the\n> > > respective backend's implementation of `transaction_abort`, and for the\n> > > files backend it actually ignores `err` completely. So if the abort\n> > > fails, we would still end up calling `error()` with an empty string.\n> >\n> > The transaction_abort implementations of the two builtin refs backends\n> > will not use \"err“ because they never fail (always return 0). Some one\n> > may want to implement their own refs backend which may use the \"err\"\n> > variable in their \"transaction_abort\". So follow the pattern as\n> > update-ref.c and files-backend.c to call ref_transaction_abort() is\n> > safe.\n> >\n> > > Furthermore, it can happen that `transaction_commit` fails, writes to\n> > > the buffer and then prints the error. If the abort now fails as well, we\n> > > would end up printing the error message twice.\n> >\n> > The abort never fails so error message from transaction_commit() will\n> > not reach the code.\n>\n> With that reasoning we could get rid of the error handling of abort\n> completely as it's known not to fail. But only because it does not fail\n> right now doesn't mean that it won't in the future, as the infra for it\n> to fail is all in place. And in case it ever does the current code will\n> run into the bug I described.\n\nIf in the future ref_transaction_abort() fails for some reason, the\nerr variable will be filled with the error message and the previous\nerror message will be discarded, no duplication will occur. So I think\nuse this fix is OK.\n\n> So in my opinion, we should either refactor the code to clarify that\n> this cannot fail indeed. Or do the right thing and handle the error case\n> correctly, which right now we don't.\n>\n> Patrick\n"},{"id":"483803","messageId":"xmqq7cnb51ii.fsf@gitster.g","threadId":"60402","inReplyTo":"ZTZF3AbNNuGpy38l@tanuki","subject":"Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-24T18:16:53Z","receivedAt":"2023-10-24T18:16:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> With that reasoning we could get rid of the error handling of abort\n> completely as it's known not to fail. But only because it does not fail\n> right now doesn't mean that it won't in the future, as the infra for it\n> to fail is all in place. And in case it ever does the current code will\n> run into the bug I described.\n>\n> So in my opinion, we should either refactor the code to clarify that\n> this cannot fail indeed. Or do the right thing and handle the error case\n> correctly, which right now we don't.\n\nSounds reasonable.  Thanks for a good review.\n"},{"id":"483844","messageId":"ZTjQIrCgSANAT8wR@tanuki","threadId":"60402","inReplyTo":"CANYiYbG0YFc4Hg=e+0db4NBgM2QwOLpjHjfp8WaoObNxR-=euA@mail.gmail.com","subject":"Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-25T08:21:54Z","receivedAt":"2023-10-25T08:22:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 24, 2023 at 07:20:08AM +0800, Jiang Xin wrote:\n> On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:\n> > > On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:\n> > > >\n> > > > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:\n> > > > > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n> > > > >       }\n> > > > >\n> > > > >  cleanup:\n> > > > > -     if (retcode && transaction) {\n> > > > > -             ref_transaction_abort(transaction, &err);\n> > > > > +     if (retcode && transaction && ref_transaction_abort(transaction, &err))\n> > > > >               error(\"%s\", err.buf);\n> > > > > -     }\n> > > >\n> > > > Right. We already call `error()` in all cases where `err` was populated\n> > > > before we `goto cleanup;`, so calling it unconditionally a second time\n> > > > here is wrong.\n> > > >\n> > > > That being said, `ref_transaction_abort()` will end up calling the\n> > > > respective backend's implementation of `transaction_abort`, and for the\n> > > > files backend it actually ignores `err` completely. So if the abort\n> > > > fails, we would still end up calling `error()` with an empty string.\n> > >\n> > > The transaction_abort implementations of the two builtin refs backends\n> > > will not use \"err“ because they never fail (always return 0). Some one\n> > > may want to implement their own refs backend which may use the \"err\"\n> > > variable in their \"transaction_abort\". So follow the pattern as\n> > > update-ref.c and files-backend.c to call ref_transaction_abort() is\n> > > safe.\n> > >\n> > > > Furthermore, it can happen that `transaction_commit` fails, writes to\n> > > > the buffer and then prints the error. If the abort now fails as well, we\n> > > > would end up printing the error message twice.\n> > >\n> > > The abort never fails so error message from transaction_commit() will\n> > > not reach the code.\n> >\n> > With that reasoning we could get rid of the error handling of abort\n> > completely as it's known not to fail. But only because it does not fail\n> > right now doesn't mean that it won't in the future, as the infra for it\n> > to fail is all in place. And in case it ever does the current code will\n> > run into the bug I described.\n> \n> If in the future ref_transaction_abort() fails for some reason, the\n> err variable will be filled with the error message and the previous\n> error message will be discarded, no duplication will occur. So I think\n> use this fix is OK.\n\nIsn't that assuming quite a lot about that future code though? It\nassumes both:\n\n    - That the code knows to always populate the error in the first\n      place. Otherwise we may end up with the same empty error message\n      that you aim to fix.\n\n    - That the code will know to overwrite it instead of appending to\n      it. Otherwise we may end up printing previous errors a second\n      time.\n\nBoth of these assumptions may not hold. Current code that does write to\nthe error buffer for example will always append to it, not overwrite its\npreexisting contents. And it's likely that future code will do the same.\n\nPatrick\n"},{"id":"485639","messageId":"cover.1702556642.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":"38b0b22038399265407f7fc5f126f471dcc6f1a3.1697725898.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 0/2] jx/fetch-atomic-error-message-fix","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T12:33:10Z","receivedAt":"2023-12-14T12:33:19Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\n# Changes since v1:\n\n1. Add a \"test_commit ...\" command in test case of t5574, so we can run\n   test cases 4-6 individually.\n\n2. Improve commit logs.\n\n\n# range-diff v1...v2\n\n1:  8c85f83e66 ! 1:  210191917b t5574: test porcelain output of atomic fetch\n    @@ Commit message\n     \n             test_must_be_empty stderr\n     \n    -    Refactor this test case to run it twice. The first time will be run\n    -    using non-atomic fetch and the other time will be run using atomic\n    -    fetch. We can see that the above assertion fails for atomic get, as\n    -    shown below:\n    +    But this assertion fails if using atomic fetch. Refactor this test case\n    +    to use different fetch options by splitting it into three test cases.\n     \n    +      1. \"setup for fetch porcelain output\".\n    +\n    +      2. \"fetch porcelain output\": for non-atomic fetch.\n    +\n    +      3. \"fetch porcelain output (atomic)\": for atomic fetch.\n    +\n    +    Add new command \"test_commit ...\" in the first test case, so that if we\n    +    run these test cases individually (--run=4-6), \"git rev-parse HEAD~\"\n    +    command will work properly. Run the above test cases, we can find that\n    +    one test case has a known breakage, as shown below:\n    +\n    +        ok 4 - setup for fetch porcelain output\n             ok 5 - fetch porcelain output  # TODO known breakage vanished\n             not ok 6 - fetch porcelain output (atomic) # TODO known breakage\n     \n    -    The failed test case had an error message with only the error prompt but\n    +    The failed test case has an error message with only the error prompt but\n         no message body, as follows:\n     \n             'stderr' is not empty, it contains:\n    @@ Commit message\n         In a later commit, we will fix this issue.\n     \n         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n     \n      ## t/t5574-fetch-output.sh ##\n     @@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '\n    @@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '\n     +test_expect_success 'setup for fetch porcelain output' '\n      \t# Set up a bunch of references that we can use to demonstrate different\n      \t# kinds of flag symbols in the output format.\n    ++\ttest_commit commit-for-porcelain-output &&\n      \tMAIN_OLD=$(git rev-parse HEAD) &&\n    + \tgit branch \"fast-forward\" &&\n    + \tgit branch \"deleted-branch\" &&\n     @@ t/t5574-fetch-output.sh: test_expect_success 'fetch porcelain output' '\n      \tFORCE_UPDATED_OLD=$(git rev-parse HEAD) &&\n      \tgit checkout main &&\n2:  d3184a9d0f ! 2:  6fb83a0000 fetch: no redundant error message for atomic fetch\n    @@ Commit message\n         will appear at the end of do_fetch(). It was introduced in b3a804663c\n         (fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).\n     \n    -    Instead of displaying the error message unconditionally, the final error\n    -    output should follow the pattern in update-ref.c and files-backend.c as\n    -    follows:\n    +    In function do_fetch(), a failure message is already shown before the\n    +    retcode is set, so we should not call additional error() at the end of\n    +    this function.\n    +\n    +    We can remove the redundant error() function, because we know that\n    +    the function ref_transaction_abort() never fails. While we can find a\n    +    common pattern for calling ref_transaction_abort() by running command\n    +    \"git grep -A1 ref_transaction_abort\", e.g.:\n     \n             if (ref_transaction_abort(transaction, &error))\n                 error(\"abort: %s\", error.buf);\n     \n    -    This will fix the test case \"fetch porcelain output (atomic)\" in t5574.\n    +    We can fix this issue follow this pattern, and the test case \"fetch\n    +    porcelain output (atomic)\" in t5574 will also be fixed. If in the future\n    +    we decide that we don't need to check the return value of the function\n    +    ref_transaction_abort(), this change can be fixed along with it.\n     \n         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n     \n      ## builtin/fetch.c ##\n     @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n\nJiang Xin (2):\n  t5574: test porcelain output of atomic fetch\n  fetch: no redundant error message for atomic fetch\n\n builtin/fetch.c         |  4 +-\n t/t5574-fetch-output.sh | 97 ++++++++++++++++++++++++-----------------\n 2 files changed, 59 insertions(+), 42 deletions(-)\n\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485640","messageId":"210191917bcfa9293622908c291652059576f3e5.1702556642.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":"cover.1702556642.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 1/2] t5574: test porcelain output of atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T12:33:11Z","receivedAt":"2023-12-14T12:33:20Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nThe test case \"fetch porcelain output\" checks output of the fetch\ncommand. The error output must be empty with the follow assertion:\n\n    test_must_be_empty stderr\n\nBut this assertion fails if using atomic fetch. Refactor this test case\nto use different fetch options by splitting it into three test cases.\n\n  1. \"setup for fetch porcelain output\".\n\n  2. \"fetch porcelain output\": for non-atomic fetch.\n\n  3. \"fetch porcelain output (atomic)\": for atomic fetch.\n\nAdd new command \"test_commit ...\" in the first test case, so that if we\nrun these test cases individually (--run=4-6), \"git rev-parse HEAD~\"\ncommand will work properly. Run the above test cases, we can find that\none test case has a known breakage, as shown below:\n\n    ok 4 - setup for fetch porcelain output\n    ok 5 - fetch porcelain output  # TODO known breakage vanished\n    not ok 6 - fetch porcelain output (atomic) # TODO known breakage\n\nThe failed test case has an error message with only the error prompt but\nno message body, as follows:\n\n    'stderr' is not empty, it contains:\n    error:\n\nIn a later commit, we will fix this issue.\n\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n t/t5574-fetch-output.sh | 97 ++++++++++++++++++++++++-----------------\n 1 file changed, 58 insertions(+), 39 deletions(-)\n\ndiff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\nindex 90e6dcb9a7..bc747efefc 100755\n--- a/t/t5574-fetch-output.sh\n+++ b/t/t5574-fetch-output.sh\n@@ -61,11 +61,10 @@ test_expect_success 'fetch compact output' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'fetch porcelain output' '\n-\ttest_when_finished \"rm -rf porcelain\" &&\n-\n+test_expect_success 'setup for fetch porcelain output' '\n \t# Set up a bunch of references that we can use to demonstrate different\n \t# kinds of flag symbols in the output format.\n+\ttest_commit commit-for-porcelain-output &&\n \tMAIN_OLD=$(git rev-parse HEAD) &&\n \tgit branch \"fast-forward\" &&\n \tgit branch \"deleted-branch\" &&\n@@ -74,15 +73,10 @@ test_expect_success 'fetch porcelain output' '\n \tFORCE_UPDATED_OLD=$(git rev-parse HEAD) &&\n \tgit checkout main &&\n \n-\t# Clone and pre-seed the repositories. We fetch references into two\n-\t# namespaces so that we can test that rejected and force-updated\n-\t# references are reported properly.\n-\trefspecs=\"refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*\" &&\n-\tgit clone . porcelain &&\n-\tgit -C porcelain fetch origin $refspecs &&\n+\t# Backup to preseed.git\n+\tgit clone --mirror . preseed.git &&\n \n-\t# Now that we have set up the client repositories we can change our\n-\t# local references.\n+\t# Continue changing our local references.\n \tgit branch new-branch &&\n \tgit branch -d deleted-branch &&\n \tgit checkout fast-forward &&\n@@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '\n \tgit checkout force-updated &&\n \tgit reset --hard HEAD~ &&\n \ttest_commit --no-tag force-update-new &&\n-\tFORCE_UPDATED_NEW=$(git rev-parse HEAD) &&\n-\n-\tcat >expect <<-EOF &&\n-\t- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n-\t- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n-\t! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n-\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/forced/new-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n-\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n-\tEOF\n-\n-\t# Execute a dry-run fetch first. We do this to assert that the dry-run\n-\t# and non-dry-run fetches produces the same output. Execution of the\n-\t# fetch is expected to fail as we have a rejected reference update.\n-\ttest_must_fail git -C porcelain fetch \\\n-\t\t--porcelain --dry-run --prune origin $refspecs >actual &&\n-\ttest_cmp expect actual &&\n-\n-\t# And now we perform a non-dry-run fetch.\n-\ttest_must_fail git -C porcelain fetch \\\n-\t\t--porcelain --prune origin $refspecs >actual 2>stderr &&\n-\ttest_cmp expect actual &&\n-\ttest_must_be_empty stderr\n+\tFORCE_UPDATED_NEW=$(git rev-parse HEAD)\n '\n \n+for opt in off on\n+do\n+\tcase $opt in\n+\ton)\n+\t\topt=--atomic\n+\t\t;;\n+\toff)\n+\t\topt=\n+\t\t;;\n+\tesac\n+\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n+\t\ttest_when_finished \"rm -rf porcelain\" &&\n+\n+\t\t# Clone and pre-seed the repositories. We fetch references into two\n+\t\t# namespaces so that we can test that rejected and force-updated\n+\t\t# references are reported properly.\n+\t\trefspecs=\"refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*\" &&\n+\t\tgit clone preseed.git porcelain &&\n+\t\tgit -C porcelain fetch origin $opt $refspecs &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\t- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n+\t\t- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n+\t\t! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n+\t\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/forced/new-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n+\t\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n+\t\tEOF\n+\n+\t\t# Change the URL of the repository to fetch different references.\n+\t\tgit -C porcelain remote set-url origin .. &&\n+\n+\t\t# Execute a dry-run fetch first. We do this to assert that the dry-run\n+\t\t# and non-dry-run fetches produces the same output. Execution of the\n+\t\t# fetch is expected to fail as we have a rejected reference update.\n+\t\ttest_must_fail git -C porcelain fetch $opt \\\n+\t\t\t--porcelain --dry-run --prune origin $refspecs >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# And now we perform a non-dry-run fetch.\n+\t\ttest_must_fail git -C porcelain fetch $opt \\\n+\t\t\t--porcelain --prune origin $refspecs >actual 2>stderr &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty stderr\n+\t'\n+done\n+\n test_expect_success 'fetch porcelain with multiple remotes' '\n \ttest_when_finished \"rm -rf porcelain\" &&\n \n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485641","messageId":"6fb83a00000563a79f3948f9087c634ae507b9f5.1702556642.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":"cover.1702556642.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 2/2] fetch: no redundant error message for atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T12:33:12Z","receivedAt":"2023-12-14T12:33:21Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nIf an error occurs during an atomic fetch, a redundant error message\nwill appear at the end of do_fetch(). It was introduced in b3a804663c\n(fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).\n\nIn function do_fetch(), a failure message is already shown before the\nretcode is set, so we should not call additional error() at the end of\nthis function.\n\nWe can remove the redundant error() function, because we know that\nthe function ref_transaction_abort() never fails. While we can find a\ncommon pattern for calling ref_transaction_abort() by running command\n\"git grep -A1 ref_transaction_abort\", e.g.:\n\n    if (ref_transaction_abort(transaction, &error))\n        error(\"abort: %s\", error.buf);\n\nWe can fix this issue follow this pattern, and the test case \"fetch\nporcelain output (atomic)\" in t5574 will also be fixed. If in the future\nwe decide that we don't need to check the return value of the function\nref_transaction_abort(), this change can be fixed along with it.\n\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n builtin/fetch.c         | 4 +---\n t/t5574-fetch-output.sh | 2 +-\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fd134ba74d..01a573cf8d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n-\tif (retcode && transaction) {\n-\t\tref_transaction_abort(transaction, &err);\n+\tif (retcode && transaction && ref_transaction_abort(transaction, &err))\n \t\terror(\"%s\", err.buf);\n-\t}\n \n \tdisplay_state_release(&display_state);\n \tclose_fetch_head(&fetch_head);\ndiff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\nindex bc747efefc..8d01e36b3d 100755\n--- a/t/t5574-fetch-output.sh\n+++ b/t/t5574-fetch-output.sh\n@@ -98,7 +98,7 @@ do\n \t\topt=\n \t\t;;\n \tesac\n-\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n+\ttest_expect_success \"fetch porcelain output ${opt:+(atomic)}\" '\n \t\ttest_when_finished \"rm -rf porcelain\" &&\n \n \t\t# Clone and pre-seed the repositories. We fetch references into two\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485712","messageId":"ZXwi0DgfhG4AEE9m@tanuki","threadId":"60402","inReplyTo":"6fb83a00000563a79f3948f9087c634ae507b9f5.1702556642.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v2 2/2] fetch: no redundant error message for atomic fetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-15T09:56:32Z","receivedAt":"2023-12-15T09:56:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 14, 2023 at 08:33:12PM +0800, Jiang Xin wrote:\n> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> \n> If an error occurs during an atomic fetch, a redundant error message\n> will appear at the end of do_fetch(). It was introduced in b3a804663c\n> (fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).\n> \n> In function do_fetch(), a failure message is already shown before the\n> retcode is set, so we should not call additional error() at the end of\n> this function.\n> \n> We can remove the redundant error() function, because we know that\n> the function ref_transaction_abort() never fails.\n\nOkay, so this still suffers from the same issue as discussed in the\nthread at <ZTYue-3gAS1aGXNa@tanuki>, but now it's documented in the\ncommit message. I'm still not convinced that is a good argument to say\nthat the function never fails, and if it ever would it would populate\nthe error message. Especially now where there's churn to introduce the\nnew reftable backend this could change any time.\n\nFor the record, I'm proposing to do something like the following:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fd134ba74d..80b8bc549d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1651,7 +1651,7 @@ static int do_fetch(struct transport *transport,\n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction) {\n-\t\t\tretcode = error(\"%s\", err.buf);\n+\t\t\tretcode = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n@@ -1711,7 +1711,6 @@ static int do_fetch(struct transport *transport,\n \n \t\tretcode = ref_transaction_commit(transaction, &err);\n \t\tif (retcode) {\n-\t\t\terror(\"%s\", err.buf);\n \t\t\tref_transaction_free(transaction);\n \t\t\ttransaction = NULL;\n \t\t\tgoto cleanup;\n@@ -1775,9 +1774,13 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\tif (retcode && err.len)\n+\t\terror(\"%s\", err.buf);\n \tif (retcode && transaction) {\n+\t\tstrbuf_reset(&err);\n \t\tref_transaction_abort(transaction, &err);\n-\t\terror(\"%s\", err.buf);\n+\t\tif (err.len)\n+\t\t\terror(\"%s\", err.buf);\n \t}\n \n \tdisplay_state_release(&display_state);\n\nThis would both fix the issue you observed, but also fixes issues in\ncase the ref backend failed without writing an error message to the\nbuffer. It also fixes issues if there were multiple failures, where we'd\nprint the initial error printed to the buffer twice.\n\nI know this is mostly solidifying us against potential future changes,\nbut if it's comparatively easy like this I don't see much of a reason\nagainst it.\n\nPatrick\n\n> While we can find a\n> common pattern for calling ref_transaction_abort() by running command\n> \"git grep -A1 ref_transaction_abort\", e.g.:\n> \n>     if (ref_transaction_abort(transaction, &error))\n>         error(\"abort: %s\", error.buf);\n> \n> We can fix this issue follow this pattern, and the test case \"fetch\n> porcelain output (atomic)\" in t5574 will also be fixed. If in the future\n> we decide that we don't need to check the return value of the function\n> ref_transaction_abort(), this change can be fixed along with it.\n> \n> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> ---\n>  builtin/fetch.c         | 4 +---\n>  t/t5574-fetch-output.sh | 2 +-\n>  2 files changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index fd134ba74d..01a573cf8d 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,\n>  \t}\n>  \n>  cleanup:\n> -\tif (retcode && transaction) {\n> -\t\tref_transaction_abort(transaction, &err);\n> +\tif (retcode && transaction && ref_transaction_abort(transaction, &err))\n>  \t\terror(\"%s\", err.buf);\n> -\t}\n>  \n>  \tdisplay_state_release(&display_state);\n>  \tclose_fetch_head(&fetch_head);\n> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\n> index bc747efefc..8d01e36b3d 100755\n> --- a/t/t5574-fetch-output.sh\n> +++ b/t/t5574-fetch-output.sh\n> @@ -98,7 +98,7 @@ do\n>  \t\topt=\n>  \t\t;;\n>  \tesac\n> -\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n> +\ttest_expect_success \"fetch porcelain output ${opt:+(atomic)}\" '\n>  \t\ttest_when_finished \"rm -rf porcelain\" &&\n>  \n>  \t\t# Clone and pre-seed the repositories. We fetch references into two\n> -- \n> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n> \n"},{"id":"485713","messageId":"ZXwi2MA-KUxszfGj@tanuki","threadId":"60402","inReplyTo":"210191917bcfa9293622908c291652059576f3e5.1702556642.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-15T09:56:40Z","receivedAt":"2023-12-15T09:56:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:\n> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n[snip]\n> @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '\n>  \tgit checkout force-updated &&\n>  \tgit reset --hard HEAD~ &&\n>  \ttest_commit --no-tag force-update-new &&\n> -\tFORCE_UPDATED_NEW=$(git rev-parse HEAD) &&\n> -\n> -\tcat >expect <<-EOF &&\n> -\t- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n> -\t- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n> -\t  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n> -\t! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n> -\t* $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n> -\t  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n> -\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n> -\t* $ZERO_OID $MAIN_OLD refs/forced/new-branch\n> -\t  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n> -\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n> -\t* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n> -\tEOF\n> -\n> -\t# Execute a dry-run fetch first. We do this to assert that the dry-run\n> -\t# and non-dry-run fetches produces the same output. Execution of the\n> -\t# fetch is expected to fail as we have a rejected reference update.\n> -\ttest_must_fail git -C porcelain fetch \\\n> -\t\t--porcelain --dry-run --prune origin $refspecs >actual &&\n> -\ttest_cmp expect actual &&\n> -\n> -\t# And now we perform a non-dry-run fetch.\n> -\ttest_must_fail git -C porcelain fetch \\\n> -\t\t--porcelain --prune origin $refspecs >actual 2>stderr &&\n> -\ttest_cmp expect actual &&\n> -\ttest_must_be_empty stderr\n> +\tFORCE_UPDATED_NEW=$(git rev-parse HEAD)\n>  '\n>  \n> +for opt in off on\n> +do\n> +\tcase $opt in\n> +\ton)\n> +\t\topt=--atomic\n> +\t\t;;\n> +\toff)\n> +\t\topt=\n> +\t\t;;\n> +\tesac\n\nNit: you could also do `for opt in \"--atomic\" \"\"` directly to get rid of\nthis case statement. Not sure whether this is worth a reroll though,\nprobably not.\n\nPatrick\n"},{"id":"485714","messageId":"CANYiYbGaJjnuVx7wJshgqiwvpGTmdq2JiOe4S_ph1bgiZ7XTJg@mail.gmail.com","threadId":"60402","inReplyTo":"ZXwi2MA-KUxszfGj@tanuki","subject":"Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-15T11:16:36Z","receivedAt":"2023-12-15T11:16:48Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Fri, Dec 15, 2023 at 5:56 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:\n> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> [snip]\n> > @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '\n> >       git checkout force-updated &&\n> >       git reset --hard HEAD~ &&\n> >       test_commit --no-tag force-update-new &&\n> > -     FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&\n> > -\n> > -     cat >expect <<-EOF &&\n> > -     - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n> > -     - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n> > -     ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n> > -     * $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n> > -     + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n> > -     * $ZERO_OID $MAIN_OLD refs/forced/new-branch\n> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n> > -     + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n> > -     * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n> > -     EOF\n> > -\n> > -     # Execute a dry-run fetch first. We do this to assert that the dry-run\n> > -     # and non-dry-run fetches produces the same output. Execution of the\n> > -     # fetch is expected to fail as we have a rejected reference update.\n> > -     test_must_fail git -C porcelain fetch \\\n> > -             --porcelain --dry-run --prune origin $refspecs >actual &&\n> > -     test_cmp expect actual &&\n> > -\n> > -     # And now we perform a non-dry-run fetch.\n> > -     test_must_fail git -C porcelain fetch \\\n> > -             --porcelain --prune origin $refspecs >actual 2>stderr &&\n> > -     test_cmp expect actual &&\n> > -     test_must_be_empty stderr\n> > +     FORCE_UPDATED_NEW=$(git rev-parse HEAD)\n> >  '\n> >\n> > +for opt in off on\n> > +do\n> > +     case $opt in\n> > +     on)\n> > +             opt=--atomic\n> > +             ;;\n> > +     off)\n> > +             opt=\n> > +             ;;\n> > +     esac\n>\n> Nit: you could also do `for opt in \"--atomic\" \"\"` directly to get rid of\n> this case statement. Not sure whether this is worth a reroll though,\n> probably not.\n\nYes, your code is much simpler.\n"},{"id":"485720","messageId":"xmqq1qbnmn02.fsf@gitster.g","threadId":"60402","inReplyTo":"CANYiYbGaJjnuVx7wJshgqiwvpGTmdq2JiOe4S_ph1bgiZ7XTJg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-15T16:47:09Z","receivedAt":"2023-12-15T16:47:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin <worldhello.net@gmail.com> writes:\n\n>> Nit: you could also do `for opt in \"--atomic\" \"\"` directly to get rid of\n>> this case statement. Not sure whether this is worth a reroll though,\n>> probably not.\n>\n> Yes, your code is much simpler.\n\nYup, thanks for careful and helpful reviews, Patrick, on both\npatches.  And of course, thanks, Jiang, for working on them.\n\n"},{"id":"485751","messageId":"cover.1702821462.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":"cover.1702556642.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 0/2] fix fetch atomic error message","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-17T14:11:32Z","receivedAt":"2023-12-17T14:11:37Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\n# Changes since v2:\n\nChanged the patches with help from Patrick.\n\n\n# range-diff v2...v3\n\n1:  0b9865f1df ! 1:  0a6c53de7c t5574: test porcelain output of atomic fetch\n    @@ Commit message\n     \n         In a later commit, we will fix this issue.\n     \n    +    Helped-by: Patrick Steinhardt <ps@pks.im>\n         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n     \n      ## t/t5574-fetch-output.sh ##\n     @@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '\n    @@ t/t5574-fetch-output.sh: test_expect_success 'fetch porcelain output' '\n     +\tFORCE_UPDATED_NEW=$(git rev-parse HEAD)\n      '\n      \n    -+for opt in off on\n    ++for opt in \"\" \"--atomic\"\n     +do\n    -+\tcase $opt in\n    -+\ton)\n    -+\t\topt=--atomic\n    -+\t\t;;\n    -+\toff)\n    -+\t\topt=\n    -+\t\t;;\n    -+\tesac\n     +\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n     +\t\ttest_when_finished \"rm -rf porcelain\" &&\n     +\n2:  e10fa198dd ! 2:  a8a7658fb2 fetch: no redundant error message for atomic fetch\n    @@ Commit message\n         will appear at the end of do_fetch(). It was introduced in b3a804663c\n         (fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).\n     \n    -    In function do_fetch(), a failure message is already shown before the\n    -    retcode is set, so we should not call additional error() at the end of\n    -    this function.\n    +    Because a failure message is displayed before setting retcode in the\n    +    function do_fetch(), calling error() on the err message at the end of\n    +    this function may result in redundant or empty error message to be\n    +    displayed.\n     \n         We can remove the redundant error() function, because we know that\n         the function ref_transaction_abort() never fails. While we can find a\n    @@ Commit message\n             if (ref_transaction_abort(transaction, &error))\n                 error(\"abort: %s\", error.buf);\n     \n    -    We can fix this issue follow this pattern, and the test case \"fetch\n    -    porcelain output (atomic)\" in t5574 will also be fixed. If in the future\n    -    we decide that we don't need to check the return value of the function\n    -    ref_transaction_abort(), this change can be fixed along with it.\n    +    Following this pattern, we can tolerate the return value of the function\n    +    ref_transaction_abort() being changed in the future. We also delay the\n    +    output of the err message to the end of do_fetch() to reduce redundant\n    +    code. With these changes, the test case \"fetch porcelain output\n    +    (atomic)\" in t5574 will also be fixed.\n     \n    +    Helped-by: Patrick Steinhardt <ps@pks.im>\n         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n     \n      ## builtin/fetch.c ##\n    +@@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n    + \tif (atomic_fetch) {\n    + \t\ttransaction = ref_transaction_begin(&err);\n    + \t\tif (!transaction) {\n    +-\t\t\tretcode = error(\"%s\", err.buf);\n    ++\t\t\tretcode = -1;\n    + \t\t\tgoto cleanup;\n    + \t\t}\n    + \t}\n    +@@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n    + \n    + \t\tretcode = ref_transaction_commit(transaction, &err);\n    + \t\tif (retcode) {\n    +-\t\t\terror(\"%s\", err.buf);\n    + \t\t\tref_transaction_free(transaction);\n    + \t\t\ttransaction = NULL;\n    + \t\t\tgoto cleanup;\n     @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n      \t}\n      \n      cleanup:\n     -\tif (retcode && transaction) {\n     -\t\tref_transaction_abort(transaction, &err);\n    -+\tif (retcode && transaction && ref_transaction_abort(transaction, &err))\n    - \t\terror(\"%s\", err.buf);\n    --\t}\n    +-\t\terror(\"%s\", err.buf);\n    ++\tif (retcode) {\n    ++\t\tif (err.len) {\n    ++\t\t\terror(\"%s\", err.buf);\n    ++\t\t\tstrbuf_reset(&err);\n    ++\t\t}\n    ++\t\tif (transaction && ref_transaction_abort(transaction, &err) &&\n    ++\t\t    err.len)\n    ++\t\t\terror(\"%s\", err.buf);\n    + \t}\n      \n      \tdisplay_state_release(&display_state);\n\n\nJiang Xin (2):\n  t5574: test porcelain output of atomic fetch\n  fetch: no redundant error message for atomic fetch\n\n builtin/fetch.c         | 14 ++++---\n t/t5574-fetch-output.sh | 89 +++++++++++++++++++++++------------------\n 2 files changed, 59 insertions(+), 44 deletions(-)\n\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485752","messageId":"0a6c53de7c8e951bf8f4bc22809df045a4c4a285.1702821462.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":"cover.1702821462.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 1/2] t5574: test porcelain output of atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-17T14:11:33Z","receivedAt":"2023-12-17T14:11:38Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nThe test case \"fetch porcelain output\" checks output of the fetch\ncommand. The error output must be empty with the follow assertion:\n\n    test_must_be_empty stderr\n\nBut this assertion fails if using atomic fetch. Refactor this test case\nto use different fetch options by splitting it into three test cases.\n\n  1. \"setup for fetch porcelain output\".\n\n  2. \"fetch porcelain output\": for non-atomic fetch.\n\n  3. \"fetch porcelain output (atomic)\": for atomic fetch.\n\nAdd new command \"test_commit ...\" in the first test case, so that if we\nrun these test cases individually (--run=4-6), \"git rev-parse HEAD~\"\ncommand will work properly. Run the above test cases, we can find that\none test case has a known breakage, as shown below:\n\n    ok 4 - setup for fetch porcelain output\n    ok 5 - fetch porcelain output  # TODO known breakage vanished\n    not ok 6 - fetch porcelain output (atomic) # TODO known breakage\n\nThe failed test case has an error message with only the error prompt but\nno message body, as follows:\n\n    'stderr' is not empty, it contains:\n    error:\n\nIn a later commit, we will fix this issue.\n\nHelped-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n t/t5574-fetch-output.sh | 89 +++++++++++++++++++++++------------------\n 1 file changed, 50 insertions(+), 39 deletions(-)\n\ndiff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\nindex 90e6dcb9a7..b579364c47 100755\n--- a/t/t5574-fetch-output.sh\n+++ b/t/t5574-fetch-output.sh\n@@ -61,11 +61,10 @@ test_expect_success 'fetch compact output' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'fetch porcelain output' '\n-\ttest_when_finished \"rm -rf porcelain\" &&\n-\n+test_expect_success 'setup for fetch porcelain output' '\n \t# Set up a bunch of references that we can use to demonstrate different\n \t# kinds of flag symbols in the output format.\n+\ttest_commit commit-for-porcelain-output &&\n \tMAIN_OLD=$(git rev-parse HEAD) &&\n \tgit branch \"fast-forward\" &&\n \tgit branch \"deleted-branch\" &&\n@@ -74,15 +73,10 @@ test_expect_success 'fetch porcelain output' '\n \tFORCE_UPDATED_OLD=$(git rev-parse HEAD) &&\n \tgit checkout main &&\n \n-\t# Clone and pre-seed the repositories. We fetch references into two\n-\t# namespaces so that we can test that rejected and force-updated\n-\t# references are reported properly.\n-\trefspecs=\"refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*\" &&\n-\tgit clone . porcelain &&\n-\tgit -C porcelain fetch origin $refspecs &&\n+\t# Backup to preseed.git\n+\tgit clone --mirror . preseed.git &&\n \n-\t# Now that we have set up the client repositories we can change our\n-\t# local references.\n+\t# Continue changing our local references.\n \tgit branch new-branch &&\n \tgit branch -d deleted-branch &&\n \tgit checkout fast-forward &&\n@@ -91,36 +85,53 @@ test_expect_success 'fetch porcelain output' '\n \tgit checkout force-updated &&\n \tgit reset --hard HEAD~ &&\n \ttest_commit --no-tag force-update-new &&\n-\tFORCE_UPDATED_NEW=$(git rev-parse HEAD) &&\n-\n-\tcat >expect <<-EOF &&\n-\t- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n-\t- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n-\t! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n-\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/forced/new-branch\n-\t  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n-\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n-\t* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n-\tEOF\n-\n-\t# Execute a dry-run fetch first. We do this to assert that the dry-run\n-\t# and non-dry-run fetches produces the same output. Execution of the\n-\t# fetch is expected to fail as we have a rejected reference update.\n-\ttest_must_fail git -C porcelain fetch \\\n-\t\t--porcelain --dry-run --prune origin $refspecs >actual &&\n-\ttest_cmp expect actual &&\n-\n-\t# And now we perform a non-dry-run fetch.\n-\ttest_must_fail git -C porcelain fetch \\\n-\t\t--porcelain --prune origin $refspecs >actual 2>stderr &&\n-\ttest_cmp expect actual &&\n-\ttest_must_be_empty stderr\n+\tFORCE_UPDATED_NEW=$(git rev-parse HEAD)\n '\n \n+for opt in \"\" \"--atomic\"\n+do\n+\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n+\t\ttest_when_finished \"rm -rf porcelain\" &&\n+\n+\t\t# Clone and pre-seed the repositories. We fetch references into two\n+\t\t# namespaces so that we can test that rejected and force-updated\n+\t\t# references are reported properly.\n+\t\trefspecs=\"refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*\" &&\n+\t\tgit clone preseed.git porcelain &&\n+\t\tgit -C porcelain fetch origin $opt $refspecs &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\t- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch\n+\t\t- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward\n+\t\t! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/unforced/new-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward\n+\t\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/forced/new-branch\n+\t\t  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward\n+\t\t+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated\n+\t\t* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch\n+\t\tEOF\n+\n+\t\t# Change the URL of the repository to fetch different references.\n+\t\tgit -C porcelain remote set-url origin .. &&\n+\n+\t\t# Execute a dry-run fetch first. We do this to assert that the dry-run\n+\t\t# and non-dry-run fetches produces the same output. Execution of the\n+\t\t# fetch is expected to fail as we have a rejected reference update.\n+\t\ttest_must_fail git -C porcelain fetch $opt \\\n+\t\t\t--porcelain --dry-run --prune origin $refspecs >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# And now we perform a non-dry-run fetch.\n+\t\ttest_must_fail git -C porcelain fetch $opt \\\n+\t\t\t--porcelain --prune origin $refspecs >actual 2>stderr &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty stderr\n+\t'\n+done\n+\n test_expect_success 'fetch porcelain with multiple remotes' '\n \ttest_when_finished \"rm -rf porcelain\" &&\n \n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485753","messageId":"a8a7658fb2e1d5194e78608ea336ce6df94045ce.1702821462.git.zhiyou.jx@alibaba-inc.com","threadId":"60402","inReplyTo":"cover.1702821462.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 2/2] fetch: no redundant error message for atomic fetch","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-17T14:11:34Z","receivedAt":"2023-12-17T14:11:39Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nIf an error occurs during an atomic fetch, a redundant error message\nwill appear at the end of do_fetch(). It was introduced in b3a804663c\n(fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).\n\nBecause a failure message is displayed before setting retcode in the\nfunction do_fetch(), calling error() on the err message at the end of\nthis function may result in redundant or empty error message to be\ndisplayed.\n\nWe can remove the redundant error() function, because we know that\nthe function ref_transaction_abort() never fails. While we can find a\ncommon pattern for calling ref_transaction_abort() by running command\n\"git grep -A1 ref_transaction_abort\", e.g.:\n\n    if (ref_transaction_abort(transaction, &error))\n        error(\"abort: %s\", error.buf);\n\nFollowing this pattern, we can tolerate the return value of the function\nref_transaction_abort() being changed in the future. We also delay the\noutput of the err message to the end of do_fetch() to reduce redundant\ncode. With these changes, the test case \"fetch porcelain output\n(atomic)\" in t5574 will also be fixed.\n\nHelped-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n builtin/fetch.c         | 14 +++++++++-----\n t/t5574-fetch-output.sh |  2 +-\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fd134ba74d..a284b970ef 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1651,7 +1651,7 @@ static int do_fetch(struct transport *transport,\n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction) {\n-\t\t\tretcode = error(\"%s\", err.buf);\n+\t\t\tretcode = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n@@ -1711,7 +1711,6 @@ static int do_fetch(struct transport *transport,\n \n \t\tretcode = ref_transaction_commit(transaction, &err);\n \t\tif (retcode) {\n-\t\t\terror(\"%s\", err.buf);\n \t\t\tref_transaction_free(transaction);\n \t\t\ttransaction = NULL;\n \t\t\tgoto cleanup;\n@@ -1775,9 +1774,14 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n-\tif (retcode && transaction) {\n-\t\tref_transaction_abort(transaction, &err);\n-\t\terror(\"%s\", err.buf);\n+\tif (retcode) {\n+\t\tif (err.len) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t}\n+\t\tif (transaction && ref_transaction_abort(transaction, &err) &&\n+\t\t    err.len)\n+\t\t\terror(\"%s\", err.buf);\n \t}\n \n \tdisplay_state_release(&display_state);\ndiff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh\nindex b579364c47..1400ef14cd 100755\n--- a/t/t5574-fetch-output.sh\n+++ b/t/t5574-fetch-output.sh\n@@ -90,7 +90,7 @@ test_expect_success 'setup for fetch porcelain output' '\n \n for opt in \"\" \"--atomic\"\n do\n-\ttest_expect_failure \"fetch porcelain output ${opt:+(atomic)}\" '\n+\ttest_expect_success \"fetch porcelain output ${opt:+(atomic)}\" '\n \t\ttest_when_finished \"rm -rf porcelain\" &&\n \n \t\t# Clone and pre-seed the repositories. We fetch references into two\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485766","messageId":"ZX__e7VjyLXIl-uV@tanuki","threadId":"60402","inReplyTo":"cover.1702821462.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v3 0/2] fix fetch atomic error message","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-18T08:14:51Z","receivedAt":"2023-12-18T08:14:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Dec 17, 2023 at 10:11:32PM +0800, Jiang Xin wrote:\n> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> \n> # Changes since v2:\n> \n> Changed the patches with help from Patrick.\n> \n\nThanks, this version looks good to me!\n\nPatrick\n"}]}