{"thread":{"id":"38305","subject":"[PATCHv12 10/10] t5543-atomic-push.sh: add basic tests for atomic pushes","startedAt":"2015-01-08T03:23:14Z","lastAt":"2015-01-13T00:08:51Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"254442","messageId":"1420687404-13997-1-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":null,"subject":"[PATCHv12 00/10] atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:14Z","receivedAt":"2015-01-08T03:23:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There wasn't discussion within the last 24 hours and we were discussing only \nabout minor changes. Changes compared to v9 (the last time I completely sent the\nseries) is only found in the first 2 patches, where we had a back and forth \nabout naming the method warn_if_skipped_connectivity_check and its behavior.\nI'm mainly sending this as a whole series, so Junio can pick it up easier\nas opposed to finding the latest version of each patch himself.\n\nThis patch series adds a flag to git push to update the remote refs atomically.\nThis series applies on top of origin/mh/reflog-expire\nIt can also be found at https://github.com/stefanbeller/git/tree/atomic-push-v12\n\nAny comment is welcome!\n\nThanks,\nStefan\n\nRonnie Sahlberg (3):\n  receive-pack.c: negotiate atomic push support\n  send-pack.c: add --atomic command line argument\n  push.c: add an --atomic argument\n\nStefan Beller (7):\n  receive-pack.c: shorten the execute_commands loop over all commands\n  receive-pack.c: die instead of error in case of possible future bug\n  receive-pack.c: move iterating over all commands outside\n    execute_commands\n  receive-pack.c: move transaction handling in a central place\n  receive-pack.c: add execute_commands_atomic function\n  send-pack: rename ref_update_to_be_sent to check_to_send_update\n  t5543-atomic-push.sh: add basic tests for atomic pushes\n\n Documentation/git-push.txt                        |   7 +-\n Documentation/git-send-pack.txt                   |   7 +-\n Documentation/technical/protocol-capabilities.txt |  13 +-\n builtin/push.c                                    |   5 +\n builtin/receive-pack.c                            | 165 ++++++++++++++----\n builtin/send-pack.c                               |   6 +-\n remote.h                                          |   3 +-\n send-pack.c                                       |  65 +++++++-\n send-pack.h                                       |   3 +-\n t/t5543-atomic-push.sh                            | 194 ++++++++++++++++++++++\n transport.c                                       |   5 +\n transport.h                                       |   1 +\n 12 files changed, 424 insertions(+), 50 deletions(-)\n create mode 100755 t/t5543-atomic-push.sh\n\n-- \n2.2.1.62.g3f15098\n"},{"id":"254446","messageId":"1420687404-13997-2-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 01/10] receive-pack.c: shorten the execute_commands loop over all commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:15Z","receivedAt":"2015-01-08T03:23:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Make the main \"execute_commands\" loop in receive-pack easier to read\nby splitting out some steps into helper functions. The new helper\n'should_process_cmd' checks if a ref update is unnecessary, whether\ndue to an error having occurred or for another reason. The helper\n'warn_if_skipped_connectivity_check' warns if we have forgotten to\nrun a connectivity check on a ref which is shallow for the client\nwhich would be a bug.\n\nThis will help us to duplicate less code in a later patch when we make\na second copy of the \"execute_commands\" loop.\n\nNo functional change intended.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v12:\n    * no changes\n    v11:\n    * rename assure_connectivity_checked to warn_if_skipped_connectivity_check\n    \n    >> This is why I choose the word assure.\n    >If this patch depends on the next one, would it make sense to put them\n    >in the opposite order?\n    \n    With the next patch applied which dies if the assumption doesn't hold,\n    assure/assume sounds to me as if it describes the siuation well.\n    \n    > My personal preference would be to refactor the preceding code to make\n    > the check unnecessary.\n    \n    The way I understand it, the shallow case is spread out all over the place\n    not by choise but because it doesn't work better. So the original author\n    (Duy) was wise enough to put checks in place because knowing it is fragile\n    and may break in the future?\n    This series doesn't intend to refactor the shallow case.\n    \n    So I picked up warn_if_skipped_connectivity_check as I'm ok with that.\n    \n    v10:\n    * rename check_shallow_bugs to assure_connectivity_checked.\n    * reword commit message.\n    \n    v9:\n    * simplified should_process_cmd to be a one liner\n    * check_shallow_bugs doesn't check of shallow_update being set, rather\n      the function is just called if that option is set.\n    \n    v8: no change\n    \n    v7:\n     new in v7 as in v7 I'd split up the previous\n     [PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes\n     as suggested by Eric.\n    \n     This is pretty much\n    > patch 1: Factor out code into helper functions which will be needed by\n    > the upcoming atomic and non-atomic worker functions. Example helpers:\n    > 'cmd->error_string' and cmd->skip_update' check; and the\n    > 'si->shallow_ref[cmd->index]' check and handling.\n\n builtin/receive-pack.c | 43 +++++++++++++++++++++++++++----------------\n 1 file changed, 27 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 32fc540..2ebaf66 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1042,11 +1042,34 @@ static void reject_updates_to_hidden(struct command *commands)\n \t}\n }\n \n+static int should_process_cmd(struct command *cmd)\n+{\n+\treturn !cmd->error_string && !cmd->skip_update;\n+}\n+\n+static void warn_if_skipped_connectivity_check(struct command *commands,\n+\t\t\t\t\t       struct shallow_info *si)\n+{\n+\tstruct command *cmd;\n+\tint checked_connectivity = 1;\n+\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {\n+\t\t\terror(\"BUG: connectivity check has not been run on ref %s\",\n+\t\t\t      cmd->ref_name);\n+\t\t\tchecked_connectivity = 0;\n+\t\t}\n+\t}\n+\tif (!checked_connectivity)\n+\t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n+\t\t      \"If there are errors, try to remove \"\n+\t\t      \"the reported refs above\");\n+}\n+\n static void execute_commands(struct command *commands,\n \t\t\t     const char *unpacker_error,\n \t\t\t     struct shallow_info *si)\n {\n-\tint checked_connectivity;\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n \tstruct iterate_data data;\n@@ -1077,27 +1100,15 @@ static void execute_commands(struct command *commands,\n \tfree(head_name_to_free);\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n \n-\tchecked_connectivity = 1;\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (cmd->error_string)\n-\t\t\tcontinue;\n-\n-\t\tif (cmd->skip_update)\n+\t\tif (!should_process_cmd(cmd))\n \t\t\tcontinue;\n \n \t\tcmd->error_string = update(cmd, si);\n-\t\tif (shallow_update && !cmd->error_string &&\n-\t\t    si->shallow_ref[cmd->index]) {\n-\t\t\terror(\"BUG: connectivity check has not been run on ref %s\",\n-\t\t\t      cmd->ref_name);\n-\t\t\tchecked_connectivity = 0;\n-\t\t}\n \t}\n \n-\tif (shallow_update && !checked_connectivity)\n-\t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n-\t\t      \"If there are errors, try to remove \"\n-\t\t      \"the reported refs above\");\n+\tif (shallow_update)\n+\t\twarn_if_skipped_connectivity_check(commands, si);\n }\n \n static struct command **queue_command(struct command **tail,\n-- \n2.2.1.62.g3f15098\n"},{"id":"254445","messageId":"1420687404-13997-3-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 02/10] receive-pack.c: die instead of error in case of possible future bug","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:16Z","receivedAt":"2015-01-08T03:23:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Discussion on the previous patch revealed we rather want to err on the\nsafe side. To do so we need to stop receive-pack in case of the possible\nfuture bug when connectivity is not checked on a shallow push.\n\nAlso while touching that code we considered that removing the reported\nrefs may be harmful in some situations. Sound the message more like a\n\"This Cannot Happen, Please Investigate!\" instead of giving advice to\nremove refs.\n\nSuggested-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v12:\n    * no advice in case we die.\n    \n    v11:\n    * only die at the end so the loop works out for all refs.\n    * Remove the advice to delete refs.\n    \n    > If the message says\n    >        fatal: BUG: connectivity check skipped???\n    > then it has exactly the right amount of information to tell me what to\n    > do.  Now I have\n    > - a short string to grep for in the source code (or on the web) to\n    >    find out what happened\n    \n    And do a git blame to see previous versions?\n    \n    I am not so sure of this patch any more as it actually stops people\n    doing work if they want to do so. (They may deliberately choose to\n    ignore the BUG:... message, because of a deadline in 2 hours.)\n    \n    So I do think this helps on getting people to report the bug in the\n    future if it arises faster, but on the other hand if we assume the\n    faulty hardware scenario and the deadline we actually stop people\n    from getting their desired work done.\n    \n    This patch doesn't actually relate to the topic of the series\n    (atomic pushes), but is a cleanup-as-we-go patch. If we need to\n    have further discussion on this, I'd rather want to delay this patch\n    and have a follow up on top of the atomic series.\n    \n    Thanks,\n    Stefan\n    \n    v10:\n    * new in v10.\n\n builtin/receive-pack.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2ebaf66..3bdb158 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1061,9 +1061,7 @@ static void warn_if_skipped_connectivity_check(struct command *commands,\n \t\t}\n \t}\n \tif (!checked_connectivity)\n-\t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n-\t\t      \"If there are errors, try to remove \"\n-\t\t      \"the reported refs above\");\n+\t\tdie(\"BUG: connectivity check skipped???\");\n }\n \n static void execute_commands(struct command *commands,\n-- \n2.2.1.62.g3f15098\n"},{"id":"254448","messageId":"1420687404-13997-4-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 03/10] receive-pack.c: move iterating over all commands outside execute_commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:17Z","receivedAt":"2015-01-08T03:23:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This commit allows us in a later patch to easily distinguish between\nthe non atomic way to update the received refs and the atomic way which\nis introduced in a later patch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n    no changes\n    \n    v9:\n     new and shiny. But makes the next patch easier to review.\n\n builtin/receive-pack.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 3bdb158..0ccfb3d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1064,6 +1064,18 @@ static void warn_if_skipped_connectivity_check(struct command *commands,\n \t\tdie(\"BUG: connectivity check skipped???\");\n }\n \n+static void execute_commands_non_atomic(struct command *commands,\n+\t\t\t\t\tstruct shallow_info *si)\n+{\n+\tstruct command *cmd;\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (!should_process_cmd(cmd))\n+\t\t\tcontinue;\n+\n+\t\tcmd->error_string = update(cmd, si);\n+\t}\n+}\n+\n static void execute_commands(struct command *commands,\n \t\t\t     const char *unpacker_error,\n \t\t\t     struct shallow_info *si)\n@@ -1098,12 +1110,7 @@ static void execute_commands(struct command *commands,\n \tfree(head_name_to_free);\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (!should_process_cmd(cmd))\n-\t\t\tcontinue;\n-\n-\t\tcmd->error_string = update(cmd, si);\n-\t}\n+\texecute_commands_non_atomic(commands, si);\n \n \tif (shallow_update)\n \t\twarn_if_skipped_connectivity_check(commands, si);\n-- \n2.2.1.62.g3f15098\n"},{"id":"254441","messageId":"1420687404-13997-5-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 04/10] receive-pack.c: move transaction handling in a central place","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:18Z","receivedAt":"2015-01-08T03:23:18Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This moves all code related to transactions into the\nexecute_commands_non_atomic function. This includes\nbeginning and committing the transaction as well as\ndealing with the errors which may occur during the\nbegin and commit phase of a transaction.\n\nNo functional changes intended.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n     no changes\n    \n    v9:\n     This was split up into the patch before and this one.\n     This patch only deals with the transactions now.\n    \n    v8:\n     move execute_commands_loop before execute_commands, so it compiles/links\n     without warnings.\n    \n    v7:\n     new in v7, this is part of the previous \"[PATCH 4/7]\n     receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes\"\n    \n    This covers the suggestion of patch 2 and 3 by Eric\n    > patch 2: Factor out the main 'for' loop of execute_commands() into a\n    > new function. This new function will eventually become\n    > execute_commands_non_atomic(). At this point, execute_commands() is\n    > pretty much in its final form with the exception of the upcoming 'if\n    > (use_atomic)' conditional.\n    > patch 3: Morph the function extracted in patch 2 into\n    > execute_commands_non_atomic() by adding transaction handling inside\n    > the 'for' loop (and applying the changes from the early part of the\n    > patch which go along with that).\n\n builtin/receive-pack.c | 51 ++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 37 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0ccfb3d..96e56a7 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -66,6 +66,7 @@ static const char *NONCE_SLOP = \"SLOP\";\n static const char *nonce_status;\n static long nonce_stamp_slop;\n static unsigned long nonce_stamp_slop_limit;\n+static struct ref_transaction *transaction;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -821,6 +822,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n \n \tif (is_null_sha1(new_sha1)) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n \t\tif (!parse_object(old_sha1)) {\n \t\t\told_sha1 = NULL;\n \t\t\tif (ref_exists(name)) {\n@@ -830,35 +832,36 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\tcmd->did_not_exist = 1;\n \t\t\t}\n \t\t}\n-\t\tif (delete_ref(namespaced_name, old_sha1, 0)) {\n-\t\t\trp_error(\"failed to delete %s\", name);\n+\t\tif (ref_transaction_delete(transaction,\n+\t\t\t\t\t   namespaced_name,\n+\t\t\t\t\t   old_sha1,\n+\t\t\t\t\t   0, old_sha1 != NULL,\n+\t\t\t\t\t   \"push\", &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n \t\t\treturn \"failed to delete\";\n \t\t}\n+\t\tstrbuf_release(&err);\n \t\treturn NULL; /* good */\n \t}\n \telse {\n \t\tstruct strbuf err = STRBUF_INIT;\n-\t\tstruct ref_transaction *transaction;\n-\n \t\tif (shallow_update && si->shallow_ref[cmd->index] &&\n \t\t    update_shallow_ref(cmd, si))\n \t\t\treturn \"shallow error\";\n \n-\t\ttransaction = ref_transaction_begin(&err);\n-\t\tif (!transaction ||\n-\t\t    ref_transaction_update(transaction, namespaced_name,\n-\t\t\t\t\t   new_sha1, old_sha1, 0, 1, \"push\",\n-\t\t\t\t\t   &err) ||\n-\t\t    ref_transaction_commit(transaction, &err)) {\n-\t\t\tref_transaction_free(transaction);\n-\n+\t\tif (ref_transaction_update(transaction,\n+\t\t\t\t\t   namespaced_name,\n+\t\t\t\t\t   new_sha1, old_sha1,\n+\t\t\t\t\t   0, 1, \"push\",\n+\t\t\t\t\t   &err)) {\n \t\t\trp_error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n+\n \t\t\treturn \"failed to update ref\";\n \t\t}\n-\n-\t\tref_transaction_free(transaction);\n \t\tstrbuf_release(&err);\n+\n \t\treturn NULL; /* good */\n \t}\n }\n@@ -1068,12 +1071,32 @@ static void execute_commands_non_atomic(struct command *commands,\n \t\t\t\t\tstruct shallow_info *si)\n {\n \tstruct command *cmd;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (!should_process_cmd(cmd))\n \t\t\tcontinue;\n \n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tcmd->error_string = \"transaction failed to start\";\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tcmd->error_string = update(cmd, si);\n+\n+\t\tif (!cmd->error_string\n+\t\t    && ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tcmd->error_string = \"failed to update ref\";\n+\t\t}\n+\t\tref_transaction_free(transaction);\n \t}\n+\n+\tstrbuf_release(&err);\n }\n \n static void execute_commands(struct command *commands,\n-- \n2.2.1.62.g3f15098\n"},{"id":"254440","messageId":"1420687404-13997-6-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 05/10] receive-pack.c: add execute_commands_atomic function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:19Z","receivedAt":"2015-01-08T03:23:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This introduces the new function execute_commands_atomic which will use\none atomic transaction for all updates. The default behavior is still\nthe old non atomic way, one ref at a time. This is to cause as little\ndisruption as possible to existing clients. It is unknown if there are\nclient scripts that depend on the old non-atomic behavior so we make it\nopt-in for now.\n\nA later patch will add the possibility to actually use the functionality\nadded by this patch. For now use_atomic is always 0.\n\nInspired-by: Ronnie Sahlberg <sahlberg@google.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n    * no changes\n    \n    v9:\n    \tBecause the patches have been reordered, we introduce use_atomic\n    \tin this patch, but don't touch it.\n    \n    > Minor comment: This cleanup code is repeated in both the success and\n    > fail branches. It _might_ (or not) be a bit cleaner and more\n    > maintainable to replace the above three lines with:\n    \n    \tPersonally I have no strong opinion about that one.\n    \tI implemented it as you suggested, having a cleanup and a failure\n    \tlabel. It seems to look ok and has less lines of code.\n    \n    \tHowever writing this code made me feel a bit like producing\n    \tspagetti code here. (\"Goto is evil!\", I accepted goto as a\n    \tconvenient way to have one common cleanup or failure before\n    \texiting the function before, but now we have two jump targets.)\n    \n    Changes in v8:\n    \tremoved superflous \"}\" to make it compile again\n    \n    Changes in v7:\n    \tEric suggested to replace \"[PATCH 4/7] receive-pack.c:\n    \treceive-pack.c: use a single ref_transaction for atomic pushes\"\n    \tby smaller patches\n    \tThis is the last patch replacing said large commit.\n    \n    Changes in v6:\n    \tThis is a complete rewrite of the patch essentially.\n    \tEric suggested to split up the flow into functions, so\n    \tit is easier to read. It's more lines of code, but indeed\n    \tit's easier to follow. Thanks Eric!\n    \n    \tNote there is another patch following this one\n    \tmoving the helper functions above execute_commands.\n    \tI just choose the order of the functions in this patch\n    \tto have a better diff (just inserting the call to the function\n    \texecute_commands_non_atomic and that function directly follows.)\n    \tThe next patch of the series will move that up.\n    \n    \tBecause of the rewrite and the fixes of the previous five\n    \tversions there is not much left of Ronnies original patch,\n    \tso I'll claim authorship of this one.\n    \n    Changes v1 -> v2:\n    \t* update(...) assumes to be always in a transaction\n    \t* Caring about when to begin/commit transactions is put\n    \t  into execute_commands\n    v2->v3:\n    \t* meditated about the error flow. Now we always construct a local\n    \t  strbuf err if required. Then the flow is easier to follow and\n    \t  destruction of it is performed nearby.\n    \t* early return in execute_commands if transaction_begin fails.\n    \n    v3->v4:\n    \t* revamp logic again. This should keep the non atomic behavior\n    \t  as is (in case of error say so, in non error case just free the\n    \t  transaction). In the atomic case we either do nothing (when no error),\n    \t  or abort with the goto.\n    \n    \t\tif (!cmd->error_string) {\n    \t\t\tif (!use_atomic\n    \t\t\t    && ref_transaction_commit(transaction, &err)) {\n    \t\t\t\tref_transaction_free(transaction);\n    \t\t\t\trp_error(\"%s\", err.buf);\n    \t\t\t\tstrbuf_release(&err);\n    \t\t\t\tcmd->error_string = \"failed to update ref\";\n    \t\t\t}\n    \t\t} else if (use_atomic) {\n    \t\t\tgoto atomic_failure;\n    \t\t} else {\n    \t\t\tref_transaction_free(transaction);\n    \t\t}\n    \n    \t * Having the goto directly there when checking for cmd->error_string,\n    \t   we don't need to do it again, so the paragraph explaining the error\n    \t   checking is gone as well. (Previous patch had the following, this is\n    \t   put at the end of the function, where the goto jumps to and the comment\n    \t   has been dropped.\n    +\t\t/*\n    +\t\t * update(...) may abort early (i.e. because the hook refused to\n    +\t\t * update that ref) which then doesn't even record a transaction\n    +\t\t * regarding that ref. Make sure all commands are without error\n    +\t\t * and then commit atomically.\n    +\t\t */\n    +\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n    +\t\t\tif (cmd->error_string)\n    +\t\t\t\tbreak;\n    \n    v4->v5:\n    Eric wrote:\n    > Repeating from my earlier review[1]: If the 'pre-receive' hook\n    > \"declines\", then this transaction is left dangling (and its resources\n    > leaked).\n    \n    You're right. The initialization of the transaction is now\n    near the actual loop after the pre receive hook.\n    \n    > The !use_atomic case (below), calls this error \"failed to start\n    > transaction\", not merely \"transaction error\".\n    \n    ok, now both are \"transaction failed to start\".\n    In all cases where these generic errors are reported,\n    we do have a rp_error(...) with details.\n    \n    > Furthermore, in the use_atomic case (also below), when a commit fails,\n    > you assign err.buf to cmd->error_string rather than a generic\n    > \"transaction error\" message. What differs between these cases which\n    > makes the generic message preferable here over the more specific\n    > err.buf message?\n    \n    They are the same now.\n    \n    > Repeating from my earlier review[1]: This is leaking 'transaction' for\n    > each successful commit (and only freeing it upon commit error).\n    \n    Right. I thought I had it covered with the else clause. Of course not.\n    \n    > At the end of this function, strbuf_release(&err) is invoked, which\n    > leaves all these cmd->error_strings dangling.\n    \n    I removed all assignments of err.buf now.\n    \n    > goto's can help simplify error-handling when multiple conditional\n    > branches need to perform common cleanup, however, this label\n    > corresponds to only a single goto statement.\n    \n    moved up again.\n\n builtin/receive-pack.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 46 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 96e56a7..362d33f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n static int report_status;\n static int use_sideband;\n+static int use_atomic;\n static int quiet;\n static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n@@ -1095,7 +1096,48 @@ static void execute_commands_non_atomic(struct command *commands,\n \t\t}\n \t\tref_transaction_free(transaction);\n \t}\n+\tstrbuf_release(&err);\n+}\n+\n+static void execute_commands_atomic(struct command *commands,\n+\t\t\t\t\tstruct shallow_info *si)\n+{\n+\tstruct command *cmd;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tconst char *reported_error = \"atomic push failure\";\n+\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\tstrbuf_reset(&err);\n+\t\treported_error = \"transaction failed to start\";\n+\t\tgoto failure;\n+\t}\n+\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (!should_process_cmd(cmd))\n+\t\t\tcontinue;\n+\n+\t\tcmd->error_string = update(cmd, si);\n+\n+\t\tif (cmd->error_string)\n+\t\t\tgoto failure;\n+\t}\n \n+\tif (ref_transaction_commit(transaction, &err)) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\treported_error = \"atomic transaction failed\";\n+\t\tgoto failure;\n+\t}\n+\tgoto cleanup;\n+\n+failure:\n+\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\tif (!cmd->error_string)\n+\t\t\tcmd->error_string = reported_error;\n+\n+cleanup:\n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n }\n \n@@ -1133,7 +1175,10 @@ static void execute_commands(struct command *commands,\n \tfree(head_name_to_free);\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n \n-\texecute_commands_non_atomic(commands, si);\n+\tif (use_atomic)\n+\t\texecute_commands_atomic(commands, si);\n+\telse\n+\t\texecute_commands_non_atomic(commands, si);\n \n \tif (shallow_update)\n \t\twarn_if_skipped_connectivity_check(commands, si);\n-- \n2.2.1.62.g3f15098\n"},{"id":"254444","messageId":"1420687404-13997-7-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 06/10] receive-pack.c: negotiate atomic push support","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:20Z","receivedAt":"2015-01-08T03:23:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nThis adds the atomic protocol option to allow\nreceive-pack to inform the client that it has\natomic push capability.\n\nThis commit makes the functionality introduced\nin the previous commits go live for the serving\nside. The changes in documentation reflect the\nprotocol capabilities of the server.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n    * no changes\n    \n    v9:\n     This was once part of \"[PATCH 1/7] receive-pack.c:\n     add protocol support to negotiate atomic-push\"\n     but now it only touches the receive-pack.c part\n     and doesn't bother with the send-pack part any more.\n     That is done in a later patch, when send-pack actually\n     learns all the things it needs to know about the\n     atomic push option.\n    \n     We can configure the remote if it wants to advertise\n     atomicity!\n    \n    All previous notes were lost due to my glorious\n    capability to operate git rebase.\n\n Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--\n builtin/receive-pack.c                            | 11 +++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 6d5424c..4f8a7bf 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -18,8 +18,9 @@ was sent.  Server MUST NOT ignore capabilities that client requested\n and server advertised.  As a consequence of these rules, server MUST\n NOT advertise capabilities it does not understand.\n \n-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities\n-are sent and recognized by the receive-pack (push to server) process.\n+The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n+capabilities are sent and recognized by the receive-pack (push to server)\n+process.\n \n The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n by both upload-pack and receive-pack protocols.  The 'agent' capability\n@@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress\n reporting if the local progress reporting is also being suppressed\n (e.g., via `push -q`, or if stderr does not go to a tty).\n \n+atomic\n+------\n+\n+If the server sends the 'atomic' capability it is capable of accepting\n+atomic pushes. If the pushing client requests this capability, the server\n+will update the refs in one atomic transaction. Either all refs are\n+updated or none.\n+\n allow-tip-sha1-in-want\n ----------------------\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 362d33f..4c069c5 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -37,6 +37,7 @@ static int receive_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n+static int advertise_atomic_push = 1;\n static int unpack_limit = 100;\n static int report_status;\n static int use_sideband;\n@@ -159,6 +160,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.advertiseatomic\") == 0) {\n+\t\tadvertise_atomic_push = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -174,6 +180,8 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \n \t\tstrbuf_addstr(&cap,\n \t\t\t      \"report-status delete-refs side-band-64k quiet\");\n+\t\tif (advertise_atomic_push)\n+\t\t\tstrbuf_addstr(&cap, \" atomic\");\n \t\tif (prefer_ofs_delta)\n \t\t\tstrbuf_addstr(&cap, \" ofs-delta\");\n \t\tif (push_cert_nonce)\n@@ -1263,6 +1271,9 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \t\t\t\tuse_sideband = LARGE_PACKET_MAX;\n \t\t\tif (parse_feature_request(feature_list, \"quiet\"))\n \t\t\t\tquiet = 1;\n+\t\t\tif (advertise_atomic_push\n+\t\t\t    && parse_feature_request(feature_list, \"atomic\"))\n+\t\t\t\tuse_atomic = 1;\n \t\t}\n \n \t\tif (!strcmp(line, \"push-cert\")) {\n-- \n2.2.1.62.g3f15098\n"},{"id":"254449","messageId":"1420687404-13997-8-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 07/10] send-pack: rename ref_update_to_be_sent to check_to_send_update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:21Z","receivedAt":"2015-01-08T03:23:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This renames ref_update_to_be_sent to check_to_send_update and inverts\nthe meaning of the return value. Having the return value inverted we\ncan have different values for the error codes. This is useful in a\nlater patch when we want to know if we hit the CHECK_REF_STATUS_REJECTED\ncase.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n    * no changes\n    \n    v7, v8, v9:\n    * no changes\n    \n    v6:\n    * negative #define'd values\n    \n    skipped v4 v5\n    \n    This was introduced with the [PATCHv2] series.\n    Changes v2 -> v3:\n    \n    * Rename to check_to_send_update\n    * Negative error values.\n    * errors values are #define'd and not raw numbers.\n\n send-pack.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 949cb61..4974825 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -190,10 +190,13 @@ static void advertise_shallow_grafts_buf(struct strbuf *sb)\n \tfor_each_commit_graft(advertise_shallow_grafts_cb, sb);\n }\n \n-static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)\n+#define CHECK_REF_NO_PUSH -1\n+#define CHECK_REF_STATUS_REJECTED -2\n+#define CHECK_REF_UPTODATE -3\n+static int check_to_send_update(const struct ref *ref, const struct send_pack_args *args)\n {\n \tif (!ref->peer_ref && !args->send_mirror)\n-\t\treturn 0;\n+\t\treturn CHECK_REF_NO_PUSH;\n \n \t/* Check for statuses set by set_ref_status_for_push() */\n \tswitch (ref->status) {\n@@ -203,10 +206,11 @@ static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_a\n \tcase REF_STATUS_REJECT_NEEDS_FORCE:\n \tcase REF_STATUS_REJECT_STALE:\n \tcase REF_STATUS_REJECT_NODELETE:\n+\t\treturn CHECK_REF_STATUS_REJECTED;\n \tcase REF_STATUS_UPTODATE:\n-\t\treturn 0;\n+\t\treturn CHECK_REF_UPTODATE;\n \tdefault:\n-\t\treturn 1;\n+\t\treturn 0;\n \t}\n }\n \n@@ -250,7 +254,7 @@ static int generate_push_cert(struct strbuf *req_buf,\n \tstrbuf_addstr(&cert, \"\\n\");\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tif (!ref_update_to_be_sent(ref, args))\n+\t\tif (check_to_send_update(ref, args) < 0)\n \t\t\tcontinue;\n \t\tupdate_seen = 1;\n \t\tstrbuf_addf(&cert, \"%s %s %s\\n\",\n@@ -359,7 +363,7 @@ int send_pack(struct send_pack_args *args,\n \t * the pack data.\n \t */\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tif (!ref_update_to_be_sent(ref, args))\n+\t\tif (check_to_send_update(ref, args) < 0)\n \t\t\tcontinue;\n \n \t\tif (!ref->deletion)\n@@ -380,7 +384,7 @@ int send_pack(struct send_pack_args *args,\n \t\tif (args->dry_run || args->push_cert)\n \t\t\tcontinue;\n \n-\t\tif (!ref_update_to_be_sent(ref, args))\n+\t\tif (check_to_send_update(ref, args) < 0)\n \t\t\tcontinue;\n \n \t\told_hex = sha1_to_hex(ref->old_sha1);\n-- \n2.2.1.62.g3f15098\n"},{"id":"254447","messageId":"1420687404-13997-9-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 08/10] send-pack.c: add --atomic command line argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:22Z","receivedAt":"2015-01-08T03:23:22Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nThis adds support to send-pack to negotiate and use atomic pushes\niff the server supports it. Atomic pushes are activated by a new command\nline flag --atomic.\n\nIn order to do this we also need to change the semantics for send_pack()\nslightly. The existing send_pack() function actually doesn't send all the\nrefs back to the server when multiple refs are involved, for example\nwhen using --all. Several of the failure modes for pushes can already be\ndetected locally in the send_pack client based on the information from the\ninitial server side list of all the refs as generated by receive-pack.\nAny such refs that we thus know would fail to push are thus pruned from\nthe list of refs we send to the server to update.\n\nFor atomic pushes, we have to deal thus with both failures that are detected\nlocally as well as failures that are reported back from the server. In order\nto do so we treat all local failures as push failures too.\n\nWe introduce a new status code REF_STATUS_ATOMIC_PUSH_FAILED so we can\nflag all refs that we would normally have tried to push to the server\nbut we did not due to local failures. This is to improve the error message\nback to the end user to flag that \"these refs failed to update since the\natomic push operation failed.\"\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n    * no changes\n    \n    v9:\n     This patch now incorporates parts of the very first patch of\n     the previous round. So this patch now implements all changes\n     in send-pack apart from the refactoring in the previous patch.\n    \n    v8:\n    \tno changes\n    v7:\n     * return error(...); instead of error(...); return -1;\n    \n    v6:\n    * realign to one error string:\n    +\terror(\"atomic push failed for ref %s. status: %d\\n\",\n    +\t      failing_ref->name, failing_ref->status);\n    \n    * Use correct value now (negative defined from previous patch)\n    \n    skipped v4 v5\n    \n    Changes v2 -> v3:\n    > We avoid assignment inside a conditional.\n    \n    Ok I switched to using a switch statement.\n    \n        Changes v1 -> v2:\n         * Now we only need a very small change in the existing code and have\n           a new static function, which cares about error reporting.\n              Junio wrote:\n              > Hmph.  Is \"atomic push\" so special that it deserves a separate\n              > parameter?  When we come up with yet another mode of failure, would\n              > we add another parameter to the callers to this function?\n         * error messages are worded differently (lower case!),\n         * use of error function instead of fprintf\n    \n         * undashed the printed error message (\"atomic push failed\");\n\n Documentation/git-send-pack.txt |  7 +++++-\n builtin/send-pack.c             |  6 ++++-\n remote.h                        |  3 ++-\n send-pack.c                     | 49 +++++++++++++++++++++++++++++++++++++++--\n send-pack.h                     |  3 ++-\n transport.c                     |  4 ++++\n 6 files changed, 66 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 2a0de42..45c7725 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another repository\n SYNOPSIS\n --------\n [verse]\n-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n+'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]\n \n DESCRIPTION\n -----------\n@@ -62,6 +62,11 @@ be in a separate packet, and the list must end with a flush packet.\n \tSend a \"thin\" pack, which records objects in deltified form based\n \ton objects not included in the pack to reduce network traffic.\n \n+--atomic::\n+\tUse an atomic transaction for updating the refs. If any of the refs\n+\tfails to update then the entire push will fail without changing any\n+\trefs.\n+\n <host>::\n \tA remote host to house the repository.  When this\n \tpart is specified, 'git-receive-pack' is invoked via\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex b564a77..b961e5a 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -13,7 +13,7 @@\n #include \"sha1-array.h\"\n \n static const char send_pack_usage[] =\n-\"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\\n\"\n+\"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]\\n\"\n \"  --all and explicit <ref> specification are mutually exclusive.\";\n \n static struct send_pack_args args;\n@@ -170,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\targs.use_thin_pack = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--atomic\")) {\n+\t\t\t\targs.atomic = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n \t\t\t\targs.stateless_rpc = 1;\n \t\t\t\tcontinue;\ndiff --git a/remote.h b/remote.h\nindex 8b62efd..f346524 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -115,7 +115,8 @@ struct ref {\n \t\tREF_STATUS_REJECT_SHALLOW,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\n-\t\tREF_STATUS_EXPECTING_REPORT\n+\t\tREF_STATUS_EXPECTING_REPORT,\n+\t\tREF_STATUS_ATOMIC_PUSH_FAILED\n \t} status;\n \tchar *remote_status;\n \tstruct ref *peer_ref; /* when renaming */\ndiff --git a/send-pack.c b/send-pack.c\nindex 4974825..e8f60df 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -282,6 +282,29 @@ free_return:\n \treturn update_seen;\n }\n \n+\n+static int atomic_push_failure(struct send_pack_args *args,\n+\t\t\t       struct ref *remote_refs,\n+\t\t\t       struct ref *failing_ref)\n+{\n+\tstruct ref *ref;\n+\t/* Mark other refs as failed */\n+\tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\tif (!ref->peer_ref && !args->send_mirror)\n+\t\t\tcontinue;\n+\n+\t\tswitch (ref->status) {\n+\t\tcase REF_STATUS_EXPECTING_REPORT:\n+\t\t\tref->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n+\t\t\tcontinue;\n+\t\tdefault:\n+\t\t\t; /* do nothing */\n+\t\t}\n+\t}\n+\treturn error(\"atomic push failed for ref %s. status: %d\\n\",\n+\t\t     failing_ref->name, failing_ref->status);\n+}\n+\n int send_pack(struct send_pack_args *args,\n \t      int fd[], struct child_process *conn,\n \t      struct ref *remote_refs,\n@@ -298,6 +321,8 @@ int send_pack(struct send_pack_args *args,\n \tint use_sideband = 0;\n \tint quiet_supported = 0;\n \tint agent_supported = 0;\n+\tint use_atomic = 0;\n+\tint atomic_supported = 0;\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n@@ -318,6 +343,8 @@ int send_pack(struct send_pack_args *args,\n \t\tagent_supported = 1;\n \tif (server_supports(\"no-thin\"))\n \t\targs->use_thin_pack = 0;\n+\tif (server_supports(\"atomic\"))\n+\t\tatomic_supported = 1;\n \tif (args->push_cert) {\n \t\tint len;\n \n@@ -332,6 +359,10 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\"Perhaps you should specify a branch such as 'master'.\\n\");\n \t\treturn 0;\n \t}\n+\tif (args->atomic && !atomic_supported)\n+\t\tdie(_(\"server does not support --atomic push\"));\n+\n+\tuse_atomic = atomic_supported && args->atomic;\n \n \tif (status_report)\n \t\tstrbuf_addstr(&cap_buf, \" report-status\");\n@@ -339,6 +370,8 @@ int send_pack(struct send_pack_args *args,\n \t\tstrbuf_addstr(&cap_buf, \" side-band-64k\");\n \tif (quiet_supported && (args->quiet || !args->progress))\n \t\tstrbuf_addstr(&cap_buf, \" quiet\");\n+\tif (use_atomic)\n+\t\tstrbuf_addstr(&cap_buf, \" atomic\");\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n \n@@ -363,9 +396,21 @@ int send_pack(struct send_pack_args *args,\n \t * the pack data.\n \t */\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tif (check_to_send_update(ref, args) < 0)\n+\t\tswitch (check_to_send_update(ref, args)) {\n+\t\tcase 0: /* no error */\n+\t\t\tbreak;\n+\t\tcase CHECK_REF_STATUS_REJECTED:\n+\t\t\t/*\n+\t\t\t * When we know the server would reject a ref update if\n+\t\t\t * we were to send it and we're trying to send the refs\n+\t\t\t * atomically, abort the whole operation.\n+\t\t\t */\n+\t\t\tif (use_atomic)\n+\t\t\t\treturn atomic_push_failure(args, remote_refs, ref);\n+\t\t\t/* Fallthrough for non atomic case. */\n+\t\tdefault:\n \t\t\tcontinue;\n-\n+\t\t}\n \t\tif (!ref->deletion)\n \t\t\tneed_pack_data = 1;\n \ndiff --git a/send-pack.h b/send-pack.h\nindex 5635457..b664648 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -13,7 +13,8 @@ struct send_pack_args {\n \t\tuse_ofs_delta:1,\n \t\tdry_run:1,\n \t\tpush_cert:1,\n-\t\tstateless_rpc:1;\n+\t\tstateless_rpc:1,\n+\t\tatomic:1;\n };\n \n int send_pack(struct send_pack_args *args,\ndiff --git a/transport.c b/transport.c\nindex 70d38e4..c67feee 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -728,6 +728,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \t\t\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n \t\t\t\t\t\t \"remote failed to report status\", porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_ATOMIC_PUSH_FAILED:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"atomic push failed\", porcelain);\n+\t\tbreak;\n \tcase REF_STATUS_OK:\n \t\tprint_ok_ref_status(ref, porcelain);\n \t\tbreak;\n-- \n2.2.1.62.g3f15098\n"},{"id":"254443","messageId":"1420687404-13997-10-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 09/10] push.c: add an --atomic argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:23Z","receivedAt":"2015-01-08T03:23:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nAdd a command line argument to the git push command to request atomic\npushes.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n    * no changes\n    \n    v8, v9:\n    no changes\n    \n    v7:\n    \tUse OPT_BOOL instead of OPT_BIT.\n    \tThis allows for --no-atomic option on the command line.\n    v6:\n    -\t\tOPT_BIT(0, \"atomic\", &flags, N_(\"use an atomic transaction remote\"),\n    +\t\tOPT_BIT(0, \"atomic\", &flags, N_(\"request atomic transaction on remote side\"),\n    \n    skipped v4 v5\n    \n    v2->v3:\n    \t* s/atomic-push/atomic/\n    \t* s/the an/an/\n    \t* no serverside, but just remote instead\n    \t* TRANSPORT_PUSH_ATOMIC instead of TRANSPORT_ATOMIC_PUSH\n    \n    Changes v1 -> v2\n    \tIt's --atomic now! (dropping the -push)\n\n Documentation/git-push.txt | 7 ++++++-\n builtin/push.c             | 5 +++++\n transport.c                | 1 +\n transport.h                | 1 +\n 4 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 21b3f29..4764fcf 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects\n SYNOPSIS\n --------\n [verse]\n-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]\n+'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]\n \t   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose]\n \t   [-u | --set-upstream] [--signed]\n \t   [--force-with-lease[=<refname>[:<expect>]]]\n@@ -136,6 +136,11 @@ already exists on the remote side.\n \tlogged.  See linkgit:git-receive-pack[1] for the details\n \ton the receiving end.\n \n+--[no-]atomic::\n+\tUse an atomic transaction on the remote side if available.\n+\tEither all refs are updated, or on error, no refs are updated.\n+\tIf the server does not support atomic pushes the push will fail.\n+\n --receive-pack=<git-receive-pack>::\n --exec=<git-receive-pack>::\n \tPath to the 'git-receive-pack' program on the remote\ndiff --git a/builtin/push.c b/builtin/push.c\nindex a076b19..8f1d945 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -487,6 +487,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tint flags = 0;\n \tint tags = 0;\n \tint rc;\n+\tint atomic = 0;\n \tconst char *repo = NULL;\t/* default repository */\n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n@@ -518,6 +519,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"follow-tags\", &flags, N_(\"push missing but relevant tags\"),\n \t\t\tTRANSPORT_PUSH_FOLLOW_TAGS),\n \t\tOPT_BIT(0, \"signed\", &flags, N_(\"GPG sign the push\"), TRANSPORT_PUSH_CERT),\n+\t\tOPT_BOOL(0, \"atomic\", &atomic, N_(\"request atomic transaction on remote side\")),\n \t\tOPT_END()\n \t};\n \n@@ -533,6 +535,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tif (tags)\n \t\tadd_refspec(\"refs/tags/*\");\n \n+\tif (atomic)\n+\t\tflags |= TRANSPORT_PUSH_ATOMIC;\n+\n \tif (argc > 0) {\n \t\trepo = argv[0];\n \t\tset_refspecs(argv + 1, argc - 1, repo);\ndiff --git a/transport.c b/transport.c\nindex c67feee..1373152 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -830,6 +830,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n \targs.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n \targs.push_cert = !!(flags & TRANSPORT_PUSH_CERT);\n+\targs.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);\n \targs.url = transport->url;\n \n \tret = send_pack(&args, data->fd, data->conn, remote_refs,\ndiff --git a/transport.h b/transport.h\nindex 3e0091e..18d2cf8 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -125,6 +125,7 @@ struct transport {\n #define TRANSPORT_PUSH_NO_HOOK 512\n #define TRANSPORT_PUSH_FOLLOW_TAGS 1024\n #define TRANSPORT_PUSH_CERT 2048\n+#define TRANSPORT_PUSH_ATOMIC 4096\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n-- \n2.2.1.62.g3f15098\n"},{"id":"254439","messageId":"1420687404-13997-11-git-send-email-sbeller@google.com","threadId":"38305","inReplyTo":"1420687404-13997-1-git-send-email-sbeller@google.com","subject":"[PATCHv12 10/10] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-08T03:23:24Z","receivedAt":"2015-01-08T03:23:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This adds tests for the atomic push option.\nThe first four tests check if the atomic option works in\ngood conditions and the last three patches check if the atomic\noption prevents any change to be pushed if just one ref cannot\nbe updated.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    v10, v11, v12:\n    * no changes\n    \n    v9:\n     add test for 'atomic push is not advertised if configured'\n     at the end.\n    \n    v7, v8: no changes\n    \n    v6:\n    the same as v2, so I can resend the whole series as v6\n    \n    v3 v4 v5 were skipped\n    \n    Changes v1 -> v2:\n    > Please drop unused comments; they are distracting.\n    \n    ok\n    \n    > It is not wrong per-se, but haven't you already tested in\n    > combination with --mirror in the previous test?\n    \n    I fixed the previous tests, so that there is no --mirror\n    and --atomic together. There is still a first --mirror push\n    for setup and a second with --atomic <branchnames> though\n    \n    > check_branches upstream master HEAD@{2} second HEAD~\n    \n    A similar function test_ref_upstream is introduced.\n    \n    > What's the value of this test?  Isn't it a non-fast-forward check\n    > you already tested in the previous one?\n    \n    I messed up there. Originally I wanted to test the 2 different\n    stages of rejection. A non-fast-forward check is done locally and\n    we don't even try pushing. But I also want to test if we locally\n    thing all is good, but the server refuses a ref to update.\n    This is now done with the last test named 'atomic push obeys\n    update hook preventing a branch to be pushed'. And that still fails.\n    \n    I'll investigate that, while still sending out the series for another\n    review though.\n    \n    * Redone the test helper, there is test_ref_upstream now.\n      This tests explicitely for SHA1 values of the ref.\n      (It's needed in the last test for example. The git push fails,\n      but still modifies the ref :/ )\n    * checked all && chains and repaired them\n    * sometimes make use of git -C <workdir>\n    \n    Notes v1:\n    Originally Ronnie had a similar patch prepared. But as I added\n    more tests and cleaned up the existing tests (e.g. use test_commit\n    instead of \"echo one >file && gitadd file && git commit -a -m 'one'\",\n    removal of dead code), the file has changed so much that I'd rather\n    take ownership.\n\n t/t5543-atomic-push.sh | 194 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 194 insertions(+)\n create mode 100755 t/t5543-atomic-push.sh\n\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nnew file mode 100755\nindex 0000000..3480b33\n--- /dev/null\n+++ b/t/t5543-atomic-push.sh\n@@ -0,0 +1,194 @@\n+#!/bin/sh\n+\n+test_description='pushing to a repository using the atomic push option'\n+\n+. ./test-lib.sh\n+\n+mk_repo_pair () {\n+\trm -rf workbench upstream &&\n+\ttest_create_repo upstream &&\n+\ttest_create_repo workbench &&\n+\t(\n+\t\tcd upstream &&\n+\t\tgit config receive.denyCurrentBranch warn\n+\t) &&\n+\t(\n+\t\tcd workbench &&\n+\t\tgit remote add up ../upstream\n+\t)\n+}\n+\n+# Compare the ref ($1) in upstream with a ref value from workbench ($2)\n+# i.e. test_refs second HEAD@{2}\n+test_refs () {\n+\ttest $# = 2 &&\n+\tgit -C upstream rev-parse --verify \"$1\" >expect &&\n+\tgit -C workbench rev-parse --verify \"$2\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'atomic push works for a single branch' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit push --atomic up master\n+\t) &&\n+\ttest_refs master master\n+'\n+\n+test_expect_success 'atomic push works for two branches' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit branch second &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit checkout second &&\n+\t\ttest_commit three &&\n+\t\tgit push --atomic up master second\n+\t) &&\n+\ttest_refs master master &&\n+\ttest_refs second second\n+'\n+\n+test_expect_success 'atomic push works in combination with --mirror' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second &&\n+\t\ttest_commit two &&\n+\t\tgit push --atomic --mirror up\n+\t) &&\n+\ttest_refs master master &&\n+\ttest_refs second second\n+'\n+\n+test_expect_success 'atomic push works in combination with --force' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit branch second master &&\n+\t\ttest_commit two_a &&\n+\t\tgit checkout second &&\n+\t\ttest_commit two_b &&\n+\t\ttest_commit three_b &&\n+\t\ttest_commit four &&\n+\t\tgit push --mirror up &&\n+\t\t# The actual test is below\n+\t\tgit checkout master &&\n+\t\ttest_commit three_a &&\n+\t\tgit checkout second &&\n+\t\tgit reset --hard HEAD^ &&\n+\t\tgit push --force --atomic up master second\n+\t) &&\n+\ttest_refs master master &&\n+\ttest_refs second second\n+'\n+\n+# set up two branches where master can be pushed but second can not\n+# (non-fast-forward). Since second can not be pushed the whole operation\n+# will fail and leave master untouched.\n+test_expect_success 'atomic push fails if one branch fails' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second master &&\n+\t\ttest_commit two &&\n+\t\ttest_commit three &&\n+\t\ttest_commit four &&\n+\t\tgit push --mirror up &&\n+\t\tgit reset --hard HEAD~2 &&\n+\t\ttest_commit five &&\n+\t\tgit checkout master &&\n+\t\ttest_commit six &&\n+\t\ttest_must_fail git push --atomic --all up\n+\t) &&\n+\ttest_refs master HEAD@{7} &&\n+\ttest_refs second HEAD@{4}\n+'\n+\n+test_expect_success 'atomic push fails if one tag fails remotely' '\n+\t# prepare the repo\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second master &&\n+\t\ttest_commit two &&\n+\t\tgit push --mirror up\n+\t) &&\n+\t# a third party modifies the server side:\n+\t(\n+\t\tcd upstream &&\n+\t\tgit checkout second &&\n+\t\tgit tag test_tag second\n+\t) &&\n+\t# see if we can now push both branches.\n+\t(\n+\t\tcd workbench &&\n+\t\tgit checkout master &&\n+\t\ttest_commit three &&\n+\t\tgit checkout second &&\n+\t\ttest_commit four &&\n+\t\tgit tag test_tag &&\n+\t\ttest_must_fail git push --tags --atomic up master second\n+\t) &&\n+\ttest_refs master HEAD@{3} &&\n+\ttest_refs second HEAD@{1}\n+'\n+\n+test_expect_success 'atomic push obeys update hook preventing a branch to be pushed' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second master &&\n+\t\ttest_commit two &&\n+\t\tgit push --mirror up\n+\t) &&\n+\t(\n+\t\tcd upstream &&\n+\t\tHOOKDIR=\"$(git rev-parse --git-dir)/hooks\" &&\n+\t\tHOOK=\"$HOOKDIR/update\" &&\n+\t\tmkdir -p \"$HOOKDIR\" &&\n+\t\twrite_script \"$HOOK\" <<-\\EOF\n+\t\t\t# only allow update to master from now on\n+\t\t\ttest \"$1\" = \"refs/heads/master\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd workbench &&\n+\t\tgit checkout master &&\n+\t\ttest_commit three &&\n+\t\tgit checkout second &&\n+\t\ttest_commit four &&\n+\t\ttest_must_fail git push --atomic up master second\n+\t) &&\n+\ttest_refs master HEAD@{3} &&\n+\ttest_refs second HEAD@{1}\n+'\n+\n+test_expect_success 'atomic push is not advertised if configured' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd upstream\n+\t\tgit config receive.advertiseatomic 0\n+\t) &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\ttest_must_fail git push --atomic up master\n+\t) &&\n+\ttest_refs master HEAD@{1}\n+'\n+\n+test_done\n-- \n2.2.1.62.g3f15098\n"},{"id":"254494","messageId":"xmqq7fwwesqs.fsf@gitster.dls.corp.google.com","threadId":"38305","inReplyTo":"1420687404-13997-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv12 06/10] receive-pack.c: negotiate atomic push support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-08T23:51:23Z","receivedAt":"2015-01-08T23:51:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> From: Ronnie Sahlberg <sahlberg@google.com>\n>\n> This adds the atomic protocol option to allow\n> receive-pack to inform the client that it has\n> atomic push capability.\n>\n> This commit makes the functionality introduced\n> in the previous commits go live for the serving\n> side. The changes in documentation reflect the\n> protocol capabilities of the server.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> Notes:\n>     v10, v11, v12:\n>     * no changes\n>     \n>     v9:\n>      This was once part of \"[PATCH 1/7] receive-pack.c:\n>      add protocol support to negotiate atomic-push\"\n>      but now it only touches the receive-pack.c part\n>      and doesn't bother with the send-pack part any more.\n>      That is done in a later patch, when send-pack actually\n>      learns all the things it needs to know about the\n>      atomic push option.\n>     \n>      We can configure the remote if it wants to advertise\n>      atomicity!\n>     \n>     All previous notes were lost due to my glorious\n>     capability to operate git rebase.\n\nThe list archive remembers if you really care ;-)\n\nI ran out of time and concentration for today to read it through at\nthis step; among things I saw, nothing looked wrong so far, and at\nthis step everything looks ready to be tested almost.\n\nLooking good.\n\n>\n>  Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--\n>  builtin/receive-pack.c                            | 11 +++++++++++\n>  2 files changed, 22 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\n> index 6d5424c..4f8a7bf 100644\n> --- a/Documentation/technical/protocol-capabilities.txt\n> +++ b/Documentation/technical/protocol-capabilities.txt\n> @@ -18,8 +18,9 @@ was sent.  Server MUST NOT ignore capabilities that client requested\n>  and server advertised.  As a consequence of these rules, server MUST\n>  NOT advertise capabilities it does not understand.\n>  \n> -The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities\n> -are sent and recognized by the receive-pack (push to server) process.\n> +The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n> +capabilities are sent and recognized by the receive-pack (push to server)\n> +process.\n>  \n>  The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n>  by both upload-pack and receive-pack protocols.  The 'agent' capability\n> @@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress\n>  reporting if the local progress reporting is also being suppressed\n>  (e.g., via `push -q`, or if stderr does not go to a tty).\n>  \n> +atomic\n> +------\n> +\n> +If the server sends the 'atomic' capability it is capable of accepting\n> +atomic pushes. If the pushing client requests this capability, the server\n> +will update the refs in one atomic transaction. Either all refs are\n> +updated or none.\n> +\n>  allow-tip-sha1-in-want\n>  ----------------------\n>  \n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 362d33f..4c069c5 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -37,6 +37,7 @@ static int receive_fsck_objects = -1;\n>  static int transfer_fsck_objects = -1;\n>  static int receive_unpack_limit = -1;\n>  static int transfer_unpack_limit = -1;\n> +static int advertise_atomic_push = 1;\n>  static int unpack_limit = 100;\n>  static int report_status;\n>  static int use_sideband;\n> @@ -159,6 +160,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n>  \t\treturn 0;\n>  \t}\n>  \n> +\tif (strcmp(var, \"receive.advertiseatomic\") == 0) {\n> +\t\tadvertise_atomic_push = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \treturn git_default_config(var, value, cb);\n>  }\n>  \n> @@ -174,6 +180,8 @@ static void show_ref(const char *path, const unsigned char *sha1)\n>  \n>  \t\tstrbuf_addstr(&cap,\n>  \t\t\t      \"report-status delete-refs side-band-64k quiet\");\n> +\t\tif (advertise_atomic_push)\n> +\t\t\tstrbuf_addstr(&cap, \" atomic\");\n>  \t\tif (prefer_ofs_delta)\n>  \t\t\tstrbuf_addstr(&cap, \" ofs-delta\");\n>  \t\tif (push_cert_nonce)\n> @@ -1263,6 +1271,9 @@ static struct command *read_head_info(struct sha1_array *shallow)\n>  \t\t\t\tuse_sideband = LARGE_PACKET_MAX;\n>  \t\t\tif (parse_feature_request(feature_list, \"quiet\"))\n>  \t\t\t\tquiet = 1;\n> +\t\t\tif (advertise_atomic_push\n> +\t\t\t    && parse_feature_request(feature_list, \"atomic\"))\n> +\t\t\t\tuse_atomic = 1;\n>  \t\t}\n>  \n>  \t\tif (!strcmp(line, \"push-cert\")) {\n"},{"id":"254564","messageId":"xmqq8uh77jct.fsf@gitster.dls.corp.google.com","threadId":"38305","inReplyTo":"1420687404-13997-9-git-send-email-sbeller@google.com","subject":"Re: [PATCHv12 08/10] send-pack.c: add --atomic command line argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-12T21:57:22Z","receivedAt":"2015-01-12T21:57:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +static int atomic_push_failure(struct send_pack_args *args,\n> +\t\t\t       struct ref *remote_refs,\n> +\t\t\t       struct ref *failing_ref)\n> +{\n> +\tstruct ref *ref;\n> +\t/* Mark other refs as failed */\n> +\tfor (ref = remote_refs; ref; ref = ref->next) {\n> +\t\tif (!ref->peer_ref && !args->send_mirror)\n> +\t\t\tcontinue;\n> +\n> +\t\tswitch (ref->status) {\n> +\t\tcase REF_STATUS_EXPECTING_REPORT:\n> +\t\t\tref->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> +\t\t\tcontinue;\n> +\t\tdefault:\n> +\t\t\t; /* do nothing */\n> +\t\t}\n> +\t}\n> +\treturn error(\"atomic push failed for ref %s. status: %d\\n\",\n> +\t\t     failing_ref->name, failing_ref->status);\n> +}\n\nGiven remote_refs, this will mark all refs that are still in the\n\"expecting\" state as \"failed\".\n\n> @@ -363,9 +396,21 @@ int send_pack(struct send_pack_args *args,\n>  \t * the pack data.\n>  \t */\n>  \tfor (ref = remote_refs; ref; ref = ref->next) {\n> -\t\tif (check_to_send_update(ref, args) < 0)\n> +\t\tswitch (check_to_send_update(ref, args)) {\n> +\t\tcase 0: /* no error */\n> +\t\t\tbreak;\n> +\t\tcase CHECK_REF_STATUS_REJECTED:\n> +\t\t\t/*\n> +\t\t\t * When we know the server would reject a ref update if\n> +\t\t\t * we were to send it and we're trying to send the refs\n> +\t\t\t * atomically, abort the whole operation.\n> +\t\t\t */\n> +\t\t\tif (use_atomic)\n> +\t\t\t\treturn atomic_push_failure(args, remote_refs, ref);\n> +\t\t\t/* Fallthrough for non atomic case. */\n> +\t\tdefault:\n>  \t\t\tcontinue;\n> -\n> +\t\t}\n\nAnd this loop stops when it sees one that would certainly get\nrejected, letting the other function mark \"expecting\" ones as \"would\nfail\".\n\nWe could have skipped refs that are OK (e.g. REF_STATUS_UPTODATE)\nbefore we saw the first rejection, but it is OK not to go back and\nmark them as \"would fail\", because the other side will not be seeing\nour attempt to update these refs anyway.\n\nOK, it makes sense to me.\n"},{"id":"254569","messageId":"CAPig+cQv6Dr7z56qXiqzZLmuBt=_TWcPSpQSTyLv0euT-k+umA@mail.gmail.com","threadId":"38305","inReplyTo":"1420687404-13997-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv12 06/10] receive-pack.c: negotiate atomic push support","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-01-12T23:29:52Z","receivedAt":"2015-01-12T23:29:52Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 7, 2015 at 10:23 PM, Stefan Beller <sbeller@google.com> wrote:\n> From: Ronnie Sahlberg <sahlberg@google.com>\n>\n> This adds the atomic protocol option to allow\n> receive-pack to inform the client that it has\n> atomic push capability.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> Notes:\n>     v9:\n>      We can configure the remote if it wants to advertise\n>      atomicity!\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 362d33f..4c069c5 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -159,6 +160,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n>                 return 0;\n>         }\n>\n> +       if (strcmp(var, \"receive.advertiseatomic\") == 0) {\n> +               advertise_atomic_push = git_config_bool(var, value);\n> +               return 0;\n> +       }\n\nIs this needed only to support the tests you add in t5543, or do you\nintend it to be useful to end users?\n\nIf it is for end users, then it deserves to be documented. Is also\ndeserves proper mention and justification in the commit message.\n\nIf it's only intended to assist automated testing, then perhaps\ncontrol it via an environment variable rather than a configuration\noption. (See, for instance, GIT_TEST_SPLIT_INDEX or GIT_USE_LOOKUP as\nprecedent.)\n\n> +\n>         return git_default_config(var, value, cb);\n>  }\n>\n> @@ -174,6 +180,8 @@ static void show_ref(const char *path, const unsigned char *sha1)\n>\n>                 strbuf_addstr(&cap,\n>                               \"report-status delete-refs side-band-64k quiet\");\n> +               if (advertise_atomic_push)\n> +                       strbuf_addstr(&cap, \" atomic\");\n>                 if (prefer_ofs_delta)\n>                         strbuf_addstr(&cap, \" ofs-delta\");\n>                 if (push_cert_nonce)\n> @@ -1263,6 +1271,9 @@ static struct command *read_head_info(struct sha1_array *shallow)\n>                                 use_sideband = LARGE_PACKET_MAX;\n>                         if (parse_feature_request(feature_list, \"quiet\"))\n>                                 quiet = 1;\n> +                       if (advertise_atomic_push\n> +                           && parse_feature_request(feature_list, \"atomic\"))\n> +                               use_atomic = 1;\n>                 }\n>\n>                 if (!strcmp(line, \"push-cert\")) {\n> --\n> 2.2.1.62.g3f15098\n"},{"id":"254570","messageId":"CAPig+cT4SvvO=KCuRNaEZ7Vn1AeBy19mMeKtC=XJ1Mqw+zYYzA@mail.gmail.com","threadId":"38305","inReplyTo":"1420687404-13997-11-git-send-email-sbeller@google.com","subject":"Re: [PATCHv12 10/10] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-01-12T23:40:28Z","receivedAt":"2015-01-12T23:40:28Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 7, 2015 at 10:23 PM, Stefan Beller <sbeller@google.com> wrote:\n> This adds tests for the atomic push option.\n> The first four tests check if the atomic option works in\n> good conditions and the last three patches check if the atomic\n\ns/patches/tests/\n\n> option prevents any change to be pushed if just one ref cannot\n> be updated.\n\nThe commit message talks about 7 tests, but I count 8.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\n> new file mode 100755\n> index 0000000..3480b33\n> --- /dev/null\n> +++ b/t/t5543-atomic-push.sh\n> @@ -0,0 +1,194 @@\n> +test_expect_success 'atomic push is not advertised if configured' '\n> +       mk_repo_pair &&\n> +       (\n> +               cd upstream\n\nBroken &&-chain.\n\n> +               git config receive.advertiseatomic 0\n> +       ) &&\n> +       (\n> +               cd workbench &&\n> +               test_commit one &&\n> +               git push --mirror up &&\n> +               test_commit two &&\n> +               test_must_fail git push --atomic up master\n> +       ) &&\n> +       test_refs master HEAD@{1}\n> +'\n> +\n> +test_done\n> --\n> 2.2.1.62.g3f15098\n"},{"id":"254571","messageId":"CAGZ79kaxFqtxBE+LaWbh92b8RK-qganuADjsnwHU5WO-oJebcw@mail.gmail.com","threadId":"38305","inReplyTo":"CAPig+cQv6Dr7z56qXiqzZLmuBt=_TWcPSpQSTyLv0euT-k+umA@mail.gmail.com","subject":"Re: [PATCHv12 06/10] receive-pack.c: negotiate atomic push support","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-12T23:43:59Z","receivedAt":"2015-01-12T23:43:59Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jan 12, 2015 at 3:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n> Is this needed only to support the tests you add in t5543, or do you\n> intend it to be useful to end users?\n>\n> If it is for end users, then it deserves to be documented. Is also\n> deserves proper mention and justification in the commit message.\n>\n> If it's only intended to assist automated testing, then perhaps\n> control it via an environment variable rather than a configuration\n> option. (See, for instance, GIT_TEST_SPLIT_INDEX or GIT_USE_LOOKUP as\n> precedent.)\n\nI added that after reading\nhttps://www.marc.info/?l=git&m=141995803919542&w=2\nwhich makes me believe we actually want to expose this to the enduser.\n\nDocumentation will follow soon.\n"},{"id":"254575","messageId":"xmqq4mrv5yp8.fsf@gitster.dls.corp.google.com","threadId":"38305","inReplyTo":"CAPig+cQv6Dr7z56qXiqzZLmuBt=_TWcPSpQSTyLv0euT-k+umA@mail.gmail.com","subject":"Re: [PATCHv12 06/10] receive-pack.c: negotiate atomic push support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-13T00:08:51Z","receivedAt":"2015-01-13T00:08:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> If it's only intended to assist automated testing, then perhaps\n> control it via an environment variable rather than a configuration\n> option. (See, for instance, GIT_TEST_SPLIT_INDEX or GIT_USE_LOOKUP as\n> precedent.)\n\nNote that passing environment to receive-pack is very cumbersome in\ngeneral, unless you know you will only use local transport via forked\nprocess pair.\n\nBy the way, is USE_LOOKUP still viable these days?  I think it was\nadded long time ago as a knob to allow us to experiment, so that we\ncan decide which codepath should survive, but if nobody has done any\nmeaningful benchmarking since it was added, perhaps we do not need\nto keep the conditionally used codepath.  We, who are too lazy to\neven try it out, do not deserve its benefit even if the experimental\ncodepath turns out to be vastly faster.\n"}]}