{"thread":{"id":"51416","subject":"[PATCH] transport-helper: enforce atomic in push_refs_with_push","startedAt":"2019-07-02T00:54:04Z","lastAt":"2019-07-27T16:11:56Z","messageCount":45,"participants":["Emily Shaffer","Johannes Schindelin","Junio C Hamano","SZEDER Gábor","Carlo Arenas","Jonathan Nieder","Bryan Turner","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378469","messageId":"20190702005340.66615-1-emilyshaffer@google.com","threadId":"51416","inReplyTo":null,"subject":"[PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-02T00:53:40Z","receivedAt":"2019-07-02T00:54:04Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach transport-helper how to notice if skipping a ref during push would\nviolate atomicity on the client side. We notice that a ref would be\nrejected, and choose not to send it, but don't notice that if the client\nhas asked for --atomic we are violating atomicity if all the other\npushes we are sending would succeed. Asking the server end to uphold\natomicity wouldn't work here as the server doesn't have any idea that we\ntried to update a ref that's broken.\n\nThe added test-case is a succinct way to reproduce this issue that fails\ntoday. The same steps work fine when we aren't using a transport-helper\nto get to the upstream, i.e. when we've added a local repository as a\nremote:\n\n  git remote add ~/upstream upstream\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\n t/t5541-http-push-smart.sh | 58 ++++++++++++++++++++++++++++++++++++++\n transport-helper.c         |  6 ++++\n transport.c                | 15 +++++++++-\n 3 files changed, 78 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 8ef8763e06..b57f6d480f 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -177,6 +177,64 @@ test_expect_success 'push (chunked)' '\n \t test $HEAD = $(git rev-parse --verify HEAD))\n '\n \n+test_expect_success 'push --atomic also prevents branch creation' '\n+\t# Make up/master\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n+\tgit init --bare \"$d\" &&\n+\tgit --git-dir=\"$d\" config http.receivepack true &&\n+\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n+\ttest_commit atomic1 &&\n+\ttest_commit atomic2 &&\n+\tgit push \"$up\" master &&\n+\t# Make master incompatible with up/master\n+\tgit reset --hard HEAD^ &&\n+\t# Add a new branch\n+\tgit branch atomic &&\n+\t# --atomic should roll back creation of up/atomic\n+\ttest_must_fail git push --atomic \"$up\" master atomic &&\n+\tgit ls-remote \"$up\" >up-remotes &&\n+\ttest_must_fail grep atomic up-remotes\n+'\n+\n+test_expect_success 'push --atomic shows all failed refs' '\n+\t# Make up/master, up/allrefs\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n+\tgit init --bare \"$d\" &&\n+\tgit --git-dir=\"$d\" config http.receivepack true &&\n+\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n+\ttest_commit allrefs1 &&\n+\ttest_commit allrefs2 &&\n+\tgit branch allrefs &&\n+\tgit push \"$up\" master allrefs &&\n+\t# Make master and allrefs incompatible with up/master, up/allrefs\n+\tgit checkout allrefs &&\n+\tgit reset --hard HEAD^ &&\n+\tgit checkout master &&\n+\tgit reset --hard HEAD^ &&\n+\t# --atomic should complain about both master and allrefs\n+\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n+\tgrep master output &&\n+\tgrep allrefs output\n+'\n+\n+test_expect_success 'push --atomic indicates collateral failures' '\n+\t# Make up/master, up/collateral\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-collateral.git &&\n+\tgit init --bare \"$d\" &&\n+\tgit --git-dir=\"$d\" config http.receivepack true &&\n+\tup=\"$HTTPD_URL\"/smart/atomic-collateral.git &&\n+\ttest_commit collateral1 &&\n+\ttest_commit collateral2 &&\n+\tgit branch collateral &&\n+\tgit push \"$up\" master collateral &&\n+\t# Make master incompatible with up/master\n+\tgit reset --hard HEAD^ &&\n+\t# --atomic should mention collateral was OK but failed anyway\n+\ttest_must_fail git push --atomic \"$up\" master collateral >&output &&\n+\tgrep \"master -> master\" output &&\n+\tgrep \"collateral -> collateral\" output\n+'\n+\n test_expect_success 'push --all can push to empty repo' '\n \td=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&\n \tgit init --bare \"$d\" &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex c7e17ec9cb..6b05a88faf 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,\n {\n \tint force_all = flags & TRANSPORT_PUSH_FORCE;\n \tint mirror = flags & TRANSPORT_PUSH_MIRROR;\n+\tint atomic = flags & TRANSPORT_PUSH_ATOMIC;\n \tstruct helper_data *data = transport->data;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref;\n@@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\tcase REF_STATUS_REJECT_STALE:\n \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n+\t\t\tif (atomic) {\n+\t\t\t\tstring_list_clear(&cas_options, 0);\n+\t\t\t\treturn 0;\n+\t\t\t} else\n+\t\t\t\tcontinue;\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport.c b/transport.c\nindex f1fcd2c4b0..f4d6b38f9d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1226,10 +1226,23 @@ int transport_push(struct repository *r,\n \t\terr = push_had_errors(remote_refs);\n \t\tret = push_ret | err;\n \n-\t\tif (!quiet || err)\n+\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n+\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n+\t\t\t\tswitch (it->status) {\n+\t\t\t\tcase REF_STATUS_NONE:\n+\t\t\t\tcase REF_STATUS_UPTODATE:\n+\t\t\t\tcase REF_STATUS_OK:\n+\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n+\t\t\t\tdefault:\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t}\n+\n+\t\tif (!quiet || err) {\n \t\t\ttransport_print_push_status(transport->url, remote_refs,\n \t\t\t\t\tverbose | porcelain, porcelain,\n \t\t\t\t\treject_reasons);\n+\t\t}\n \n \t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n \t\t\tset_upstreams(transport, remote_refs, pretend);\n-- \n2.22.0.410.gd8fdbe21b5-goog\n\n"},{"id":"378494","messageId":"nycvar.QRO.7.76.6.1907021540330.48@tvgsbejvaqbjf.bet","threadId":"51416","inReplyTo":"20190702005340.66615-1-emilyshaffer@google.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-02T13:51:55Z","receivedAt":"2019-07-02T13:52:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Emily,\n\nOn Mon, 1 Jul 2019, Emily Shaffer wrote:\n\n> Teach transport-helper how to notice if skipping a ref during push would\n> violate atomicity on the client side. We notice that a ref would be\n> rejected, and choose not to send it, but don't notice that if the client\n> has asked for --atomic we are violating atomicity if all the other\n> pushes we are sending would succeed. Asking the server end to uphold\n> atomicity wouldn't work here as the server doesn't have any idea that we\n> tried to update a ref that's broken.\n\nMakes sense.\n\n> The added test-case is a succinct way to reproduce this issue that fails\n> today. The same steps work fine when we aren't using a transport-helper\n> to get to the upstream, i.e. when we've added a local repository as a\n> remote:\n>\n>   git remote add ~/upstream upstream\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  t/t5541-http-push-smart.sh | 58 ++++++++++++++++++++++++++++++++++++++\n>  transport-helper.c         |  6 ++++\n>  transport.c                | 15 +++++++++-\n>  3 files changed, 78 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index 8ef8763e06..b57f6d480f 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -177,6 +177,64 @@ test_expect_success 'push (chunked)' '\n>  \t test $HEAD = $(git rev-parse --verify HEAD))\n>  '\n>\n> +test_expect_success 'push --atomic also prevents branch creation' '\n> +\t# Make up/master\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n> +\tgit init --bare \"$d\" &&\n> +\tgit --git-dir=\"$d\" config http.receivepack true &&\n\nWhy not `-C \"$d\"`? And why not `test_config`?\n\n> +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> +\ttest_commit atomic1 &&\n> +\ttest_commit atomic2 &&\n> +\tgit push \"$up\" master &&\n\nIt would be more succinct to do a `git clone --bare . \"$d\"` here, instead\nof a `git init --bare` and a `git push` no?\n\n> +\t# Make master incompatible with up/master\n> +\tgit reset --hard HEAD^ &&\n> +\t# Add a new branch\n> +\tgit branch atomic &&\n> +\t# --atomic should roll back creation of up/atomic\n> +\ttest_must_fail git push --atomic \"$up\" master atomic &&\n> +\tgit ls-remote \"$up\" >up-remotes &&\n> +\ttest_must_fail grep atomic up-remotes\n\nWhy not `test_must_fail git -C \"$d\" rev-parse refs/heads/atomic`?\n\n> +'\n> +\n> +test_expect_success 'push --atomic shows all failed refs' '\n> +\t# Make up/master, up/allrefs\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n> +\tgit init --bare \"$d\" &&\n> +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n> +\ttest_commit allrefs1 &&\n> +\ttest_commit allrefs2 &&\n> +\tgit branch allrefs &&\n> +\tgit push \"$up\" master allrefs &&\n> +\t# Make master and allrefs incompatible with up/master, up/allrefs\n> +\tgit checkout allrefs &&\n> +\tgit reset --hard HEAD^ &&\n> +\tgit checkout master &&\n> +\tgit reset --hard HEAD^ &&\n> +\t# --atomic should complain about both master and allrefs\n> +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n> +\tgrep master output &&\n> +\tgrep allrefs output\n> +'\n\nI have the impression that the setup these two new test cases perform are\n_very_ similar, making it most likely that a combined test case would be\nmore succinct, yet still complete and easily readable.\n\n> +\n> +test_expect_success 'push --atomic indicates collateral failures' '\n> +\t# Make up/master, up/collateral\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-collateral.git &&\n> +\tgit init --bare \"$d\" &&\n> +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> +\tup=\"$HTTPD_URL\"/smart/atomic-collateral.git &&\n> +\ttest_commit collateral1 &&\n> +\ttest_commit collateral2 &&\n> +\tgit branch collateral &&\n> +\tgit push \"$up\" master collateral &&\n> +\t# Make master incompatible with up/master\n> +\tgit reset --hard HEAD^ &&\n> +\t# --atomic should mention collateral was OK but failed anyway\n> +\ttest_must_fail git push --atomic \"$up\" master collateral >&output &&\n> +\tgrep \"master -> master\" output &&\n> +\tgrep \"collateral -> collateral\" output\n> +'\n\nSame here.\n\n> +\n>  test_expect_success 'push --all can push to empty repo' '\n>  \td=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&\n>  \tgit init --bare \"$d\" &&\n> diff --git a/transport-helper.c b/transport-helper.c\n> index c7e17ec9cb..6b05a88faf 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,\n>  {\n>  \tint force_all = flags & TRANSPORT_PUSH_FORCE;\n>  \tint mirror = flags & TRANSPORT_PUSH_MIRROR;\n> +\tint atomic = flags & TRANSPORT_PUSH_ATOMIC;\n>  \tstruct helper_data *data = transport->data;\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tstruct ref *ref;\n> @@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,\n>  \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n>  \t\tcase REF_STATUS_REJECT_STALE:\n>  \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n> +\t\t\tif (atomic) {\n> +\t\t\t\tstring_list_clear(&cas_options, 0);\n> +\t\t\t\treturn 0;\n> +\t\t\t} else\n> +\t\t\t\tcontinue;\n>  \t\tcase REF_STATUS_UPTODATE:\n>  \t\t\tcontinue;\n>  \t\tdefault:\n> diff --git a/transport.c b/transport.c\n> index f1fcd2c4b0..f4d6b38f9d 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1226,10 +1226,23 @@ int transport_push(struct repository *r,\n>  \t\terr = push_had_errors(remote_refs);\n>  \t\tret = push_ret | err;\n>\n> -\t\tif (!quiet || err)\n> +\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n\nThis looks funny. And it does so only...\n\n> +\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n> +\t\t\t\tswitch (it->status) {\n> +\t\t\t\tcase REF_STATUS_NONE:\n> +\t\t\t\tcase REF_STATUS_UPTODATE:\n> +\t\t\t\tcase REF_STATUS_OK:\n> +\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> +\t\t\t\tdefault:\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!quiet || err) {\n\n... because a curly was introduced around a single-liner. This adds\nunnecessary noise to the patch.\n\nThis easily distracts reviewers like myself from more important questions\nsuch as: why was this conditional switch added before this conditional\nblock, does it intend to influence the printed push status? Ah, yes, of\ncourse, even if `it->status` is changed, it actually modifies the data\nto which `remote_refs` points. So yes, this has to be done here.\n\n>  \t\t\ttransport_print_push_status(transport->url, remote_refs,\n>  \t\t\t\t\tverbose | porcelain, porcelain,\n>  \t\t\t\t\treject_reasons);\n> +\t\t}\n>\n>  \t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n>  \t\t\tset_upstreams(transport, remote_refs, pretend);\n> --\n\nApart from minor nits, I like it. Thanks,\nDscho\n\n> 2.22.0.410.gd8fdbe21b5-goog\n>\n>\n"},{"id":"378509","messageId":"xmqq5zok47ia.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"nycvar.QRO.7.76.6.1907021540330.48@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-02T18:27:09Z","receivedAt":"2019-07-02T18:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> +\t# Make master incompatible with up/master\n>> +\tgit reset --hard HEAD^ &&\n>> +\t# Add a new branch\n>> +\tgit branch atomic &&\n>> +\t# --atomic should roll back creation of up/atomic\n>> +\ttest_must_fail git push --atomic \"$up\" master atomic &&\n>> +\tgit ls-remote \"$up\" >up-remotes &&\n>> +\ttest_must_fail grep atomic up-remotes\n>\n> Why not `test_must_fail git -C \"$d\" rev-parse refs/heads/atomic`?\n\nThey check different expectations.\n\nBetween making sure that the state observable in the resulting\nrepository matches what should have been left and checking what the\ncommand says it did to its output, I agree that the former is a more\nrobust way to test the commands.\n\nWe obviously could test both, though ;-)\n\n"},{"id":"378512","messageId":"xmqq1rz845pc.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190702005340.66615-1-emilyshaffer@google.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-02T19:06:07Z","receivedAt":"2019-07-02T19:06:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index c7e17ec9cb..6b05a88faf 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,\n>  {\n>  \tint force_all = flags & TRANSPORT_PUSH_FORCE;\n>  \tint mirror = flags & TRANSPORT_PUSH_MIRROR;\n> +\tint atomic = flags & TRANSPORT_PUSH_ATOMIC;\n>  \tstruct helper_data *data = transport->data;\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tstruct ref *ref;\n> @@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,\n>  \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n>  \t\tcase REF_STATUS_REJECT_STALE:\n>  \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n> +\t\t\tif (atomic) {\n> +\t\t\t\tstring_list_clear(&cas_options, 0);\n> +\t\t\t\treturn 0;\n> +\t\t\t} else\n> +\t\t\t\tcontinue;\n\nAh, this looks vaguely familiar.  Thanks for resurrecting the topic.\n\nThe clearing is merely to avoid leaks, and the primary change to the\nfunction is to immediately return 0.\n\n>  \t\tcase REF_STATUS_UPTODATE:\n>  \t\t\tcontinue;\n>  \t\tdefault:\n> diff --git a/transport.c b/transport.c\n> index f1fcd2c4b0..f4d6b38f9d 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1226,10 +1226,23 @@ int transport_push(struct repository *r,\n>  \t\terr = push_had_errors(remote_refs);\n>  \t\tret = push_ret | err;\n\nHere, before reporting the push result, when we are doing ATOMIC,\nwe tweak the result we are going to report to atomic-push-failed.\n\n> +\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n> +\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n> +\t\t\t\tswitch (it->status) {\n> +\t\t\t\tcase REF_STATUS_NONE:\n> +\t\t\t\tcase REF_STATUS_UPTODATE:\n> +\t\t\t\tcase REF_STATUS_OK:\n> +\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> +\t\t\t\tdefault:\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\t\t}\n\nThis roughly corresponds to what send-pack.c::atomic_push_failure()\ndoes.  Here, we avoid overwriting a status that already signals a\nfailure.  The list of \"good\" statuses used here match what is used\nat the end of send_pack.c::send_pack(), which decides the final\noutcome of \"git push\" for the native transport.\n\nLooks good.\n\nBy the way, I rearranged the patch as I happen to agree with Dscho\nthat the additional {} was unwarranted and made it harder to review.\nIt is clear that we need to tweak the status before reporting.\n\n>\n> \t\tif (!quiet || err)\n>  \t\t\ttransport_print_push_status(transport->url, remote_refs,\n>  \t\t\t\t\tverbose | porcelain, porcelain,\n>  \t\t\t\t\treject_reasons);\n>  \n>  \t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n>  \t\t\tset_upstreams(transport, remote_refs, pretend);\n"},{"id":"378519","messageId":"xmqqk1d02nv5.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190702005340.66615-1-emilyshaffer@google.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-02T20:16:46Z","receivedAt":"2019-07-03T00:35:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> +\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n> +\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n> +\t\t\t\tswitch (it->status) {\n> +\t\t\t\tcase REF_STATUS_NONE:\n> +\t\t\t\tcase REF_STATUS_UPTODATE:\n> +\t\t\t\tcase REF_STATUS_OK:\n> +\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> +\t\t\t\tdefault:\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\t\t}\n\n\nLet's write this more like so\n\n\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n\t\t\t\tswitch (it->status) {\n\t\t\t\tcase REF_STATUS_NONE:\n\t\t\t\tcase REF_STATUS_UPTODATE:\n\t\t\t\tcase REF_STATUS_OK:\n\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n\t\t\t\t\tbreak;\n\t\t\t\tdefault:\n\t\t\t\t\tbreak;\n\t\t\t\t}\n\t\t}\n\nto prevent compilers from giving \"implicit fallthru\" warnings.\n"},{"id":"378520","messageId":"xmqqa7dw2k49.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190702005340.66615-1-emilyshaffer@google.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-02T21:37:42Z","receivedAt":"2019-07-03T00:35:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> Teach transport-helper how to notice if skipping a ref during push would\n> violate atomicity on the client side. We notice that a ref would be\n> rejected, and choose not to send it, but don't notice that if the client\n> has asked for --atomic we are violating atomicity if all the other\n> pushes we are sending would succeed. Asking the server end to uphold\n> atomicity wouldn't work here as the server doesn't have any idea that we\n> tried to update a ref that's broken.\n>\n> The added test-case is a succinct way to reproduce this issue that fails\n> today. The same steps work fine when we aren't using a transport-helper\n> to get to the upstream, i.e. when we've added a local repository as a\n> remote:\n>\n>   git remote add ~/upstream upstream\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  t/t5541-http-push-smart.sh | 58 ++++++++++++++++++++++++++++++++++++++\n>  transport-helper.c         |  6 ++++\n>  transport.c                | 15 +++++++++-\n>  3 files changed, 78 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index 8ef8763e06..b57f6d480f 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -177,6 +177,64 @@ test_expect_success 'push (chunked)' '\n>  \t test $HEAD = $(git rev-parse --verify HEAD))\n>  '\n>  \n> +test_expect_success 'push --atomic also prevents branch creation' '\n> +\t# Make up/master\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n> +\tgit init --bare \"$d\" &&\n> +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> +\ttest_commit atomic1 &&\n> +\ttest_commit atomic2 &&\n> +\tgit push \"$up\" master &&\n> +\t# Make master incompatible with up/master\n> +\tgit reset --hard HEAD^ &&\n> +\t# Add a new branch\n> +\tgit branch atomic &&\n> +\t# --atomic should roll back creation of up/atomic\n> +\ttest_must_fail git push --atomic \"$up\" master atomic &&\n> +\tgit ls-remote \"$up\" >up-remotes &&\n> +\ttest_must_fail grep atomic up-remotes\n\nDon't use test_must_fail on non-git things.  We are not in the\nbusiness of catching segfaulting system programs.\n\n> +'\n> +\n> +test_expect_success 'push --atomic shows all failed refs' '\n> +\t# Make up/master, up/allrefs\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n> +\tgit init --bare \"$d\" &&\n> +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n> +\ttest_commit allrefs1 &&\n> +\ttest_commit allrefs2 &&\n> +\tgit branch allrefs &&\n> +\tgit push \"$up\" master allrefs &&\n> +\t# Make master and allrefs incompatible with up/master, up/allrefs\n> +\tgit checkout allrefs &&\n> +\tgit reset --hard HEAD^ &&\n> +\tgit checkout master &&\n> +\tgit reset --hard HEAD^ &&\n> +\t# --atomic should complain about both master and allrefs\n> +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n\nDon't rely on \">&output\", which is an unnecessary bash-ism here.  It\nbreaks test run under shells like dash.\n\n\t>output 2>&1\n\nshould be OK.\n\n> +\tgrep master output &&\n> +\tgrep allrefs output\n> +'\n> +\n> +test_expect_success 'push --atomic indicates collateral failures' '\n> +\t# Make up/master, up/collateral\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-collateral.git &&\n> +\tgit init --bare \"$d\" &&\n> +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> +\tup=\"$HTTPD_URL\"/smart/atomic-collateral.git &&\n> +\ttest_commit collateral1 &&\n> +\ttest_commit collateral2 &&\n> +\tgit branch collateral &&\n> +\tgit push \"$up\" master collateral &&\n> +\t# Make master incompatible with up/master\n> +\tgit reset --hard HEAD^ &&\n> +\t# --atomic should mention collateral was OK but failed anyway\n> +\ttest_must_fail git push --atomic \"$up\" master collateral >&output &&\n\nDitto.\n\n> +\tgrep \"master -> master\" output &&\n> +\tgrep \"collateral -> collateral\" output\n> +'\n> +\n\n"},{"id":"378521","messageId":"20190703000957.GB121233@google.com","threadId":"51416","inReplyTo":"xmqqk1d02nv5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-03T00:09:57Z","receivedAt":"2019-07-03T00:36:56Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jul 02, 2019 at 01:16:46PM -0700, Junio C Hamano wrote:\n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> > +\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n> > +\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n> > +\t\t\t\tswitch (it->status) {\n> > +\t\t\t\tcase REF_STATUS_NONE:\n> > +\t\t\t\tcase REF_STATUS_UPTODATE:\n> > +\t\t\t\tcase REF_STATUS_OK:\n> > +\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> > +\t\t\t\tdefault:\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t\t}\n> > +\t\t}\n> \n> \n> Let's write this more like so\n> \n> \t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n> \t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n> \t\t\t\tswitch (it->status) {\n> \t\t\t\tcase REF_STATUS_NONE:\n> \t\t\t\tcase REF_STATUS_UPTODATE:\n> \t\t\t\tcase REF_STATUS_OK:\n> \t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> \t\t\t\t\tbreak;\n> \t\t\t\tdefault:\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t}\n> \n> to prevent compilers from giving \"implicit fallthru\" warnings.\n\nDone, thanks.\n"},{"id":"378522","messageId":"20190703000844.GA121233@google.com","threadId":"51416","inReplyTo":"xmqqa7dw2k49.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-03T00:08:44Z","receivedAt":"2019-07-03T00:39:40Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:\n> >  \n> > +test_expect_success 'push --atomic also prevents branch creation' '\n> > +\t# Make up/master\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> > +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> > +\ttest_commit atomic1 &&\n> > +\ttest_commit atomic2 &&\n> > +\tgit push \"$up\" master &&\n> > +\t# Make master incompatible with up/master\n> > +\tgit reset --hard HEAD^ &&\n> > +\t# Add a new branch\n> > +\tgit branch atomic &&\n> > +\t# --atomic should roll back creation of up/atomic\n> > +\ttest_must_fail git push --atomic \"$up\" master atomic &&\n> > +\tgit ls-remote \"$up\" >up-remotes &&\n> > +\ttest_must_fail grep atomic up-remotes\n> \n> Don't use test_must_fail on non-git things.  We are not in the\n> business of catching segfaulting system programs.\n\nDone:\n  ! grep atomic up-remotes\n\n> \n> > +'\n> > +\n> > +test_expect_success 'push --atomic shows all failed refs' '\n> > +\t# Make up/master, up/allrefs\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> > +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n> > +\ttest_commit allrefs1 &&\n> > +\ttest_commit allrefs2 &&\n> > +\tgit branch allrefs &&\n> > +\tgit push \"$up\" master allrefs &&\n> > +\t# Make master and allrefs incompatible with up/master, up/allrefs\n> > +\tgit checkout allrefs &&\n> > +\tgit reset --hard HEAD^ &&\n> > +\tgit checkout master &&\n> > +\tgit reset --hard HEAD^ &&\n> > +\t# --atomic should complain about both master and allrefs\n> > +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n> \n> Don't rely on \">&output\", which is an unnecessary bash-ism here.  It\n> breaks test run under shells like dash.\n> \n> \t>output 2>&1\n> \n> should be OK.\n\nDone.\n\n> \n> > +\tgrep master output &&\n> > +\tgrep allrefs output\n> > +'\n> > +\n> > +test_expect_success 'push --atomic indicates collateral failures' '\n> > +\t# Make up/master, up/collateral\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-collateral.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> > +\tup=\"$HTTPD_URL\"/smart/atomic-collateral.git &&\n> > +\ttest_commit collateral1 &&\n> > +\ttest_commit collateral2 &&\n> > +\tgit branch collateral &&\n> > +\tgit push \"$up\" master collateral &&\n> > +\t# Make master incompatible with up/master\n> > +\tgit reset --hard HEAD^ &&\n> > +\t# --atomic should mention collateral was OK but failed anyway\n> > +\ttest_must_fail git push --atomic \"$up\" master collateral >&output &&\n> \n> Ditto.\n\nDone.\n\n - Emily\n"},{"id":"378531","messageId":"20190703091030.GP21574@szeder.dev","threadId":"51416","inReplyTo":"xmqqa7dw2k49.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-03T09:10:30Z","receivedAt":"2019-07-03T09:10:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:\n> > +test_expect_success 'push --atomic shows all failed refs' '\n> > +\t# Make up/master, up/allrefs\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> > +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n> > +\ttest_commit allrefs1 &&\n> > +\ttest_commit allrefs2 &&\n> > +\tgit branch allrefs &&\n> > +\tgit push \"$up\" master allrefs &&\n> > +\t# Make master and allrefs incompatible with up/master, up/allrefs\n> > +\tgit checkout allrefs &&\n> > +\tgit reset --hard HEAD^ &&\n> > +\tgit checkout master &&\n> > +\tgit reset --hard HEAD^ &&\n> > +\t# --atomic should complain about both master and allrefs\n> > +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n> \n> Don't rely on \">&output\", which is an unnecessary bash-ism here.  It\n> breaks test run under shells like dash.\n> \n> \t>output 2>&1\n> \n> should be OK.\n\n'2>output' would be a tad better, because those refs should be printed\nto stderr.\n\n"},{"id":"378559","messageId":"xmqqblyb0yw6.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190703091030.GP21574@szeder.dev","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-03T18:13:45Z","receivedAt":"2019-07-03T18:13:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:\n>> > +test_expect_success 'push --atomic shows all failed refs' '\n>> > +\t# Make up/master, up/allrefs\n>> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n>> > +\tgit init --bare \"$d\" &&\n>> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n>> > +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n>> > +\ttest_commit allrefs1 &&\n>> > +\ttest_commit allrefs2 &&\n>> > +\tgit branch allrefs &&\n>> > +\tgit push \"$up\" master allrefs &&\n>> > +\t# Make master and allrefs incompatible with up/master, up/allrefs\n>> > +\tgit checkout allrefs &&\n>> > +\tgit reset --hard HEAD^ &&\n>> > +\tgit checkout master &&\n>> > +\tgit reset --hard HEAD^ &&\n>> > +\t# --atomic should complain about both master and allrefs\n>> > +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n>> \n>> Don't rely on \">&output\", which is an unnecessary bash-ism here.  It\n>> breaks test run under shells like dash.\n>> \n>> \t>output 2>&1\n>> \n>> should be OK.\n>\n> '2>output' would be a tad better, because those refs should be printed\n> to stderr.\n\nYeah; there are many existing uses of \">output 2>&1\" in the same\nscript and I was following the suit.  There also are 2>err and I\nagree that it is more appropriate in this case.\n"},{"id":"378564","messageId":"20190703185535.GC121233@google.com","threadId":"51416","inReplyTo":"nycvar.QRO.7.76.6.1907021540330.48@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-03T18:56:07Z","receivedAt":"2019-07-03T18:56:15Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"> > +test_expect_success 'push --atomic also prevents branch creation' '\n> > +\t# Make up/master\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> \n> Why not `-C \"$d\"`?\nThe example I had found below the new ones used --git-dir, but yeah, there's no\nreason not to use -C instead. Changing.\n\n> And why not `test_config`?\n\nDone, didn't know about it and it's not used in the test I referred to\nwhile writing this one ('push --all can push to empty repo'). Thanks, I\nlearned something new.\n\n> \n> > +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> > +\ttest_commit atomic1 &&\n> > +\ttest_commit atomic2 &&\n> > +\tgit push \"$up\" master &&\n> \n> It would be more succinct to do a `git clone --bare . \"$d\"` here, instead\n> of a `git init --bare` and a `git push` no?\n\nI'm not sure I would say \"more succinct.\" This leaves the test with the\nsame number of lines, but now it says:\n\n  Make some commits\n  Name a Git directory\n  Clone to the new Git directory\n  Do some config on the new Git directory\n  Name a remote URL\n  Change some commits\n  ...\n\nIn my opinion, it's more readable the way it is now:\n\n  {Do some setup stuff.}\n  Name a Git directory\n  Init it\n  Config it\n  Name the remote URL\n  {Do the test stuff.}\n  Make some commits\n  Push some commits\n  Change some commits\n  ...\n\nI did add another comment to separate \"Make 'up'\" and \"Make up/master\",\nwhich I hope expresses my intent in organizing it this way.\n\n> \n> > +\t# Make master incompatible with up/master\n> > +\tgit reset --hard HEAD^ &&\n> > +\t# Add a new branch\n> > +\tgit branch atomic &&\n> > +\t# --atomic should roll back creation of up/atomic\n> > +\ttest_must_fail git push --atomic \"$up\" master atomic &&\n> > +\tgit ls-remote \"$up\" >up-remotes &&\n> > +\ttest_must_fail grep atomic up-remotes\n> \n> Why not `test_must_fail git -C \"$d\" rev-parse refs/heads/atomic`?\n\nSure, changed.\n\n> > > +'\n> > +\n> > +test_expect_success 'push --atomic shows all failed refs' '\n> > +\t# Make up/master, up/allrefs\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> > +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n> > +\ttest_commit allrefs1 &&\n> > +\ttest_commit allrefs2 &&\n> > +\tgit branch allrefs &&\n> > +\tgit push \"$up\" master allrefs &&\n> > +\t# Make master and allrefs incompatible with up/master, up/allrefs\n> > +\tgit checkout allrefs &&\n> > +\tgit reset --hard HEAD^ &&\n> > +\tgit checkout master &&\n> > +\tgit reset --hard HEAD^ &&\n> > +\t# --atomic should complain about both master and allrefs\n> > +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n> > +\tgrep master output &&\n> > +\tgrep allrefs output\n> > +'\n> \n> I have the impression that the setup these two new test cases perform are\n> _very_ similar, making it most likely that a combined test case would be\n> more succinct, yet still complete and easily readable.\n\n(Junio replied to this downthread... I have more to ask too.)\n\n> \n> > +\n> > +test_expect_success 'push --atomic indicates collateral failures' '\n> > +\t# Make up/master, up/collateral\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-collateral.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> > +\tup=\"$HTTPD_URL\"/smart/atomic-collateral.git &&\n> > +\ttest_commit collateral1 &&\n> > +\ttest_commit collateral2 &&\n> > +\tgit branch collateral &&\n> > +\tgit push \"$up\" master collateral &&\n> > +\t# Make master incompatible with up/master\n> > +\tgit reset --hard HEAD^ &&\n> > +\t# --atomic should mention collateral was OK but failed anyway\n> > +\ttest_must_fail git push --atomic \"$up\" master collateral >&output &&\n> > +\tgrep \"master -> master\" output &&\n> > +\tgrep \"collateral -> collateral\" output\n> > +'\n> \n> Same here.\n> \n> > +\n> >  test_expect_success 'push --all can push to empty repo' '\n> >  \td=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&\n> >  \tgit init --bare \"$d\" &&\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index c7e17ec9cb..6b05a88faf 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,\n> >  {\n> >  \tint force_all = flags & TRANSPORT_PUSH_FORCE;\n> >  \tint mirror = flags & TRANSPORT_PUSH_MIRROR;\n> > +\tint atomic = flags & TRANSPORT_PUSH_ATOMIC;\n> >  \tstruct helper_data *data = transport->data;\n> >  \tstruct strbuf buf = STRBUF_INIT;\n> >  \tstruct ref *ref;\n> > @@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,\n> >  \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n> >  \t\tcase REF_STATUS_REJECT_STALE:\n> >  \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n> > +\t\t\tif (atomic) {\n> > +\t\t\t\tstring_list_clear(&cas_options, 0);\n> > +\t\t\t\treturn 0;\n> > +\t\t\t} else\n> > +\t\t\t\tcontinue;\n> >  \t\tcase REF_STATUS_UPTODATE:\n> >  \t\t\tcontinue;\n> >  \t\tdefault:\n> > diff --git a/transport.c b/transport.c\n> > index f1fcd2c4b0..f4d6b38f9d 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -1226,10 +1226,23 @@ int transport_push(struct repository *r,\n> >  \t\terr = push_had_errors(remote_refs);\n> >  \t\tret = push_ret | err;\n> >\n> > -\t\tif (!quiet || err)\n> > +\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n> \n> This looks funny. And it does so only...\n> \n> > +\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n> > +\t\t\t\tswitch (it->status) {\n> > +\t\t\t\tcase REF_STATUS_NONE:\n> > +\t\t\t\tcase REF_STATUS_UPTODATE:\n> > +\t\t\t\tcase REF_STATUS_OK:\n> > +\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> > +\t\t\t\tdefault:\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (!quiet || err) {\n> \n> ... because a curly was introduced around a single-liner. This adds\n> unnecessary noise to the patch.\n> \n> This easily distracts reviewers like myself from more important questions\n> such as: why was this conditional switch added before this conditional\n> block, does it intend to influence the printed push status? Ah, yes, of\n> course, even if `it->status` is changed, it actually modifies the data\n> to which `remote_refs` points. So yes, this has to be done here.\n\nOops, I thought I had omitted the new braces when I was staging the changes.\nReally sorry for the distraction. You're right that it makes the diff\nlook weird.\n\n> \n> >  \t\t\ttransport_print_push_status(transport->url, remote_refs,\n> >  \t\t\t\t\tverbose | porcelain, porcelain,\n> >  \t\t\t\t\treject_reasons);\n> > +\t\t}\n> >\n> >  \t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n> >  \t\t\tset_upstreams(transport, remote_refs, pretend);\n> > --\n> \n> Apart from minor nits, I like it. Thanks,\n> Dscho\n> \n> > 2.22.0.410.gd8fdbe21b5-goog\n> >\n> >\n"},{"id":"378565","messageId":"20190703185857.GD121233@google.com","threadId":"51416","inReplyTo":"xmqqblyb0yw6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-03T18:58:57Z","receivedAt":"2019-07-03T18:59:05Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jul 03, 2019 at 11:13:45AM -0700, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:\n> >> > +test_expect_success 'push --atomic shows all failed refs' '\n> >> > +\t# Make up/master, up/allrefs\n> >> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n> >> > +\tgit init --bare \"$d\" &&\n> >> > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> >> > +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n> >> > +\ttest_commit allrefs1 &&\n> >> > +\ttest_commit allrefs2 &&\n> >> > +\tgit branch allrefs &&\n> >> > +\tgit push \"$up\" master allrefs &&\n> >> > +\t# Make master and allrefs incompatible with up/master, up/allrefs\n> >> > +\tgit checkout allrefs &&\n> >> > +\tgit reset --hard HEAD^ &&\n> >> > +\tgit checkout master &&\n> >> > +\tgit reset --hard HEAD^ &&\n> >> > +\t# --atomic should complain about both master and allrefs\n> >> > +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n> >> \n> >> Don't rely on \">&output\", which is an unnecessary bash-ism here.  It\n> >> breaks test run under shells like dash.\n> >> \n> >> \t>output 2>&1\n> >> \n> >> should be OK.\n> >\n> > '2>output' would be a tad better, because those refs should be printed\n> > to stderr.\n> \n> Yeah; there are many existing uses of \">output 2>&1\" in the same\n> script and I was following the suit.  There also are 2>err and I\n> agree that it is more appropriate in this case.\n\nOh, this is a good point. I'll change it, thanks both.\n"},{"id":"378566","messageId":"20190703190118.GE121233@google.com","threadId":"51416","inReplyTo":"20190703185535.GC121233@google.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-03T19:01:18Z","receivedAt":"2019-07-03T19:01:25Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"> > > > +'\n> > > +\n> > > +test_expect_success 'push --atomic shows all failed refs' '\n> > > +\t# Make up/master, up/allrefs\n> > > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&\n> > > +\tgit init --bare \"$d\" &&\n> > > +\tgit --git-dir=\"$d\" config http.receivepack true &&\n> > > +\tup=\"$HTTPD_URL\"/smart/atomic-failed-refs.git &&\n> > > +\ttest_commit allrefs1 &&\n> > > +\ttest_commit allrefs2 &&\n> > > +\tgit branch allrefs &&\n> > > +\tgit push \"$up\" master allrefs &&\n> > > +\t# Make master and allrefs incompatible with up/master, up/allrefs\n> > > +\tgit checkout allrefs &&\n> > > +\tgit reset --hard HEAD^ &&\n> > > +\tgit checkout master &&\n> > > +\tgit reset --hard HEAD^ &&\n> > > +\t# --atomic should complain about both master and allrefs\n> > > +\ttest_must_fail git push --atomic \"$up\" master allrefs >&output &&\n> > > +\tgrep master output &&\n> > > +\tgrep allrefs output\n> > > +'\n> > \n> > I have the impression that the setup these two new test cases perform are\n> > _very_ similar, making it most likely that a combined test case would be\n> > more succinct, yet still complete and easily readable.\n> \n> (Junio replied to this downthread... I have more to ask too.)\n\nOops, I misremembered which comment he had replied to. Yeah, I'll think\nabout this and reword.\n\nReroll to come shortly. Thanks, all.\n\n - Emily\n"},{"id":"378569","messageId":"nycvar.QRO.7.76.6.1907032137210.44@tvgsbejvaqbjf.bet","threadId":"51416","inReplyTo":"20190703185535.GC121233@google.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-03T19:41:46Z","receivedAt":"2019-07-03T19:41:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Emily,\n\nOn Wed, 3 Jul 2019, Emily Shaffer wrote:\n\n> > > +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> > > +\ttest_commit atomic1 &&\n> > > +\ttest_commit atomic2 &&\n> > > +\tgit push \"$up\" master &&\n> >\n> > It would be more succinct to do a `git clone --bare . \"$d\"` here, instead\n> > of a `git init --bare` and a `git push` no?\n>\n> I'm not sure I would say \"more succinct.\" This leaves the test with the\n> same number of lines,\n\nNo, it does not, as `git clone --bare . \"$d\"` does _both_ the initializing\nand the object transfer.\n\nIt only saves one line, of course, but do keep in mind that anybody\nrunning into any kind of regression with your test case needs to\nunderstand what it does. And from experience I can tell you that reading\nany test case longer than 5 lines is quite annoying when you actually\nonly care about fixing the regression, and not so much about the wonderful\nstory the test case tells.\n\nSo in a sense, I guess I would even suggest to move as much of the setup\nfor your test cases outside, preferably into an initial `setup` test case\nthat initializes the minimal scenario required by the regression test\ncase.\n\nThanks,\nDscho\n"},{"id":"378573","messageId":"20190703205755.GF121233@google.com","threadId":"51416","inReplyTo":"nycvar.QRO.7.76.6.1907032137210.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-03T20:57:55Z","receivedAt":"2019-07-03T20:58:03Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jul 03, 2019 at 09:41:46PM +0200, Johannes Schindelin wrote:\n> Hi Emily,\n> \n> On Wed, 3 Jul 2019, Emily Shaffer wrote:\n> \n> > > > +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> > > > +\ttest_commit atomic1 &&\n> > > > +\ttest_commit atomic2 &&\n> > > > +\tgit push \"$up\" master &&\n> > >\n> > > It would be more succinct to do a `git clone --bare . \"$d\"` here, instead\n> > > of a `git init --bare` and a `git push` no?\n> >\n> > I'm not sure I would say \"more succinct.\" This leaves the test with the\n> > same number of lines,\n> \n> No, it does not, as `git clone --bare . \"$d\"` does _both_ the initializing\n> and the object transfer.\n> \n> It only saves one line, of course, but do keep in mind that anybody\n> running into any kind of regression with your test case needs to\n> understand what it does. And from experience I can tell you that reading\n> any test case longer than 5 lines is quite annoying when you actually\n> only care about fixing the regression, and not so much about the wonderful\n> story the test case tells.\n\nI suppose I'm confused, then, as I understood you were asking me to\ncombine my three test cases into one, which naturally makes the test\nitself more complex and longer to read. Which do you prefer?\n\n> \n> So in a sense, I guess I would even suggest to move as much of the setup\n> for your test cases outside, preferably into an initial `setup` test case\n> that initializes the minimal scenario required by the regression test\n> case.\n> \n> Thanks,\n> Dscho\n"},{"id":"378579","messageId":"nycvar.QRO.7.76.6.1907040947170.44@tvgsbejvaqbjf.bet","threadId":"51416","inReplyTo":"20190703205755.GF121233@google.com","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-04T08:29:53Z","receivedAt":"2019-07-04T08:29:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Emily,\n\nOn Wed, 3 Jul 2019, Emily Shaffer wrote:\n\n> On Wed, Jul 03, 2019 at 09:41:46PM +0200, Johannes Schindelin wrote:\n>\n> > On Wed, 3 Jul 2019, Emily Shaffer wrote:\n> >\n> > > > > +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> > > > > +\ttest_commit atomic1 &&\n> > > > > +\ttest_commit atomic2 &&\n> > > > > +\tgit push \"$up\" master &&\n> > > >\n> > > > It would be more succinct to do a `git clone --bare . \"$d\"` here,\n> > > > instead of a `git init --bare` and a `git push` no?\n> > >\n> > > I'm not sure I would say \"more succinct.\" This leaves the test with the\n> > > same number of lines,\n> >\n> > No, it does not, as `git clone --bare . \"$d\"` does _both_ the initializing\n> > and the object transfer.\n> >\n> > It only saves one line, of course, but do keep in mind that anybody\n> > running into any kind of regression with your test case needs to\n> > understand what it does. And from experience I can tell you that reading\n> > any test case longer than 5 lines is quite annoying when you actually\n> > only care about fixing the regression, and not so much about the wonderful\n> > story the test case tells.\n>\n> I suppose I'm confused, then, as I understood you were asking me to\n> combine my three test cases into one, which naturally makes the test\n> itself more complex and longer to read. Which do you prefer?\n\nIf I were tasked with developing this further, I would try to move as much\nof the setup into the initial test case (if there is already a `setup`\ntest case; otherwise I would create one). In fact, I would try to reuse as\nmuch of the existing setup test case as possible, and only add commands if\nreally necessary. Then, I would try to combine the three test cases in the\npatch into a single one, structuring it by white space and using comments\nto clarify what is happening.\n\nIn my mind, even just adding an empty line before the comments like \"Make\nmaster incompatible with up/master\" would make it much easier for me to\nread, were I to analyze a test breakage.\n\nI have to admit that I have a hard time understanding what the intention\nof those three test cases is because I get confused: where does the set-up\nend, where is the code that is expected to fail, where are the\nexpectations tested?\n\nAlso, I get confused by how similar the test cases look, and have a hard\ntime discerning what the differences are (i.e. what are the interesting\nbits, where the entropy comes from).\n\nI could imagine that I would have had an easier time reading something\nlike this:\n\ntest_expect_success 'push --atomic' '\n\t: set up two branches, one which will require a force push &&\n\tgit checkout -b fast-forwarding master &&\n\ttest_commit push-atomic &&\n\tgit checkout -b non-fast-forwarding &&\n\n\t: now, initialize the bare repository to push to &&\n\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic.git &&\n\tgit clone --bare . $d &&\n\n\t: modify the two branches and create a third one &&\n\tgit reset --hard HEAD^ &&\n\tgit checkout fast-forwarding &&\n\ttest_commit no-need-to-force &&\n\tgit branch new-branch &&\n\n\t: now the atomic push must fail &&\n\ttest_must_fail git push --atomic \"$HTTPD_URL\"/smart/atomic.git \\\n\t\tfast-forwarding non-fast-forwarding new-branch 2>err &&\n\n\t: verify that new-branch was not pushed &&\n\ttest_must_fail git -C $d rev-parse --verify refs/heads/new-branch &&\n\n\t: fast-forwarding should be mentioned even if it would have been OK &&\n\tgrep fast-forwarding err\n'\n\nOf course, everybody has their preferences, and their personal style. I do\nnot want you to imitate my style just to \"pacify\" me. That's not the point\nof this example.\n\nThe point is that I need some structure to walk along, especially when I\nam a bit annoyed at a test case that shows that I introduced a regression\nand all I want is to understand as quickly as possible what I did wrong\nagain so that I can fix it and move on.\n\nThe point is that I want a regression test case to _not_ distract me from\nthe essential part, ideally I should be able to ignore all the set-up code\nand deduce from the command-line of the failing command what is going on.\n\nFor example, if the test case fails in the line `grep fast-forwarding err`\nabove, that command-line alone does not tell me anything, it just\nfrustrates me. If there is a comment above that line (which is ideally\npart of the `-x` trace, that's why I used `:` instead of `#`) that states\nthe intention in what I consider to be clear, simple English, it is a lot\neasier to figure out what the heck is going wrong.\n\nOf course, it would be even better if we had a function, say,\n`must_contain` that runs that `grep` and shows the file contents and the\nregular expression if that `grep` failed. That's of course outside of the\nconcern of your patch. But this idea illustrates what I want in a\nregression test case: I want it to _help_ me figure out things when it\nbreaks.\n\nCiao,\nDscho\n\nP.S.: Please note that the many test cases _I_ introduced into Git's test\nsuite mostly do not conform to what I wrote above. I learned _quite_ a\nlot of how I want regression test cases to look like in the past six\nmonths, not from writing them, but from analyzing literally hundreds of\nAzure Pipelines builds. And your question forced me to think about my\nlearnings to formulate the above (hopefully clear) explanation of what I\nwant in a regression test, so: thank you.\n\n"},{"id":"378733","messageId":"20190709202355.GA113966@google.com","threadId":"51416","inReplyTo":"nycvar.QRO.7.76.6.1907040947170.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-09T20:23:55Z","receivedAt":"2019-07-09T20:24:03Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Firstly, sorry for the delay, I wasn't working for national holiday from\nthe 4th til yesterday.\n\n> If I were tasked with developing this further, I would try to move as much\n> of the setup into the initial test case (if there is already a `setup`\n> test case; otherwise I would create one). In fact, I would try to reuse as\n> much of the existing setup test case as possible, and only add commands if\n> really necessary. Then, I would try to combine the three test cases in the\n> patch into a single one, structuring it by white space and using comments\n> to clarify what is happening.\n> \n> In my mind, even just adding an empty line before the comments like \"Make\n> master incompatible with up/master\" would make it much easier for me to\n> read, were I to analyze a test breakage.\n> \n> I have to admit that I have a hard time understanding what the intention\n> of those three test cases is because I get confused: where does the set-up\n> end, where is the code that is expected to fail, where are the\n> expectations tested?\n> \n> Also, I get confused by how similar the test cases look, and have a hard\n> time discerning what the differences are (i.e. what are the interesting\n> bits, where the entropy comes from).\n> \n> I could imagine that I would have had an easier time reading something\n> like this:\n> \n> test_expect_success 'push --atomic' '\n> \t: set up two branches, one which will require a force push &&\n> \tgit checkout -b fast-forwarding master &&\n> \ttest_commit push-atomic &&\n> \tgit checkout -b non-fast-forwarding &&\n> \n> \t: now, initialize the bare repository to push to &&\n> \td=$HTTPD_DOCUMENT_ROOT_PATH/atomic.git &&\n> \tgit clone --bare . $d &&\n> \n> \t: modify the two branches and create a third one &&\n> \tgit reset --hard HEAD^ &&\n> \tgit checkout fast-forwarding &&\n> \ttest_commit no-need-to-force &&\n> \tgit branch new-branch &&\n> \n> \t: now the atomic push must fail &&\n> \ttest_must_fail git push --atomic \"$HTTPD_URL\"/smart/atomic.git \\\n> \t\tfast-forwarding non-fast-forwarding new-branch 2>err &&\n> \n> \t: verify that new-branch was not pushed &&\n> \ttest_must_fail git -C $d rev-parse --verify refs/heads/new-branch &&\n> \n> \t: fast-forwarding should be mentioned even if it would have been OK &&\n> \tgrep fast-forwarding err\n> '\n\nAfter I read your initial comment this is close to what my rewrite\nstarted to look like; I suppose we're only the same page after all :)\n\n> \n> Of course, everybody has their preferences, and their personal style. I do\n> not want you to imitate my style just to \"pacify\" me. That's not the point\n> of this example.\n> \n> The point is that I need some structure to walk along, especially when I\n> am a bit annoyed at a test case that shows that I introduced a regression\n> and all I want is to understand as quickly as possible what I did wrong\n> again so that I can fix it and move on.\n> \n> The point is that I want a regression test case to _not_ distract me from\n> the essential part, ideally I should be able to ignore all the set-up code\n> and deduce from the command-line of the failing command what is going on.\n> \n> For example, if the test case fails in the line `grep fast-forwarding err`\n> above, that command-line alone does not tell me anything, it just\n> frustrates me. If there is a comment above that line (which is ideally\n> part of the `-x` trace, that's why I used `:` instead of `#`) that states\n> the intention in what I consider to be clear, simple English, it is a lot\n> easier to figure out what the heck is going wrong.\n> \n> Of course, it would be even better if we had a function, say,\n> `must_contain` that runs that `grep` and shows the file contents and the\n> regular expression if that `grep` failed. That's of course outside of the\n> concern of your patch. But this idea illustrates what I want in a\n> regression test case: I want it to _help_ me figure out things when it\n> breaks.\n> \n> Ciao,\n> Dscho\n> \n> P.S.: Please note that the many test cases _I_ introduced into Git's test\n> suite mostly do not conform to what I wrote above. I learned _quite_ a\n> lot of how I want regression test cases to look like in the past six\n> months, not from writing them, but from analyzing literally hundreds of\n> Azure Pipelines builds. And your question forced me to think about my\n> learnings to formulate the above (hopefully clear) explanation of what I\n> want in a regression test, so: thank you.\n> \n\nThis writeup is great, and I think your example is very clear and\nreadable. I'll push a reroll with my similar-looking reformat soon.\nThanks, Johannes.\n\n - Emily\n"},{"id":"378736","messageId":"20190709211043.48597-1-emilyshaffer@google.com","threadId":"51416","inReplyTo":"20190702005340.66615-1-emilyshaffer@google.com","subject":"[PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-09T21:10:43Z","receivedAt":"2019-07-09T21:11:01Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach transport-helper how to notice if skipping a ref during push would\nviolate atomicity on the client side. We notice that a ref would be\nrejected, and choose not to send it, but don't notice that if the client\nhas asked for --atomic we are violating atomicity if all the other\npushes we are sending would succeed. Asking the server end to uphold\natomicity wouldn't work here as the server doesn't have any idea that we\ntried to update a ref that's broken.\n\nThe added test-case is a succinct way to reproduce this issue that fails\ntoday. The same steps work fine when we aren't using a transport-helper\nto get to the upstream, i.e. when we've added a local repository as a\nremote:\n\n  git remote add ~/upstream upstream\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nSince v2, I combined the test cases into one. Unfortunately the new test\ncase is checking a few things at once, but I hope the comments make it\nclear enough. Johannes, I still left the setup at the top; for me in the\ntradeoff between \"one less line\" and \"setup happens first\", the latter\nwins.\n\nI also removed the \"try pushing two bad branches at the same time\" case;\nI don't think it's really covered by atomicity. So now we check that if\nwe push one bad ref, collateral damage in the form of 1) a new branch\nand 2) a perfectly good push of another branch are both rejected and\nreported correctly.\n\nAlso, got rid of the accidental new curly braces around unrelated part\nof the code.\n\nThanks all.\n\n - Emily\n\n t/t5541-http-push-smart.sh | 39 ++++++++++++++++++++++++++++++++++++++\n transport-helper.c         |  6 ++++++\n transport.c                | 13 +++++++++++++\n 3 files changed, 58 insertions(+)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 8ef8763e06..7f9ae1ef3f 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -177,6 +177,45 @@ test_expect_success 'push (chunked)' '\n \t test $HEAD = $(git rev-parse --verify HEAD))\n '\n \n+test_expect_success 'push --atomic also prevents branch creation, reports collateral' '\n+\t# Setup upstream repo - empty for now\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n+\tgit init --bare \"$d\" &&\n+\ttest_config -C \"$d\" http.receivepack true &&\n+\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n+\n+\t# Tell up about two branches for now\n+\ttest_commit atomic1 &&\n+\ttest_commit atomic2 &&\n+\tgit branch collateral &&\n+\tgit push \"$up\" master collateral &&\n+\n+\t# collateral is a valid push, but should be failed by atomic push\n+\tgit checkout collateral &&\n+\ttest_commit collateral1 &&\n+\n+\t# Make master incompatible with upstream to provoke atomic\n+\tgit checkout master &&\n+\tgit reset --hard HEAD^ &&\n+\n+\t# Add a new branch which should be failed by atomic push. This is a\n+\t# regression case.\n+\tgit branch atomic &&\n+\n+\t# --atomic should cause entire push to be rejected\n+\ttest_must_fail git push --atomic \"$up\" master atomic collateral 2>output &&\n+\n+\t# the new branch should not have been created upstream\n+\ttest_must_fail git -C \"$d\" rev-parse refs/heads/atomic &&\n+\n+\t# the failed refs should be indicated\n+\tgrep \"master -> master\" output | grep rejected &&\n+\n+\t# the collateral failure refs should be indicated\n+\tgrep \"atomic -> atomic\" output | grep \"atomic push failed\" &&\n+\tgrep \"collateral -> collateral\" output | grep \"atomic push failed\"\n+'\n+\n test_expect_success 'push --all can push to empty repo' '\n \td=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&\n \tgit init --bare \"$d\" &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex c7e17ec9cb..6b05a88faf 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,\n {\n \tint force_all = flags & TRANSPORT_PUSH_FORCE;\n \tint mirror = flags & TRANSPORT_PUSH_MIRROR;\n+\tint atomic = flags & TRANSPORT_PUSH_ATOMIC;\n \tstruct helper_data *data = transport->data;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref;\n@@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\tcase REF_STATUS_REJECT_STALE:\n \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n+\t\t\tif (atomic) {\n+\t\t\t\tstring_list_clear(&cas_options, 0);\n+\t\t\t\treturn 0;\n+\t\t\t} else\n+\t\t\t\tcontinue;\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport.c b/transport.c\nindex f1fcd2c4b0..d768bc275e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,\n \t\terr = push_had_errors(remote_refs);\n \t\tret = push_ret | err;\n \n+\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n+\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n+\t\t\t\tswitch (it->status) {\n+\t\t\t\tcase REF_STATUS_NONE:\n+\t\t\t\tcase REF_STATUS_UPTODATE:\n+\t\t\t\tcase REF_STATUS_OK:\n+\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n+\t\t\t\t\tbreak;\n+\t\t\t\tdefault:\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t}\n+\n \t\tif (!quiet || err)\n \t\t\ttransport_print_push_status(transport->url, remote_refs,\n \t\t\t\t\tverbose | porcelain, porcelain,\n-- \n2.22.0.410.gd8fdbe21b5-goog\n\n"},{"id":"378766","messageId":"xmqqpnmhu6mh.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190709211043.48597-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-10T17:44:22Z","receivedAt":"2019-07-10T17:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> +test_expect_success 'push --atomic also prevents branch creation, reports collateral' '\n> +\t# Setup upstream repo - empty for now\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n> +\tgit init --bare \"$d\" &&\n> +\ttest_config -C \"$d\" http.receivepack true &&\n> +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> +\n> +\t# Tell up about two branches for now\n\n-ECANTPARSE \"Tell up\" part.\n\n> +\ttest_commit atomic1 &&\n> +\ttest_commit atomic2 &&\n> +\tgit branch collateral &&\n> +\tgit push \"$up\" master collateral &&\n\nOK, so an initially empty directory $d that appears to network\nclients as $up now has two branches, 'master' and 'collateral',\nboth pointing at the same history that ends with two commits,\natomic2 whose parent is atomic1.\n\n> +\t# collateral is a valid push, but should be failed by atomic push\n> +\tgit checkout collateral &&\n> +\ttest_commit collateral1 &&\n> +\n> +\t# Make master incompatible with upstream to provoke atomic\n> +\tgit checkout master &&\n> +\tgit reset --hard HEAD^ &&\n\ncollateral grows, master rewinds.\n\n> +\t# Add a new branch which should be failed by atomic push. This is a\n> +\t# regression case.\n> +\tgit branch atomic &&\n\nAnother branch atomic is added\n\n> +\t# --atomic should cause entire push to be rejected\n> +\ttest_must_fail git push --atomic \"$up\" master atomic collateral 2>output &&\n\nAttempt to push all three: collateral alone would be OK, so is\natomic, but because master rewinds, we expect none of the three to\ngo through.\n\n> +\t# the new branch should not have been created upstream\n> +\ttest_must_fail git -C \"$d\" rev-parse refs/heads/atomic &&\n\nThe new branch should not have been created; if this rev-parse\nsucceeded, it would be a bug.\n\nUp to point, I have no possible improvements to offer ;-)\nVery well done.\n\n> +\t# the failed refs should be indicated\n> +\tgrep \"master -> master\" output | grep rejected &&\n\nI'd rather see the effect, i.e. what the command did that can be\nobserved externally, than the report, i.e. what the command claims\nto have done, if it is equally straight-forward to verify either.\n\nThat can be done by making sure that the output from \"git -C \"$d\"\nrev-parse refs/heads/master\" match output from \"git rev-parse\natomic2\", no?  That ensures 'master' in the receiving end stayed the\nsame.\n\n> +\t# the collateral failure refs should be indicated\n> +\tgrep \"atomic -> atomic\" output | grep \"atomic push failed\" &&\n> +\tgrep \"collateral -> collateral\" output | grep \"atomic push failed\"\n\nLikewise for the other two.  \n\nFWIW, these three can further lose a process each, i.e.\n\n\tgrep \"^ ! .*rejected.* master -> master\" output\n\neven if we for some reason do not want to check the effect and take\nthe claim by the command being tested at the face value (which I do\nnot think is a good idea).\n\nThanks.\n"},{"id":"378767","messageId":"xmqqims9u679.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"xmqqpnmhu6mh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-10T17:53:30Z","receivedAt":"2019-07-10T17:53:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +\t# the new branch should not have been created upstream\n>> +\ttest_must_fail git -C \"$d\" rev-parse refs/heads/atomic &&\n>\n> The new branch should not have been created; if this rev-parse\n> succeeded, it would be a bug.\n\nOne thing I forgot.  If refs/heads/atomic did not exist, but say\nrefs/tags/refs/heads/atomic did, the rev-parse would succeed, which\nis a rather unfortunate source of confusion.\n\nOf course, we know we have never touched \"$d\" to cause such a funny\ntag, so rev-parse is good enough in practice, but\n\n    git -C \"$d\" show-ref --verify refs/heads/atomic\n\nwould not dwim (its --verify mode was invented specifically for\nrectifying this issue with rev-parse, if I recall correctly), and is\nmore appropriate best-practice version to write here, especially if\nwe anticipate that future developers and Git users would treat this\nline as an example to mimic.\n\n> Up to point, I have no possible improvements to offer ;-)\n> Very well done.\n\nSo, I lied, but still the tests looked quite well done.\n"},{"id":"378864","messageId":"20190711205706.GD113966@google.com","threadId":"51416","inReplyTo":"xmqqpnmhu6mh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-11T20:57:06Z","receivedAt":"2019-07-11T20:57:14Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jul 10, 2019 at 10:44:22AM -0700, Junio C Hamano wrote:\n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> > +test_expect_success 'push --atomic also prevents branch creation, reports collateral' '\n> > +\t# Setup upstream repo - empty for now\n> > +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n> > +\tgit init --bare \"$d\" &&\n> > +\ttest_config -C \"$d\" http.receivepack true &&\n> > +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> > +\n> > +\t# Tell up about two branches for now\n> \n> -ECANTPARSE \"Tell up\" part.\n\ns/up/\"$&\"/ - thanks.\n\n[snip]\n> Up to point, I have no possible improvements to offer ;-)\n> Very well done.\n\nThis is nice to hear, thanks! :)\n\n> > +\t# the failed refs should be indicated\n> > +\tgrep \"master -> master\" output | grep rejected &&\n> \n> I'd rather see the effect, i.e. what the command did that can be\n> observed externally, than the report, i.e. what the command claims\n> to have done, if it is equally straight-forward to verify either.\n\nHmm. I'd like to argue that part of the requirement is to show the user\nwhat happened; for example, in an earlier iteration I was not\nsuccessfully reporting the \"collateral damage\" refs to the user, even\nthough they were not being pushed. To that end, I'd rather check both.\n\n> \n> That can be done by making sure that the output from \"git -C \"$d\"\n> rev-parse refs/heads/master\" match output from \"git rev-parse\n> atomic2\", no?  That ensures 'master' in the receiving end stayed the\n> same.\n\nSure, I agree.\n\n> \n> > +\t# the collateral failure refs should be indicated\n> > +\tgrep \"atomic -> atomic\" output | grep \"atomic push failed\" &&\n> > +\tgrep \"collateral -> collateral\" output | grep \"atomic push failed\"\n> \n> Likewise for the other two.  \n> \n> FWIW, these three can further lose a process each, i.e.\n> \n> \tgrep \"^ ! .*rejected.* master -> master\" output\n> \n> even if we for some reason do not want to check the effect and take\n> the claim by the command being tested at the face value (which I do\n> not think is a good idea).\n\nWill swap, wasn't sure on preference between regex or process count.\nThanks.\n\n - Emily\n"},{"id":"378866","messageId":"xmqqmuhkqnp7.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190711205706.GD113966@google.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-11T21:13:40Z","receivedAt":"2019-07-11T21:13:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> Hmm. I'd like to argue that part of the requirement is to show the user\n> what happened; for example, in an earlier iteration I was not\n> successfully reporting the \"collateral damage\" refs to the user, even\n> though they were not being pushed. To that end, I'd rather check both.\n\nIf we can afford to check both, making sure that we behave correctly\nand report what we did correctly would of course be the best.\n\n;-)\n\n\n"},{"id":"378867","messageId":"20190711211436.GE113966@google.com","threadId":"51416","inReplyTo":"xmqqims9u679.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-11T21:14:36Z","receivedAt":"2019-07-11T21:14:43Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jul 10, 2019 at 10:53:30AM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >> +\t# the new branch should not have been created upstream\n> >> +\ttest_must_fail git -C \"$d\" rev-parse refs/heads/atomic &&\n> >\n> > The new branch should not have been created; if this rev-parse\n> > succeeded, it would be a bug.\n> \n> One thing I forgot.  If refs/heads/atomic did not exist, but say\n> refs/tags/refs/heads/atomic did, the rev-parse would succeed, which\n> is a rather unfortunate source of confusion.\n> \n> Of course, we know we have never touched \"$d\" to cause such a funny\n> tag, so rev-parse is good enough in practice, but\n> \n>     git -C \"$d\" show-ref --verify refs/heads/atomic\n> \n> would not dwim (its --verify mode was invented specifically for\n> rectifying this issue with rev-parse, if I recall correctly), and is\n> more appropriate best-practice version to write here, especially if\n> we anticipate that future developers and Git users would treat this\n> line as an example to mimic.\n> \n> > Up to point, I have no possible improvements to offer ;-)\n> > Very well done.\n> \n> So, I lied, but still the tests looked quite well done.\n\n\nOh, that's very interesting! Thanks for pointing it out. :)\n\nReroll coming in a few. Thanks, Junio.\n\n - Emily\n"},{"id":"378868","messageId":"20190711211919.32734-1-emilyshaffer@google.com","threadId":"51416","inReplyTo":"20190709211043.48597-1-emilyshaffer@google.com","subject":"[PATCH v3] transport-helper: enforce atomic in push_refs_with_push","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-11T21:19:19Z","receivedAt":"2019-07-11T21:19:29Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach transport-helper how to notice if skipping a ref during push would\nviolate atomicity on the client side. We notice that a ref would be\nrejected, and choose not to send it, but don't notice that if the client\nhas asked for --atomic we are violating atomicity if all the other\npushes we are sending would succeed. Asking the server end to uphold\natomicity wouldn't work here as the server doesn't have any idea that we\ntried to update a ref that's broken.\n\nThe added test-case is a succinct way to reproduce this issue that fails\ntoday. The same steps work fine when we aren't using a transport-helper\nto get to the upstream, i.e. when we've added a local repository as a\nremote:\n\n  git remote add ~/upstream upstream\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nSince the previous version, made a couple changes based on Junio's\ncomments on the testcase.\n\n- Use show-ref instead of rev-parse to avoid a corner case where a tag\n  is named the same thing as the branch we hope not to create\n- Reduce number of grep processes by using a longer regex instead\n- Check the real contents of the upstream repo instead of just relying\n  on the client side's reporting\n  - The check on the client side remains, however, to ensure the user is\n    made aware of why their perfectly good ref failed to push.\n\nThanks, all.\n\n t/t5541-http-push-smart.sh | 49 ++++++++++++++++++++++++++++++++++++++\n transport-helper.c         |  6 +++++\n transport.c                | 13 ++++++++++\n 3 files changed, 68 insertions(+)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 8ef8763e06..92bac43257 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -177,6 +177,55 @@ test_expect_success 'push (chunked)' '\n \t test $HEAD = $(git rev-parse --verify HEAD))\n '\n \n+test_expect_success 'push --atomic also prevents branch creation, reports collateral' '\n+\t# Setup upstream repo - empty for now\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n+\tgit init --bare \"$d\" &&\n+\ttest_config -C \"$d\" http.receivepack true &&\n+\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n+\n+\t# Tell \"$up\" about two branches for now\n+\ttest_commit atomic1 &&\n+\ttest_commit atomic2 &&\n+\tgit branch collateral &&\n+\tgit push \"$up\" master collateral &&\n+\n+\t# collateral is a valid push, but should be failed by atomic push\n+\tgit checkout collateral &&\n+\ttest_commit collateral1 &&\n+\n+\t# Make master incompatible with upstream to provoke atomic\n+\tgit checkout master &&\n+\tgit reset --hard HEAD^ &&\n+\n+\t# Add a new branch which should be failed by atomic push. This is a\n+\t# regression case.\n+\tgit branch atomic &&\n+\n+\t# --atomic should cause entire push to be rejected\n+\ttest_must_fail git push --atomic \"$up\" master atomic collateral 2>output &&\n+\n+\t# the new branch should not have been created upstream\n+\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n+\n+\t# upstream should still reflect atomic2, the last thing we pushed\n+\t# successfully\n+\tgit rev-parse atomic2 >expected &&\n+\t# on master...\n+\tgit -C \"$d\" rev-parse refs/heads/master >actual &&\n+\ttest_cmp expected actual &&\n+\t# ...and collateral.\n+\tgit -C \"$d\" rev-parse refs/heads/collateral >actual &&\n+\ttest_cmp expected actual &&\n+\n+\t# the failed refs should be indicated to the user\n+\tgrep \"^ ! .*rejected.* master -> master\" output &&\n+\n+\t# the collateral failure refs should be indicated to the user\n+\tgrep \"^ ! .*rejected.* atomic -> atomic .*atomic push failed\" output &&\n+\tgrep \"^ ! .*rejected.* collateral -> collateral .*atomic push failed\" output\n+'\n+\n test_expect_success 'push --all can push to empty repo' '\n \td=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&\n \tgit init --bare \"$d\" &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex c7e17ec9cb..6b05a88faf 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,\n {\n \tint force_all = flags & TRANSPORT_PUSH_FORCE;\n \tint mirror = flags & TRANSPORT_PUSH_MIRROR;\n+\tint atomic = flags & TRANSPORT_PUSH_ATOMIC;\n \tstruct helper_data *data = transport->data;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref;\n@@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\tcase REF_STATUS_REJECT_STALE:\n \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n+\t\t\tif (atomic) {\n+\t\t\t\tstring_list_clear(&cas_options, 0);\n+\t\t\t\treturn 0;\n+\t\t\t} else\n+\t\t\t\tcontinue;\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport.c b/transport.c\nindex f1fcd2c4b0..d768bc275e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,\n \t\terr = push_had_errors(remote_refs);\n \t\tret = push_ret | err;\n \n+\t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n+\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n+\t\t\t\tswitch (it->status) {\n+\t\t\t\tcase REF_STATUS_NONE:\n+\t\t\t\tcase REF_STATUS_UPTODATE:\n+\t\t\t\tcase REF_STATUS_OK:\n+\t\t\t\t\tit->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n+\t\t\t\t\tbreak;\n+\t\t\t\tdefault:\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t}\n+\n \t\tif (!quiet || err)\n \t\t\ttransport_print_push_status(transport->url, remote_refs,\n \t\t\t\t\tverbose | porcelain, porcelain,\n-- \n2.22.0.410.gd8fdbe21b5-goog\n\n"},{"id":"378894","messageId":"xmqq36jbqkyf.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190711211919.32734-1-emilyshaffer@google.com","subject":"Re: [PATCH v3] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-12T16:25:12Z","receivedAt":"2019-07-12T16:25:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> +test_expect_success 'push --atomic also prevents branch creation, reports collateral' '\n> +\t# Setup upstream repo - empty for now\n> +\td=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&\n> +\tgit init --bare \"$d\" &&\n> +\ttest_config -C \"$d\" http.receivepack true &&\n> +\tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n> +\n> +\t# Tell \"$up\" about two branches for now\n\nAhh, that's what you meant ;-)  Makes sense.\n"},{"id":"378957","messageId":"CAPUEspgjSAqHUP2vsCCjqG8b0QkWdgoAByh4XdqsThQMt=V38w@mail.gmail.com","threadId":"51416","inReplyTo":"20190709211043.48597-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-16T07:10:16Z","receivedAt":"2019-07-16T07:10:32Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jul 9, 2019 at 2:16 PM Emily Shaffer <emilyshaffer@google.com> wrote:\n>\n> diff --git a/transport.c b/transport.c\n> index f1fcd2c4b0..d768bc275e 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,\n>                 err = push_had_errors(remote_refs);\n>                 ret = push_ret | err;\n>\n> +               if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n> +                       for (struct ref *it = remote_refs; it; it = it->next)\n\nmoving \"struct ref it\" out of the loop, allows for building with ancient\ncompilers that don't support C90 (even if only by default) as I found\nout while building pu in a Centos 6 box\n\nCarlo\n"},{"id":"378978","messageId":"xmqq8ssx53a0.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"CAPUEspgjSAqHUP2vsCCjqG8b0QkWdgoAByh4XdqsThQMt=V38w@mail.gmail.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-16T16:53:59Z","receivedAt":"2019-07-16T16:54:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> On Tue, Jul 9, 2019 at 2:16 PM Emily Shaffer <emilyshaffer@google.com> wrote:\n>>\n>> diff --git a/transport.c b/transport.c\n>> index f1fcd2c4b0..d768bc275e 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,\n>>                 err = push_had_errors(remote_refs);\n>>                 ret = push_ret | err;\n>>\n>> +               if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n>> +                       for (struct ref *it = remote_refs; it; it = it->next)\n>\n> moving \"struct ref it\" out of the loop, allows for building with ancient\n> compilers that don't support C90 (even if only by default) as I found\n> out while building pu in a Centos 6 box\n\nDoes everything else compile OK with your rather old compiler on\nCentos 6?  I was historically rather pedantic to stick to C89 but\nfor past several years we've been experimenting with a bit more\nmodern features of the language to see if anybody screams (namely,\ndesignated initializers in structs and arrays, and trailing comma in\nenum definition) but I think we still reject variable definition in\nfor loop control (we saw and rewrote another patch that tried to use\nit late last year).\n\nApparently, this one slipped our review process.\n\nThanks.\n\ncf. https://public-inbox.org/git/xmqqwopgqsws.fsf@gitster-ct.c.googlers.com/\ncf. https://public-inbox.org/git/xmqqmuuz9jih.fsf@gitster-ct.c.googlers.com/\n\n\n\n \n"},{"id":"378979","messageId":"xmqq4l3l520f.fsf_-_@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"xmqq8ssx53a0.fsf@gitster-ct.c.googlers.com","subject":"[RFC/PATCH] CodingGuidelines: spell out post-C89 rules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-16T17:21:20Z","receivedAt":"2019-07-16T17:21:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even though we have been sticking to C89, there are a few handy\nfeatures we borrow from more recent C language in our codebase after\ntrying them in weather balloons and saw that nobody screamed.\n\nSpell them out.\n\nWhile at it, extend the existing variable declaration rule a bit to\nread better with the newly spelled out rule for the for loop.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/CodingGuidelines | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 1169ff6c8e..53903b14c8 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -195,10 +195,24 @@ For C programs:\n    by e.g. \"echo DEVELOPER=1 >>config.mak\".\n \n  - We try to support a wide range of C compilers to compile Git with,\n-   including old ones. That means that you should not use C99\n-   initializers, even if a lot of compilers grok it.\n+   including old ones. That means that you should not use certain C99\n+   features, even if your compiler groks it.  There are a few\n+   exceptions:\n \n- - Variables have to be declared at the beginning of the block.\n+   . since early 2012 with e1327023ea, we have been using an enum\n+     definition whose last element is followed by a comma.\n+\n+   . since mid 2017 with cbc0f81d and 512f41cf, we have been using\n+     designated initializers for struct and array.\n+\n+   These used to be forbidden, but we have not heard breakage report,\n+   so they are assumed to be safe.\n+\n+ - Variables have to be declared at the beginning of the block, before\n+   the first statement (i.e. -Wdeclaration-after-statement).\n+\n+ - Declaring a variable in the for loop \"for (int i = 0; i < 10; i++)\"\n+   is still not allowed in this codebase.\n \n  - NULL pointers shall be written as NULL, not as 0.\n \n\n"},{"id":"378982","messageId":"CAPUEspjCXe4ga9HrF=RwGfP3Wc6Et4JW5ZG5thihgQTUQss77A@mail.gmail.com","threadId":"51416","inReplyTo":"xmqq8ssx53a0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-16T18:00:01Z","receivedAt":"2019-07-16T18:00:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jul 16, 2019 at 9:54 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Does everything else compile OK with your rather old compiler on\n> Centos 6?\n\nyes, they were a few -Wmaybe-uninitialized warnings but they were most\nlikely false positives.\n\ngcc 4.4.7 aborts the build (even without -Werror) with the following message:\ntransport.c: In function 'transport_push':\ntransport.c:1232: error: 'for' loop initial declarations are only\nallowed in C99 mode\ntransport.c:1232: note: use option -std=c99 or -std=gnu99 to compile your code\nmake: *** [transport.o] Error 1\n\nCarlo\n"},{"id":"378994","messageId":"xmqq36j53esa.fsf_-_@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"xmqq8ssx53a0.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] transport-helper: avoid var decl in for () loop control","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-16T20:28:21Z","receivedAt":"2019-07-16T20:28:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We do allow a few selected C99 constructs in our codebase these\ndays, but this is not among them (yet).\n\nReported-by: Carlo Arenas <carenas@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n transport.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/transport.c b/transport.c\nindex d768bc275e..453de8f704 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1227,7 +1227,8 @@ int transport_push(struct repository *r,\n \t\tret = push_ret | err;\n \n \t\tif ((flags & TRANSPORT_PUSH_ATOMIC) && err) {\n-\t\t\tfor (struct ref *it = remote_refs; it; it = it->next)\n+\t\t\tstruct ref *it;\n+\t\t\tfor (it = remote_refs; it; it = it->next)\n \t\t\t\tswitch (it->status) {\n \t\t\t\tcase REF_STATUS_NONE:\n \t\t\t\tcase REF_STATUS_UPTODATE:\n"},{"id":"378997","messageId":"20190717004231.GA93801@google.com","threadId":"51416","inReplyTo":"xmqq36j53esa.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH] transport-helper: avoid var decl in for () loop control","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-07-17T00:42:31Z","receivedAt":"2019-07-17T00:42:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> We do allow a few selected C99 constructs in our codebase these\n> days, but this is not among them (yet).\n>\n> Reported-by: Carlo Arenas <carenas@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  transport.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n\nYes, gcc 4.8 fails to build without this:\n\n transport.c:1234:4: error: ‘for’ loop initial declarations are only allowed in C99 mode\n     for (struct ref *it = remote_refs; it; it = it->next)\n     ^\n transport.c:1234:4: note: use option -std=c99 or -std=gnu99 to compile your code\n\nArguably it would be nice to use -std=gnu99 for better consistency\nbetween gcc versions, but it's moot here: avoiding the declaration in\nfor loop initializer is more consistent with\n-Wdeclaration-after-statement anyway.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nThanks.\n"},{"id":"378998","messageId":"20190717005537.GB93801@google.com","threadId":"51416","inReplyTo":"xmqq4l3l520f.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [RFC/PATCH] CodingGuidelines: spell out post-C89 rules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-07-17T00:55:37Z","receivedAt":"2019-07-17T00:55:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Even though we have been sticking to C89, there are a few handy\n> features we borrow from more recent C language in our codebase after\n> trying them in weather balloons and saw that nobody screamed.\n>\n> Spell them out.\n\nThanks for this.  It gives a place to advertise future weather balloons,\ntoo.\n\n[...]\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -195,10 +195,24 @@ For C programs:\n>     by e.g. \"echo DEVELOPER=1 >>config.mak\".\n>  \n>   - We try to support a wide range of C compilers to compile Git with,\n> -   including old ones. That means that you should not use C99\n> -   initializers, even if a lot of compilers grok it.\n> +   including old ones. That means that you should not use certain C99\n> +   features, even if your compiler groks it.  There are a few\n> +   exceptions:\n> +\n> +   . since early 2012 with e1327023ea, we have been using an enum\n> +     definition whose last element is followed by a comma.\n\nThis is an interesting one: it's super convenient, but we have received\npatches every 10 years or so to remove the trailing comma --- e.g.\nhttps://public-inbox.org/git/20100311163235.GC7877@thor.il.thewrittenword.com/\n\nI *think* these were motivated by wanting to be able to build Git with\nold compilers with pedantic warnings on, and certainly the last seven\nyears of silence on the subject suggests it's okay.  Should we be even\nmore prescriptive and say that the last element should always be\nfollowed by a comma, for ease of later patching?\n\n> +\n> +   . since mid 2017 with cbc0f81d and 512f41cf, we have been using\n> +     designated initializers for struct and array.\n\nCan this include an example for the benefit of readers that don't know\nwhat a designated initializer is?  E.g.\n\n      . since mid 2017 with cb0f81d and 512f41cf, we have been using\n        designated initializers for struct members (\"{ .alloc = 1 }\")\n\tand array members (\"[5] = 0\").\n\n> +\n> +   These used to be forbidden, but we have not heard breakage report,\n> +   so they are assumed to be safe.\n\nnit: missing article \"any\" before \"breakage reports\".\n\n>  \n> - - Variables have to be declared at the beginning of the block.\n> + - Variables have to be declared at the beginning of the block, before\n> +   the first statement (i.e. -Wdeclaration-after-statement).\n> +\n> + - Declaring a variable in the for loop \"for (int i = 0; i < 10; i++)\"\n> +   is still not allowed in this codebase.\n\nNice.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nThanks.\n"},{"id":"378999","messageId":"CAGyf7-EWuW5NX27YPeSb9+5iMU0pftUs-WxvUH1NqpGeNsXESg@mail.gmail.com","threadId":"51416","inReplyTo":"xmqq4l3l520f.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [RFC/PATCH] CodingGuidelines: spell out post-C89 rules","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2019-07-17T01:09:06Z","receivedAt":"2019-07-17T01:09:21Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Tue, Jul 16, 2019 at 10:21 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Even though we have been sticking to C89, there are a few handy\n> features we borrow from more recent C language in our codebase after\n> trying them in weather balloons and saw that nobody screamed.\n>\n> Spell them out.\n>\n> While at it, extend the existing variable declaration rule a bit to\n> read better with the newly spelled out rule for the for loop.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  Documentation/CodingGuidelines | 20 +++++++++++++++++---\n>  1 file changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 1169ff6c8e..53903b14c8 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -195,10 +195,24 @@ For C programs:\n>     by e.g. \"echo DEVELOPER=1 >>config.mak\".\n>\n>   - We try to support a wide range of C compilers to compile Git with,\n> -   including old ones. That means that you should not use C99\n> -   initializers, even if a lot of compilers grok it.\n> +   including old ones. That means that you should not use certain C99\n> +   features, even if your compiler groks it.  There are a few\n> +   exceptions:\n>\n> - - Variables have to be declared at the beginning of the block.\n> +   . since early 2012 with e1327023ea, we have been using an enum\n> +     definition whose last element is followed by a comma.\n\nIs there a significance to the leading . here versus a leading - below?\n\n> +\n> +   . since mid 2017 with cbc0f81d and 512f41cf, we have been using\n> +     designated initializers for struct and array.\n> +\n> +   These used to be forbidden, but we have not heard breakage report,\n> +   so they are assumed to be safe.\n\nWith the placement here, is it possible that someone might read the\n“These used to be forbidden” as applying to the items that follow\nafter it, rather than the items that preceded it? Put a different way,\ncould there be some value in having some additional verbiage here that\nindicates something along the lines of “Aside from those exceptions,\nother C99 features are not allowed. Some common examples are:”\n\nJust a thought. (Pardon the suggestion from the peanut gallery!)\n\n> +\n> + - Variables have to be declared at the beginning of the block, before\n> +   the first statement (i.e. -Wdeclaration-after-statement).\n> +\n> + - Declaring a variable in the for loop \"for (int i = 0; i < 10; i++)\"\n> +   is still not allowed in this codebase.\n>\n>   - NULL pointers shall be written as NULL, not as 0.\n>\n>\n"},{"id":"379017","messageId":"xmqqv9w01wdn.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190717005537.GB93801@google.com","subject":"Re: [RFC/PATCH] CodingGuidelines: spell out post-C89 rules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-17T16:03:32Z","receivedAt":"2019-07-17T16:03:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>\n>> Even though we have been sticking to C89, there are a few handy\n>> features we borrow from more recent C language in our codebase after\n>> trying them in weather balloons and saw that nobody screamed.\n>>\n>> Spell them out.\n>\n> Thanks for this.  It gives a place to advertise future weather balloons,\n> too.\n>\n> [...]\n>> --- a/Documentation/CodingGuidelines\n>> +++ b/Documentation/CodingGuidelines\n>> @@ -195,10 +195,24 @@ For C programs:\n>>     by e.g. \"echo DEVELOPER=1 >>config.mak\".\n>>  \n>>   - We try to support a wide range of C compilers to compile Git with,\n>> -   including old ones. That means that you should not use C99\n>> -   initializers, even if a lot of compilers grok it.\n>> +   including old ones. That means that you should not use certain C99\n>> +   features, even if your compiler groks it.  There are a few\n\ns/it/them/ I think.\n\n>> +   exceptions:\n>> +\n>> +   . since early 2012 with e1327023ea, we have been using an enum\n>> +     definition whose last element is followed by a comma.\n>\n> ...  Should we be even\n> more prescriptive and say that the last element should always be\n> followed by a comma, for ease of later patching?\n\nThat is an interesting tangent.  We have been taking advantage of\nthe trailing comma for array initializers (e.g. options[] fed to\nparse_options() API)---we would gain similar convenience with it.\n\n>> +\n>> +   . since mid 2017 with cbc0f81d and 512f41cf, we have been using\n>> +     designated initializers for struct and array.\n>\n> Can this include an example for the benefit of readers that don't know\n> what a designated initializer is?\n\nI thought about it, but decided against it, because those who really\nwant to are already given the commit object names to learn from, and\nin larger context.\n\nBut if we are to split this into two bullet points (one for struct,\none for array), I am OK to add an example to it while at it.  Let me\nsee how it looks like.\n\n>> +\n>> +   These used to be forbidden, but we have not heard breakage report,\n>> +   so they are assumed to be safe.\n>\n> nit: missing article \"any\" before \"breakage reports\".\n\nThanks.\n\n-- >8 --\n    \nEven though we have been sticking to C89, there are a few handy\nfeatures we borrow from more recent C language in our codebase after\ntrying them in weather balloons and saw that nobody screamed.\n\nSpell them out.\n\nWhile at it, extend the existing variable declaration rule a bit to\nread better with the newly spelled out rule for the for loop.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/CodingGuidelines | 25 ++++++++++++++++++++++---\n 1 file changed, 22 insertions(+), 3 deletions(-)\n\n Documentation/CodingGuidelines | 26 +++++++++++++++++++++++---\n 1 file changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 32210a4386..bd4b7905d0 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -195,10 +195,30 @@ For C programs:\n    by e.g. \"echo DEVELOPER=1 >>config.mak\".\n \n  - We try to support a wide range of C compilers to compile Git with,\n-   including old ones. That means that you should not use C99\n-   initializers, even if a lot of compilers grok it.\n+   including old ones.  You should not use features from newer C\n+   standard, even if your compiler groks them.  \n \n- - Variables have to be declared at the beginning of the block.\n+   There are a few exceptions to this guideline:\n+\n+   . since early 2012 with e1327023ea, we have been using an enum\n+     definition whose last element is followed by a comma.  This, like\n+     an array initializer that ends with a trailing comma, can be used\n+     to reduce the patch noise when adding a new identifer at the end.\n+\n+   . since mid 2017 with cbc0f81d, we have been using designated\n+     initializers for struct (e.g. \"struct t v = { .val = 'a' };\").\n+\n+   . since mid 2017 with 512f41cf, we have been using designated\n+     initializers for array (e.g. \"int array[10] = { [5] = 2 }\").\n+\n+   These used to be forbidden, but we have not heard any breakage\n+   report, and they are assumed to be safe.\n+\n+ - Variables have to be declared at the beginning of the block, before\n+   the first statement (i.e. -Wdeclaration-after-statement).\n+\n+ - Declaring a variable in the for loop \"for (int i = 0; i < 10; i++)\"\n+   is still not allowed in this codebase.\n \n  - NULL pointers shall be written as NULL, not as 0.\n \n"},{"id":"379018","messageId":"xmqqr26o1wa1.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"CAGyf7-EWuW5NX27YPeSb9+5iMU0pftUs-WxvUH1NqpGeNsXESg@mail.gmail.com","subject":"Re: [RFC/PATCH] CodingGuidelines: spell out post-C89 rules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-17T16:05:42Z","receivedAt":"2019-07-17T16:05:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bryan Turner <bturner@atlassian.com> writes:\n\n> On Tue, Jul 16, 2019 at 10:21 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Even though we have been sticking to C89, there are a few handy\n>> features we borrow from more recent C language in our codebase after\n>> trying them in weather balloons and saw that nobody screamed.\n>>\n>> Spell them out.\n>>\n>> While at it, extend the existing variable declaration rule a bit to\n>> read better with the newly spelled out rule for the for loop.\n>>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>>  Documentation/CodingGuidelines | 20 +++++++++++++++++---\n>>  1 file changed, 17 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n>> index 1169ff6c8e..53903b14c8 100644\n>> --- a/Documentation/CodingGuidelines\n>> +++ b/Documentation/CodingGuidelines\n>> @@ -195,10 +195,24 @@ For C programs:\n>>     by e.g. \"echo DEVELOPER=1 >>config.mak\".\n>>\n>>   - We try to support a wide range of C compilers to compile Git with,\n>> -   including old ones. That means that you should not use C99\n>> -   initializers, even if a lot of compilers grok it.\n>> +   including old ones. That means that you should not use certain C99\n>> +   features, even if your compiler groks it.  There are a few\n>> +   exceptions:\n>>\n>> +   . since early 2012 with e1327023ea, we have been using an enum\n>> +     definition whose last element is followed by a comma.\n>\n> Is there a significance to the leading . here versus a leading - below?\n\nAbsolutely.\n\n    - Item 1's description\n\n    - Item 2's description\n      . subitem a of 2\n      . subitem b of 2\n      These two subitems are exceptions.\n\n    - Item 3's description\n\nwas what I meant.\n\n"},{"id":"379061","messageId":"20190718152234.GI20404@szeder.dev","threadId":"51416","inReplyTo":"xmqq8ssx53a0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-18T15:22:34Z","receivedAt":"2019-07-18T15:22:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jul 16, 2019 at 09:53:59AM -0700, Junio C Hamano wrote:\n> Carlo Arenas <carenas@gmail.com> writes:\n\n> >> +                       for (struct ref *it = remote_refs; it; it = it->next)\n> >\n> > moving \"struct ref it\" out of the loop, allows for building with ancient\n> > compilers that don't support C90 (even if only by default) as I found\n> > out while building pu in a Centos 6 box\n\n> but I think we still reject variable definition in\n> for loop control (we saw and rewrote another patch that tried to use\n> it late last year).\n> \n> Apparently, this one slipped our review process.\n\nI expected that this will eventually happen after Travis CI's default\nLinux image recently changed from Ubuntu 14.04 to 16.04; explanation\nin the commit message below.\n\nWith that patch issues like this could be caught earlier, while they\nare only in 'pu' but not yet in 'next'.  But do we really want to do\nthat, is that the right tradeoff?  Dunno...  Adding a dedicated CI job\njust to check that there are no 'for' loop initial declarations seems\nkind of excessive, even if it only builds but doesn't run the test\nsuite.  And I don't know whether there are any other undesired (\"too\nnew\") constructs that GCC 4.8 would catch but later compilers quietly\naccept.\n\n  --- >8 ---\n\nSubject: [PATCH] travis-ci: build with GCC 4.8 as well\n\nC99 'for' loop initial declaration, i.e. 'for (int i = 0; i < n; i++)',\nis not allowed in Git's codebase yet, to maintain compatibility with\nsome older compilers.\n\nOur Travis CI builds used to catch 'for' loop initial declarations,\nbecause the GETTEXT_POISON job has always built Git with the default\n'cc', which in Travis CI's previous default Linux image (based on\nUbuntu 14.04 Trusty) is GCC 4.8, and that GCC version errors out on\nthis construct (not only with DEVELOPER=1, but with our default CFLAGS\nas well).  Alas, that's not the case anymore, becase after 14.04's EOL\nTravis CI's current default Linux image is based on Ubuntu 16.04\nXenial [1] and its default 'cc' is now GCC 5.4, which, just like all\nlater GCC and Clang versions, simply accepts this construct, even if\nwe don't explicitly specify '-std=c99'.\n\nIdeally we would adjust our CFLAGS used with DEVELOPER=1 to catch this\nundesired construct already when contributors build Git on their own\nmachines.  Unfortunately, however, there seems to be no compiler\noption that would catch only this particular construct without choking\non many other things, e.g. while a later compiler with '-std=c90'\nand/or '-ansi' does catch this construct, it can't build Git because\nof several screenfulls of other errors.\n\nAdd the 'linux-gcc-4.8' job to Travis CI, in order to build Git with\nGCC 4.8, and thus to timely catch any 'for' loop initial declarations.\nTo catch those it's sufficient to only build Git with GCC 4.8, so\ndon't run the test suite in this job, because 'make test' takes rather\nlong [2], and it's already run five times in other jobs, so we\nwouldn't get our time's worth.\n\n[1] The Azure Pipelines builds have been using Ubuntu 16.04 images\n    from the start, so I belive they never caught 'for' loop initial\n    declarations.\n\n[2] On Travis CI 'make test' alone would take about 9 minutes in this\n    new job (without running httpd, Subversion, and P4 tests).  For\n    comparison, starting the job and building Git with GCC 4.8 takes\n    only about 2 minutes.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n .travis.yml               |  4 ++++\n ci/run-build-and-tests.sh | 17 +++++++++++++----\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex ffb1bc46f2..fc5730b085 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -21,6 +21,10 @@ matrix:\n       compiler:\n       addons:\n       before_install:\n+    - env: jobname=linux-gcc-4.8\n+      os: linux\n+      dist: trusty\n+      compiler:\n     - env: jobname=Linux32\n       os: linux\n       compiler:\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex cdd2913440..ff0ef7f08e 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\\\.prove \"$(cygpath -aw \"$cache_dir/.prove\")\";;\n esac\n \n make\n-make test\n-if test \"$jobname\" = \"linux-gcc\"\n-then\n+case \"$jobname\" in\n+linux-gcc)\n+\tmake test\n \texport GIT_TEST_SPLIT_INDEX=yes\n \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\n \texport GIT_TEST_OE_SIZE=10\n@@ -21,7 +21,16 @@ then\n \texport GIT_TEST_COMMIT_GRAPH=1\n \texport GIT_TEST_MULTI_PACK_INDEX=1\n \tmake test\n-fi\n+\t;;\n+linux-gcc-4.8)\n+\t# Don't run the tests; we only care about whether Git can be\n+\t# built with GCC 4.8, as it errors out on some undesired (C99)\n+\t# constructs that newer compilers seem to quietly accept.\n+\t;;\n+*)\n+\tmake test\n+\t;;\n+esac\n \n check_unignored_build_artifacts\n \n-- \n2.22.0.810.g50207c7d84\n\n\n"},{"id":"379063","messageId":"xmqqa7dbz5h7.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190718152234.GI20404@szeder.dev","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-18T16:12:52Z","receivedAt":"2019-07-18T16:12:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> With that patch issues like this could be caught earlier, while they\n> are only in 'pu' but not yet in 'next'.  But do we really want to do\n> that, is that the right tradeoff?\n\nI am sort of in favor of having at least one build with an older\ncompiler without \"-std=c99\", like the set-up you are proposing.\nI got an impression from an earlier message that Jonathan's\npreference is the opposite.  I'd prefer to hear opinions from\nothers, too.\n\nThe main reason why we accept other new features like trailing comma\nin enum def and designated initializers while rejecting for loop\ninit is because there apparently are those who build Git with\ncompilers that accept & reject these combinations of features and\nwho care enough to report compilation failure from their build.  And\napparently gcc4.8 can serve as a representative \"old\" compiler,\nso...\n"},{"id":"379064","messageId":"CAPig+cT5nbXcsj+CH38FwfE0m+S6VffG_Zz8iz0zpw49zDwd+Q@mail.gmail.com","threadId":"51416","inReplyTo":"20190718152234.GI20404@szeder.dev","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-07-18T16:29:42Z","receivedAt":"2019-07-18T16:29:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 18, 2019 at 11:22 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> C99 'for' loop initial declaration, i.e. 'for (int i = 0; i < n; i++)',\n> is not allowed in Git's codebase yet, to maintain compatibility with\n> some older compilers.\n> [...]\n> [1] The Azure Pipelines builds have been using Ubuntu 16.04 images\n>     from the start, so I belive they never caught 'for' loop initial\n>     declarations.\n\ns/belive/believe/\n\n> [2] On Travis CI 'make test' alone would take about 9 minutes in this\n>     new job (without running httpd, Subversion, and P4 tests).  For\n>     comparison, starting the job and building Git with GCC 4.8 takes\n>     only about 2 minutes.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n"},{"id":"379089","messageId":"20190718234647.GJ20404@szeder.dev","threadId":"51416","inReplyTo":"xmqqa7dbz5h7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-18T23:46:47Z","receivedAt":"2019-07-18T23:46:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jul 18, 2019 at 09:12:52AM -0700, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > With that patch issues like this could be caught earlier, while they\n> > are only in 'pu' but not yet in 'next'.  But do we really want to do\n> > that, is that the right tradeoff?\n> \n> I am sort of in favor of having at least one build with an older\n> compiler without \"-std=c99\", like the set-up you are proposing.\n\nAlternatively, we could sort-of restore the old state of the\nGETTEXT_POISON job, i.e. build Git with GCC 4.8 in that job.\n\"Sort-of\", because we don't necessarily have to go back to the Ubuntu\n14.04 based Linux image, because GCC 4.8 is packaged in 16.04 as well,\nso it's just an 'apt-get install gcc-4.8' away.\n\n"},{"id":"379091","messageId":"20190719011516.GA251102@google.com","threadId":"51416","inReplyTo":"xmqqv9w01wdn.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC/PATCH] CodingGuidelines: spell out post-C89 rules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-07-19T01:15:16Z","receivedAt":"2019-07-19T01:15:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> -- >8 --\n> Even though we have been sticking to C89, there are a few handy\n> features we borrow from more recent C language in our codebase after\n> trying them in weather balloons and saw that nobody screamed.\n>\n> Spell them out.\n>\n> While at it, extend the existing variable declaration rule a bit to\n> read better with the newly spelled out rule for the for loop.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  Documentation/CodingGuidelines | 25 ++++++++++++++++++++++---\n>  1 file changed, 22 insertions(+), 3 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nThanks.\n"},{"id":"379092","messageId":"20190719013128.GB251102@google.com","threadId":"51416","inReplyTo":"20190718152234.GI20404@szeder.dev","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-07-19T01:31:28Z","receivedAt":"2019-07-19T01:31:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> I expected that this will eventually happen after Travis CI's default\n> Linux image recently changed from Ubuntu 14.04 to 16.04; explanation\n> in the commit message below.\n>\n> With that patch issues like this could be caught earlier, while they\n> are only in 'pu' but not yet in 'next'.  But do we really want to do\n> that, is that the right tradeoff?  Dunno...  Adding a dedicated CI job\n> just to check that there are no 'for' loop initial declarations seems\n> kind of excessive, even if it only builds but doesn't run the test\n> suite.  And I don't know whether there are any other undesired (\"too\n> new\") constructs that GCC 4.8 would catch but later compilers quietly\n> accept.\n\nThis makes sense to me.  Not really for the 'for' loop declaration\naspect: for that, I'd want some more specialized tool that allows\nturning on such a check specifically.  But more because Ubuntu trusty\nis still a platform that some people use (though hopefully not for\nlong), so it's helpful as a representative old platform to see if we\nbreak the build on it.\n\n[...]\n> [2] On Travis CI 'make test' alone would take about 9 minutes in this\n>     new job (without running httpd, Subversion, and P4 tests).  For\n>     comparison, starting the job and building Git with GCC 4.8 takes\n>     only about 2 minutes.\n\nNice.  In an ideal world there would be some kind of \"make fasttest\"\nthat runs some fast subset of tests.  But this seems pretty safe.\n\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  .travis.yml               |  4 ++++\n>  ci/run-build-and-tests.sh | 17 +++++++++++++----\n>  2 files changed, 17 insertions(+), 4 deletions(-)\n\nThanks.\n\n[...]\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\\\.prove \"$(cygpath -aw \"$cache_dir/.prove\")\";;\n>  esac\n>  \n>  make\n> -make test\n> -if test \"$jobname\" = \"linux-gcc\"\n> -then\n> +case \"$jobname\" in\n> +linux-gcc)\n> +\tmake test\n>  \texport GIT_TEST_SPLIT_INDEX=yes\n>  \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\n>  \texport GIT_TEST_OE_SIZE=10\n> @@ -21,7 +21,16 @@ then\n>  \texport GIT_TEST_COMMIT_GRAPH=1\n>  \texport GIT_TEST_MULTI_PACK_INDEX=1\n>  \tmake test\n> -fi\n> +\t;;\n> +linux-gcc-4.8)\n> +\t# Don't run the tests; we only care about whether Git can be\n> +\t# built with GCC 4.8, as it errors out on some undesired (C99)\n> +\t# constructs that newer compilers seem to quietly accept.\n> +\t;;\n> +*)\n> +\tmake test\n> +\t;;\n> +esac\n\nDoes what it says on the tin.\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"379093","messageId":"CAPUEsphnCvK+RZ+h30ZarA1zo9yZ=ndEBrcAbKGf4W92j647vA@mail.gmail.com","threadId":"51416","inReplyTo":"20190719013128.GB251102@google.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-19T04:49:47Z","receivedAt":"2019-07-19T04:50:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Jul 18, 2019 at 6:31 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> This makes sense to me.  Not really for the 'for' loop declaration\n> aspect: for that, I'd want some more specialized tool that allows\n> turning on such a check specifically.  But more because Ubuntu trusty\n> is still a platform that some people use (though hopefully not for\n> long), so it's helpful as a representative old platform to see if we\n> break the build on it.\n\nFWIW this also breaks Centos 7 using gcc 4.8.5, as well as the one\noriginally reported (Centos 6), and anything else that uses gcc 4\n(tested up to 4.9.4)\n\nCarlo\n"},{"id":"379112","messageId":"xmqqwogdvnsx.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"CAPUEsphnCvK+RZ+h30ZarA1zo9yZ=ndEBrcAbKGf4W92j647vA@mail.gmail.com","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-19T19:15:10Z","receivedAt":"2019-07-19T19:15:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> On Thu, Jul 18, 2019 at 6:31 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>>\n>> This makes sense to me.  Not really for the 'for' loop declaration\n>> aspect: for that, I'd want some more specialized tool that allows\n>> turning on such a check specifically.  But more because Ubuntu trusty\n>> is still a platform that some people use (though hopefully not for\n>> long), so it's helpful as a representative old platform to see if we\n>> break the build on it.\n>\n> FWIW this also breaks Centos 7 using gcc 4.8.5, as well as the one\n> originally reported (Centos 6), and anything else that uses gcc 4\n> (tested up to 4.9.4)\n\nThanks.\n"},{"id":"379418","messageId":"20190727084348.GO20404@szeder.dev","threadId":"51416","inReplyTo":"20190718152234.GI20404@szeder.dev","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-27T08:43:48Z","receivedAt":"2019-07-27T08:43:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Junio,\n\nOn Thu, Jul 18, 2019 at 05:22:34PM +0200, SZEDER Gábor wrote:\n> Subject: [PATCH] travis-ci: build with GCC 4.8 as well\n\nThis patch conflicts with topic 'js/trace2-json-schema', and the\ncurrent conflict resolution in 'pu' is not quite correct.\n\n> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> index cdd2913440..ff0ef7f08e 100755\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\\\.prove \"$(cygpath -aw \"$cache_dir/.prove\")\";;\n>  esac\n>  \n>  make\n> -make test\n> -if test \"$jobname\" = \"linux-gcc\"\n> -then\n> +case \"$jobname\" in\n> +linux-gcc)\n> +\tmake test\n\nThis 'make test' here is important, but the confict resolution\naccidentally removed it.\n\n>  \texport GIT_TEST_SPLIT_INDEX=yes\n>  \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\n>  \texport GIT_TEST_OE_SIZE=10\n> @@ -21,7 +21,16 @@ then\n>  \texport GIT_TEST_COMMIT_GRAPH=1\n>  \texport GIT_TEST_MULTI_PACK_INDEX=1\n>  \tmake test\n> -fi\n> +\t;;\n> +linux-gcc-4.8)\n> +\t# Don't run the tests; we only care about whether Git can be\n> +\t# built with GCC 4.8, as it errors out on some undesired (C99)\n> +\t# constructs that newer compilers seem to quietly accept.\n> +\t;;\n> +*)\n> +\tmake test\n> +\t;;\n> +esac\n>  \n>  check_unignored_build_artifacts\n>  \n> -- \n> 2.22.0.810.g50207c7d84\n> \n> \n"},{"id":"379419","messageId":"xmqq4l37mp8d.fsf@gitster-ct.c.googlers.com","threadId":"51416","inReplyTo":"20190727084348.GO20404@szeder.dev","subject":"Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-27T16:11:46Z","receivedAt":"2019-07-27T16:11:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> Junio,\n>\n> On Thu, Jul 18, 2019 at 05:22:34PM +0200, SZEDER Gábor wrote:\n>> Subject: [PATCH] travis-ci: build with GCC 4.8 as well\n>\n> This patch conflicts with topic 'js/trace2-json-schema', and the\n> current conflict resolution in 'pu' is not quite correct.\n\nThanks.\n\n\"git diff ...MERGE_HEAD\" during a merge of js/trace2-json-schema\ngives me this patch:\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex cdd2913440..ec38bf379a 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -14,6 +14,8 @@ make\n make test\n if test \"$jobname\" = \"linux-gcc\"\n then\n+\tmake -C t/trace_schema_validator\n+\texport GIT_TRACE2_EVENT=$(mktemp)\n \texport GIT_TEST_SPLIT_INDEX=yes\n \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\n \texport GIT_TEST_OE_SIZE=10\n@@ -21,6 +23,10 @@ then\n \texport GIT_TEST_COMMIT_GRAPH=1\n \texport GIT_TEST_MULTI_PACK_INDEX=1\n \tmake test\n+\tt/trace_schema_validator/trace_schema_validator \\\n+\t\t--trace2_event_file=${GIT_TRACE2_EVENT} \\\n+\t\t--schema_file=t/trace_schema_validator/strict_schema.json \\\n+\t\t--progress=10000\n fi\n \n check_unignored_build_artifacts\n\ni.e. they want to run an extra make in that validator directory,\nexport another environment variable, and then run the validator\n*after* running the normal \"make test\", in linux-gcc job.\n\n>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n>> index cdd2913440..ff0ef7f08e 100755\n>> --- a/ci/run-build-and-tests.sh\n>> +++ b/ci/run-build-and-tests.sh\n>> @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\\\.prove \"$(cygpath -aw \"$cache_dir/.prove\")\";;\n>>  esac\n>>  \n>>  make\n>> -make test\n>> -if test \"$jobname\" = \"linux-gcc\"\n>> -then\n>> +case \"$jobname\" in\n>> +linux-gcc)\n>> +\tmake test\n>\n> This 'make test' here is important, but the confict resolution\n> accidentally removed it.\n\nRight.  Thanks for spotting.\n\nI can see in \"git diff pu~2 pu~1\" that indeed the first 'make test'\nthat we want to run without any of these environment variables is\nlost in the merge.  We want to run two tests, with or without these\nenvironment variables, and the validator wants to piggyback on the\nsecond one.\n\nWill fix in the meantime, but I was expecting that this \"validator\nin CI\" business to be redone differently, perhaps with a different\nvalidator implementation and either in a separate job or just part\nof an existing job but trace enabled only for some subset of the\ntests and/or only for new tests specifically written for trace\ncoverage, so after that happens this may turn into a moot point.\n\nThe change the merge brings in to the file now reads like this.\n\nThanks, again.\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex ff0ef7f08e..35ff4d3038 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -14,6 +14,9 @@ make\n case \"$jobname\" in\n linux-gcc)\n \tmake test\n+\n+\tmake -C t/trace_schema_validator\n+\texport GIT_TRACE2_EVENT=$(mktemp)\n \texport GIT_TEST_SPLIT_INDEX=yes\n \texport GIT_TEST_FULL_IN_PACK_ARRAY=true\n \texport GIT_TEST_OE_SIZE=10\n@@ -21,6 +24,11 @@ linux-gcc)\n \texport GIT_TEST_COMMIT_GRAPH=1\n \texport GIT_TEST_MULTI_PACK_INDEX=1\n \tmake test\n+\n+\tt/trace_schema_validator/trace_schema_validator \\\n+\t\t--trace2_event_file=${GIT_TRACE2_EVENT} \\\n+\t\t--schema_file=t/trace_schema_validator/strict_schema.json \\\n+\t\t--progress=10000\n \t;;\n linux-gcc-4.8)\n \t# Don't run the tests; we only care about whether Git can be\n"}]}