{"thread":{"id":"38210","subject":"[PATCHv6 0/7] atomic pushes","startedAt":"2014-12-19T19:38:54Z","lastAt":"2014-12-30T16:47:05Z","messageCount":20,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"253863","messageId":"1419017941-7090-1-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":null,"subject":"[PATCHv6 0/7] atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:38:54Z","receivedAt":"2014-12-19T19:38:54Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch series adds a flag to git push to update the remote refs atomically.\n\nThis series applies on top of origin/mh/reflog-expire\nIt can also be found at github[1].\n\nThis series incorporates all suggestions by Eric. Most changes appear in\npatch 4/7 as it is a complete rewrite compared to v5 of this patch series.\n5/7 is new to the series and cleans up the rewrite by moving the function\nexecute_command around.\n\nAny comments are welcome!\n\nThanks,\nStefan\n\n[1] https://github.com/stefanbeller/git/tree/atomic-push-v6\n\nRonnie Sahlberg (3):\n receive-pack.c: add protocol support to negotiate atomic-push\n send-pack.c: add --atomic command line argument\n push.c: add an --atomic argument\n\nStefan Beller (4):\n send-pack: Rename ref_update_to_be_sent to check_to_send_update\n receive-pack.c: receive-pack.c: use a single ref_transaction for\n atomic pushes\n receive-pack: move execute_commands_non_atomic before execute_commands\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 | 2 +\n builtin/receive-pack.c | 165 +++++++++++++++-----\n builtin/send-pack.c | 6 +-\n remote.h | 3 +-\n send-pack.c | 66 +++++++-\n send-pack.h | 3 +-\n t/t5543-atomic-push.sh | 178 ++++++++++++++++++++++\n transport.c | 5 +\n transport.h | 1 +\n 12 files changed, 405 insertions(+), 51 deletions(-)\n create mode 100755 t/t5543-atomic-push.sh\n\n-- \n2.2.1.62.g3f15098\n"},{"id":"253864","messageId":"1419017941-7090-2-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:38:55Z","receivedAt":"2014-12-19T19:38:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nThis adds support to the protocol between send-pack and receive-pack to\n* allow receive-pack to inform the client that it has atomic push capability\n* allow send-pack to request atomic push back.\n\nThere is currently no setting in send-pack to actually request that atomic\npushes are to be used yet. This only adds protocol capability not ability\nfor the user to activate it.\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nNotes:\n Changes v1 -> v2:\n \t* Name it atomic instead of atomic-push. The name changes affects\n \t the flags send over the wire as well as the flags in\n \t struct send_pack_args\n \t* Add check, which was part of the later patch here:\n \tif (args->atomic && !atomic_supported) {\n \t\tfprintf(stderr, \"Server does not support atomic push.\");\n \t\treturn -1;\n \t}\n \n v2 -> v3:\n More concise error reporting as suggested by Junio\n -\t\tfprintf(stderr, \"Server does not support atomic push.\");\n -\t\treturn -1;\n +\t\treturn error(\"server does not support atomic push.\");\n \n skipped v4 v5\n \n v6:\n \t* s/one single atomic transaction./one atomic transaction./\n \t* die(_( instead of return error(\n\n Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--\n builtin/receive-pack.c | 6 +++++-\n send-pack.c | 10 ++++++++++\n send-pack.h | 3 ++-\n 4 files changed, 28 insertions(+), 4 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 32fc540..e76e5d5 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@@ -171,7 +172,8 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t\tstruct strbuf cap = STRBUF_INIT;\n \n \t\tstrbuf_addstr(&cap,\n-\t\t\t \"report-status delete-refs side-band-64k quiet\");\n+\t\t\t \"report-status delete-refs side-band-64k quiet \"\n+\t\t\t \"atomic\");\n \t\tif (prefer_ofs_delta)\n \t\t\tstrbuf_addstr(&cap, \" ofs-delta\");\n \t\tif (push_cert_nonce)\n@@ -1179,6 +1181,8 @@ 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 (parse_feature_request(feature_list, \"atomic\"))\n+\t\t\t\tuse_atomic = 1;\n \t\t}\n \n \t\tif (!strcmp(line, \"push-cert\")) {\ndiff --git a/send-pack.c b/send-pack.c\nindex 949cb61..6646600 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -294,6 +294,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;\n+\tint atomic_supported = 0;\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n@@ -314,6 +316,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@@ -328,6 +332,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@@ -335,6 +343,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 \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,\n-- \n2.2.1.62.g3f15098\n"},{"id":"253865","messageId":"1419017941-7090-3-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/7] send-pack: Rename ref_update_to_be_sent to check_to_send_update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:38:56Z","receivedAt":"2014-12-19T19:38:56Z","isPatch":true,"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 \n---\n\nNotes:\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 skipped v4 v5\n \n v6:\n * negative #define'd values\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 6646600..465e6fb 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@@ -369,7 +373,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@@ -390,7 +394,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":"253869","messageId":"1419017941-7090-4-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/7] send-pack.c: add --atomic command line argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:38:57Z","receivedAt":"2014-12-19T19:38:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \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 \nSigned-off-by: Stefan Beller \n---\n\nNotes:\n Notes:\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 Changes v2 -> v3:\n > We avoid assignment inside a conditional.\n \n Ok I switched to using a switch statement.\n \n skipped v4 v5\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 Documentation/git-send-pack.txt | 7 ++++++-\n builtin/send-pack.c | 6 +++++-\n remote.h | 3 ++-\n send-pack.c | 40 ++++++++++++++++++++++++++++++++++++++--\n transport.c | 4 ++++\n 5 files changed, 55 insertions(+), 5 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=] [--verbose] [--thin] [:] [...]\n+'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=] [--verbose] [--thin] [--atomic] [:] [...]\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 ::\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=] [--verbose] [--thin] [:] [...]\\n\"\n+\"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=] [--verbose] [--thin] [--atomic] [:] [...]\\n\"\n \" --all and explicit 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 465e6fb..ba63231 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -282,6 +282,30 @@ 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+\terror(\"atomic push failed for ref %s. status: %d\\n\",\n+\t failing_ref->name, failing_ref->status);\n+\treturn -1;\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@@ -373,9 +397,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/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":"253867","messageId":"1419017941-7090-5-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:38:58Z","receivedAt":"2014-12-19T19:38:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Update receive-pack to use an atomic transaction iff the client negotiated\nthat it wanted atomic-push. This leaves the default behavior to be the old\nnon-atomic one ref at a time update. This is to cause as little disruption\nas possible to existing clients. It is unknown if there are client scripts\nthat depend on the old non-atomic behavior so we make it opt-in for now.\n\nIf it turns out over time that there are no client scripts that depend on the\nold behavior we can change git to default to use atomic pushes and instead\noffer an opt-out argument for people that do not want atomic pushes.\n\nInspired-by: Ronnie Sahlberg \nHelped-by: Eric Sunshine \nSigned-off-by: Stefan Beller \n---\n\nNotes:\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 | 137 +++++++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 115 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e76e5d5..710cd7f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -67,6 +67,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@@ -823,6 +824,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@@ -832,35 +834,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@@ -1044,11 +1047,72 @@ static void reject_updates_to_hidden(struct command *commands)\n \t}\n }\n \n+static void execute_commands_non_atomic(struct command *commands,\n+\t\t\t\t struct shallow_info *si);\n+\n+\n+static void execute_commands_atomic(struct command *commands,\n+\t\t\t\t struct shallow_info *si)\n+{\n+\tstruct command *cmd;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tconst char *reported_error = \"atomic push failure\";\n+\tint checked_connectivity = 1;\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction) {\n+\t\trp_error(\"%s\", err.buf);\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 (cmd->error_string)\n+\t\t\tgoto failure;\n+\n+\t\tif (cmd->skip_update)\n+\t\t\tgoto failure;\n+\n+\t\tcmd->error_string = update(cmd, si);\n+\n+\t\tif (cmd->error_string)\n+\t\t\tgoto failure;\n+\n+\t\tif (shallow_update && 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\treported_error = \"transaction failed due to internal bug\";\n+\t\t\tgoto failure;\n+\t\t}\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+\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\n+\treturn;\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+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\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+}\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@@ -1079,7 +1143,20 @@ 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+\tif (use_atomic) {\n+\t\texecute_commands_atomic(commands, si);\n+\t} else {\n+\t\texecute_commands_non_atomic(commands, si);\n+\t}\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+\tstruct strbuf err = STRBUF_INIT;\n+\tint checked_connectivity = 1;\n+\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (cmd->error_string)\n \t\t\tcontinue;\n@@ -1087,19 +1164,35 @@ static void execute_commands(struct command *commands,\n \t\tif (cmd->skip_update)\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-\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+\n+\t\tif (!cmd->error_string) {\n+\t\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\tstrbuf_reset(&err);\n+\t\t\t\tcmd->error_string = \"failed to update ref\";\n+\t\t\t}\n+\t\t\tif (shallow_update && si->shallow_ref[cmd->index]) {\n+\t\t\t\terror(\"BUG: connectivity check has not been run on ref %s\",\n+\t\t\t\t cmd->ref_name);\n+\t\t\t\tchecked_connectivity = 0;\n+\t\t\t}\n \t\t}\n+\t\tref_transaction_free(transaction);\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+\tstrbuf_release(&err);\n }\n \n static struct command **queue_command(struct command **tail,\n-- \n2.2.1.62.g3f15098\n"},{"id":"253866","messageId":"1419017941-7090-6-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/7] receive-pack: move execute_commands_non_atomic before execute_commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:38:59Z","receivedAt":"2014-12-19T19:38:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This way we don't need to declare the function first and implement it\nlater, but rather we put the implementation directly at the place where\nthe function was declared.\n\nThe reason I did not move it up in the first place is better readability\nof the diff as the execute_commands_non_atomic function is taking lots of\ncode that was once in execute_commands.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n This patch is new with v6 of the series\n \n As execute_commands_non_atomic is larger than execute_commands, the diff\n is not moving around execute_commands_non_atomic, but execute_commands.\n These two functions were one after the other and switch places that way.\n\n builtin/receive-pack.c | 86 ++++++++++++++++++++++++--------------------------\n 1 file changed, 41 insertions(+), 45 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 710cd7f..d47e73b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1047,10 +1047,6 @@ static void reject_updates_to_hidden(struct command *commands)\n \t}\n }\n \n-static void execute_commands_non_atomic(struct command *commands,\n-\t\t\t\t struct shallow_info *si);\n-\n-\n static void execute_commands_atomic(struct command *commands,\n \t\t\t\t struct shallow_info *si)\n {\n@@ -1109,47 +1105,6 @@ failure:\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-\tstruct command *cmd;\n-\tunsigned char sha1[20];\n-\tstruct iterate_data data;\n-\n-\tif (unpacker_error) {\n-\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n-\t\t\tcmd->error_string = \"unpacker error\";\n-\t\treturn;\n-\t}\n-\n-\tdata.cmds = commands;\n-\tdata.si = si;\n-\tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n-\t\tset_connectivity_errors(commands, si);\n-\n-\treject_updates_to_hidden(commands);\n-\n-\tif (run_receive_hook(commands, \"pre-receive\", 0)) {\n-\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\t\tif (!cmd->error_string)\n-\t\t\t\tcmd->error_string = \"pre-receive hook declined\";\n-\t\t}\n-\t\treturn;\n-\t}\n-\n-\tcheck_aliased_updates(commands);\n-\n-\tfree(head_name_to_free);\n-\thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n-\n-\tif (use_atomic) {\n-\t\texecute_commands_atomic(commands, si);\n-\t} else {\n-\t\texecute_commands_non_atomic(commands, si);\n-\t}\n-}\n-\n static void execute_commands_non_atomic(struct command *commands,\n \t\t\t\t\tstruct shallow_info *si)\n {\n@@ -1195,6 +1150,47 @@ static void execute_commands_non_atomic(struct command *commands,\n \tstrbuf_release(&err);\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+\tstruct command *cmd;\n+\tunsigned char sha1[20];\n+\tstruct iterate_data data;\n+\n+\tif (unpacker_error) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\tcmd->error_string = \"unpacker error\";\n+\t\treturn;\n+\t}\n+\n+\tdata.cmds = commands;\n+\tdata.si = si;\n+\tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n+\t\tset_connectivity_errors(commands, si);\n+\n+\treject_updates_to_hidden(commands);\n+\n+\tif (run_receive_hook(commands, \"pre-receive\", 0)) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (!cmd->error_string)\n+\t\t\t\tcmd->error_string = \"pre-receive hook declined\";\n+\t\t}\n+\t\treturn;\n+\t}\n+\n+\tcheck_aliased_updates(commands);\n+\n+\tfree(head_name_to_free);\n+\thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n+\n+\tif (use_atomic) {\n+\t\texecute_commands_atomic(commands, si);\n+\t} else {\n+\t\texecute_commands_non_atomic(commands, si);\n+\t}\n+}\n+\n static struct command **queue_command(struct command **tail,\n \t\t\t\t const char *line,\n \t\t\t\t int linelen)\n-- \n2.2.1.62.g3f15098\n"},{"id":"253870","messageId":"1419017941-7090-7-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/7] push.c: add an --atomic argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:39:00Z","receivedAt":"2014-12-19T19:39:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nAdd a command line argument to the git push command to request atomic\npushes.\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nNotes:\n Changes v1 -> v2\n \tIt's --atomic now! (dropping the -push)\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 skipped v4 v5\n \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 Documentation/git-push.txt | 7 ++++++-\n builtin/push.c | 2 ++\n transport.c | 1 +\n transport.h | 1 +\n 4 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 21b3f29..da63bdf 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=]\n+'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=]\n \t [--repo=] [-f | --force] [--prune] [-v | --verbose]\n \t [-u | --set-upstream] [--signed]\n \t [--force-with-lease[=[:]]]\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+--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=::\n --exec=::\n \tPath to the 'git-receive-pack' program on the remote\ndiff --git a/builtin/push.c b/builtin/push.c\nindex a076b19..1689cec 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -518,6 +518,8 @@ 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_BIT(0, \"atomic\", &flags, N_(\"request atomic transaction on remote side\"),\n+\t\t\tTRANSPORT_PUSH_ATOMIC),\n \t\tOPT_END()\n \t};\n \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":"253868","messageId":"1419017941-7090-8-git-send-email-sbeller@google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"[PATCH 7/7] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T19:39:01Z","receivedAt":"2014-12-19T19:39:01Z","isPatch":true,"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 \n---\n\nNotes:\n v6: 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 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 \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 | 178 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 178 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..b81a542\n--- /dev/null\n+++ b/t/t5543-atomic-push.sh\n@@ -0,0 +1,178 @@\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_done\n-- \n2.2.1.62.g3f15098\n"},{"id":"253937","messageId":"xmqqfvc7a6o3.fsf@gitster.dls.corp.google.com","threadId":"38210","inReplyTo":"1419017941-7090-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/7] receive-pack: move execute_commands_non_atomic before execute_commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-22T18:19:40Z","receivedAt":"2014-12-22T18:19:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Notes:\n> This patch is new with v6 of the series\n> \n> As execute_commands_non_atomic is larger than execute_commands, the diff\n> is not moving around execute_commands_non_atomic, but execute_commands.\n\n;-)\n\nNext time perhaps try \"--patience\" to decide between with and\nwithout which one reads better?\n"},{"id":"253938","messageId":"xmqqbnmva6fu.fsf@gitster.dls.corp.google.com","threadId":"38210","inReplyTo":"1419017941-7090-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv6 0/7] atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-22T18:24:37Z","receivedAt":"2014-12-22T18:24:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Will queue; thanks for a pleasant read.\n"},{"id":"253967","messageId":"CAPig+cRStBZMNj_K-81n2aZ3SzvH8VSeXwwk0MM5Q01FANRdPg@mail.gmail.com","threadId":"38210","inReplyTo":"1419017941-7090-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-22T22:52:48Z","receivedAt":"2014-12-22T22:52:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n>\n> This adds support to the protocol between send-pack and receive-pack to\n> * allow receive-pack to inform the client that it has atomic push capability\n> * allow send-pack to request atomic push back.\n>\n> There is currently no setting in send-pack to actually request that atomic\n> pushes are to be used yet. This only adds protocol capability not ability\n> for the user to activate it.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \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> @@ -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\nNot itself worth a re-send, but if you re-send for some other reason...\n\n\"one atomic\" still smacks of redundancy; \"an atomic\" sounds better.\n\n> +updated or none.\n> +\n> allow-tip-sha1-in-want\n> ----------------------\n"},{"id":"253970","messageId":"CAPig+cQCLWy8rEqMCMxkvz+jAYwOA2tZE1LagkB5vY-60pe1mQ@mail.gmail.com","threadId":"38210","inReplyTo":"1419017941-7090-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/7] send-pack.c: add --atomic command line argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-22T22:58:18Z","receivedAt":"2014-12-22T22:58:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n>\n> This adds support to send-pack to negotiate and use atomic pushes\n> iff the server supports it. Atomic pushes are activated by a new command\n> line flag --atomic.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index b564a77..b961e5a 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -282,6 +282,30 @@ free_return:\n> return update_seen;\n> }\n>\n> +\n> +static int atomic_push_failure(struct send_pack_args *args,\n> + struct ref *remote_refs,\n> + struct ref *failing_ref)\n> +{\n> + struct ref *ref;\n> + /* Mark other refs as failed */\n> + for (ref = remote_refs; ref; ref = ref->next) {\n> + if (!ref->peer_ref && !args->send_mirror)\n> + continue;\n> +\n> + switch (ref->status) {\n> + case REF_STATUS_EXPECTING_REPORT:\n> + ref->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> + continue;\n> + default:\n> + ; /* do nothing */\n> + }\n> + }\n> + error(\"atomic push failed for ref %s. status: %d\\n\",\n> + failing_ref->name, failing_ref->status);\n> + return -1;\n\nNot itself worth a re-send, but if you do re-send for some other reason...\n\n return error(...);\n\nwould be more idiomatic (as mentioned in the previous review).\n\n> +}\n> +\n> int send_pack(struct send_pack_args *args,\n> int fd[], struct child_process *conn,\n> struct ref *remote_refs,\n"},{"id":"253981","messageId":"CAPig+cTjwiDPdQv-i3VBeQEJDTiPGaB4G-WA141qNA9fCyDKLw@mail.gmail.com","threadId":"38210","inReplyTo":"1419017941-7090-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-23T01:31:28Z","receivedAt":"2014-12-23T01:31:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote:\n> Update receive-pack to use an atomic transaction iff the client negotiated\n> that it wanted atomic-push. This leaves the default behavior to be the old\n> non-atomic one ref at a time update. This is to cause as little disruption\n> as possible to existing clients. It is unknown if there are client scripts\n> that depend on the old non-atomic behavior so we make it opt-in for now.\n>\n> If it turns out over time that there are no client scripts that depend on the\n> old behavior we can change git to default to use atomic pushes and instead\n> offer an opt-out argument for people that do not want atomic pushes.\n\nNotes and observations below. None of them are particularly\nactionable. If Junio is happy with the current round, and if you don't\nhave some other reason to re-roll, then consider them food for thought\nfor future patches.\n\n> Inspired-by: Ronnie Sahlberg \n> Helped-by: Eric Sunshine \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index e76e5d5..710cd7f 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1044,11 +1047,72 @@ static void reject_updates_to_hidden(struct command *commands)\n> }\n> }\n>\n> +static void execute_commands_non_atomic(struct command *commands,\n> + struct shallow_info *si);\n> +\n> +\n> +static void execute_commands_atomic(struct command *commands,\n> + struct shallow_info *si)\n> +{\n> + struct command *cmd;\n> + struct strbuf err = STRBUF_INIT;\n> + const char *reported_error = \"atomic push failure\";\n> + int checked_connectivity = 1;\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + reported_error = \"transaction failed to start\";\n> + goto failure;\n> + }\n> +\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (cmd->error_string)\n> + goto failure;\n> +\n> + if (cmd->skip_update)\n> + goto failure;\n\nThese checks are common to the atomic and non-atomic cases. To reduce\ncode duplication between the two cases, you could factor them out to a\ncmd_okay() helper function (or some such name) which checks both\nconditions.\n\n> + cmd->error_string = update(cmd, si);\n> +\n> + if (cmd->error_string)\n> + goto failure;\n> +\n> + if (shallow_update && si->shallow_ref[cmd->index]) {\n> + error(\"BUG: connectivity check has not been run on ref %s\",\n> + cmd->ref_name);\n> + checked_connectivity = 0;\n> + reported_error = \"transaction failed due to internal bug\";\n> + goto failure;\n\nThis code is also common to atomic and non-atomic cases and could be\nfactored out to a helper function, thus further reducing duplication.\nThe less code duplication between cases, the lower the cognitive load,\nmaking the code easier to understand.\n\n> + }\n> + }\n> +\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + reported_error = \"atomic transaction failed\";\n> + goto failure;\n> + }\n> +\n> + ref_transaction_free(transaction);\n> + strbuf_release(&err);\n> + return;\n> +\n> +failure:\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + if (!cmd->error_string)\n> + cmd->error_string = reported_error;\n> + ref_transaction_free(transaction);\n> + strbuf_release(&err);\n> +\n> + if (shallow_update && !checked_connectivity)\n> + error(\"BUG: run 'git fsck' for safety.\\n\"\n> + \"If there are errors, try to remove \"\n> + \"the reported refs above\");\n\nThis final conditional is common to both cases but does not even need\nto be factored out to a helper function. It could/should have remained\nin execute_commands() at its original position, following the call to\nexecute_commands_atomic() or execute_commands_non_atomic().\n\n> +}\n> +\n> static void execute_commands(struct command *commands,\n> const char *unpacker_error,\n> struct shallow_info *si)\n> {\n> - int checked_connectivity;\n> struct command *cmd;\n> unsigned char sha1[20];\n> struct iterate_data data;\n> @@ -1079,7 +1143,20 @@ static void execute_commands(struct command *commands,\n> free(head_name_to_free);\n> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>\n> - checked_connectivity = 1;\n> + if (use_atomic) {\n> + execute_commands_atomic(commands, si);\n> + } else {\n> + execute_commands_non_atomic(commands, si);\n> + }\n\nStyle: Unnecessary braces.\n\nMore below.\n\n> +}\n> +\n> +static void execute_commands_non_atomic(struct command *commands,\n> + struct shallow_info *si)\n> +{\n> + struct command *cmd;\n> + struct strbuf err = STRBUF_INIT;\n> + int checked_connectivity = 1;\n> +\n> for (cmd = commands; cmd; cmd = cmd->next) {\n> if (cmd->error_string)\n> continue;\n> @@ -1087,19 +1164,35 @@ static void execute_commands(struct command *commands,\n> if (cmd->skip_update)\n> continue;\n>\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_reset(&err);\n> + cmd->error_string = \"transaction failed to start\";\n> + continue;\n> + }\n> +\n> cmd->error_string = update(cmd, si);\n> - if (shallow_update && !cmd->error_string &&\n> - si->shallow_ref[cmd->index]) {\n> - error(\"BUG: connectivity check has not been run on ref %s\",\n> - cmd->ref_name);\n> - checked_connectivity = 0;\n> +\n> + if (!cmd->error_string) {\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_reset(&err);\n> + cmd->error_string = \"failed to update ref\";\n> + }\n> + if (shallow_update && si->shallow_ref[cmd->index]) {\n> + error(\"BUG: connectivity check has not been run on ref %s\",\n> + cmd->ref_name);\n> + checked_connectivity = 0;\n> + }\n\nThere is a behavior change here. In the original, the\n'si->shallow_ref[cmd->index]' check was performed only if no other\nerror occurred, however, in the updated code, the check is performed\neven when ref_transaction_commit() fails. Is this correct behavior?\n\n> }\n> + ref_transaction_free(transaction);\n> }\n> -\n> if (shallow_update && !checked_connectivity)\n> error(\"BUG: run 'git fsck' for safety.\\n\"\n> \"If there are errors, try to remove \"\n> \"the reported refs above\");\n> + strbuf_release(&err);\n> }\n>\n> static struct command **queue_command(struct command **tail,\n> --\n\nA few comments about the refactoring...\n\nIt would have been easier to follow and understand the changes had\nthey been done in a stepwise fashion over a series of patches,\nresulting in a less confusing diff (and avoiding the need for the\nfollow-up patch where you merely relocate the modified code).\n\nOne possible approach would have been something like this:\n\npatch 1: Factor out code into helper functions which will be needed by\nthe 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\npatch 2: Factor out the main 'for' loop of execute_commands() into a\nnew function. This new function will eventually become\nexecute_commands_non_atomic(). At this point, execute_commands() is\npretty much in its final form with the exception of the upcoming 'if\n(use_atomic)' conditional.\n\npatch 3: Morph the function extracted in patch 2 into\nexecute_commands_non_atomic() by adding transaction handling inside\nthe 'for' loop (and applying the changes from the early part of the\npatch which go along with that).\n\npatch 4: Add execute_commands_atomic(), and the corresponding 'if\n(use_atomic)' conditional to execute_commands().\n\nOther approaches are possible, but the motivation behind the above\norganization is that reviewer time is precious, and that it's\nbeneficial to present changes in an easy-to-digest form.\n"},{"id":"253984","messageId":"5498CEEE.9010105@gmail.com","threadId":"38210","inReplyTo":"CAPig+cRStBZMNj_K-81n2aZ3SzvH8VSeXwwk0MM5Q01FANRdPg@mail.gmail.com","subject":"Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-12-23T02:09:50Z","receivedAt":"2014-12-23T02:09:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 22.12.2014 14:52, Eric Sunshine wrote:\n> On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote:\n>> From: Ronnie Sahlberg \n>>\n>> This adds support to the protocol between send-pack and receive-pack to\n>> * allow receive-pack to inform the client that it has atomic push capability\n>> * allow send-pack to request atomic push back.\n>>\n>> There is currently no setting in send-pack to actually request that atomic\n>> pushes are to be used yet. This only adds protocol capability not ability\n>> for the user to activate it.\n>>\n>> Signed-off-by: Ronnie Sahlberg \n>> Signed-off-by: Stefan Beller \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>> @@ -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> \n> Not itself worth a re-send, but if you re-send for some other reason...\n> \n> \"one atomic\" still smacks of redundancy; \"an atomic\" sounds better.\n\nI did hear you saying 'one single atomic' being too redundant. And I\nagree that 'one' and 'single' makes the redundancy.\n\nHowever I have the impression 'an atomic' is too weak. Not everybody is\na careful reader picking up the subtle notions. Not everybody is english\nnative. Or concentrated.\n\nLook at it the other way: How could it be done?\n\n* All of the refs could be updated one at a time, not atomically, so\n\tforeach ref:\n\t\topen refs/heads/bla:\n\t\t\twrite new sha1\n\n* All of the refs could be updated at once, not atomically:\n\topen refs pack file:\n\t\twrite new content\n* All of the refs could be updated, one at a time, atomically:\n\tforeach ref:\n\t\tget lock\n\t\twrite content to lock\n\t\trename into place\n* All of the refs at once, atomically.\n\topen packed refs file lock:\n\twrite new content\n\trename into place\n\nThat said, atomicity and how many transactions there are, are orthogonal\nto each other. That's why I'd keep pointing out 'one atomic'\ntransaction.\n\nThanks for all the comments. I may be doing cleanup patches for you on\ntop of what Junio queued.\n\n\t\n\n\n\n\n\n> \n>> +updated or none.\n>> +\n>> allow-tip-sha1-in-want\n>> ----------------------\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"254060","messageId":"CAGZ79ka0hAko54g4rG27Ef=YFAXd-x50bPr0NSHRwPtDEobzHQ@mail.gmail.com","threadId":"38210","inReplyTo":"xmqqfvc7a6o3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/7] receive-pack: move execute_commands_non_atomic before execute_commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-24T00:30:44Z","receivedAt":"2014-12-24T00:30:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I tried all four diff options as listed in the man page of\nformat-diff. I forget which one I used, but there was no large\ndifference w.r.t. reviewability if I remember correctly.\n\n\nOn Mon, Dec 22, 2014 at 10:19 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Notes:\n>> This patch is new with v6 of the series\n>>\n>> As execute_commands_non_atomic is larger than execute_commands, the diff\n>> is not moving around execute_commands_non_atomic, but execute_commands.\n>\n> ;-)\n>\n> Next time perhaps try \"--patience\" to decide between with and\n> without which one reads better?\n"},{"id":"254068","messageId":"549A6C4C.8000905@alum.mit.edu","threadId":"38210","inReplyTo":"1419017941-7090-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-24T07:33:32Z","receivedAt":"2014-12-24T07:33:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/19/2014 08:38 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n> \n> This adds support to the protocol between send-pack and receive-pack to\n> * allow receive-pack to inform the client that it has atomic push capability\n> * allow send-pack to request atomic push back.\n> \n> There is currently no setting in send-pack to actually request that atomic\n> pushes are to be used yet. This only adds protocol capability not ability\n> for the user to activate it.\n\nSorry to jump in so late...\n\nIf I understand correctly, after this patch the server advertises the\n\"atomic\" capability even though it doesn't actually have that ability\nuntil a later patch. It seems to me that the order of the patches should\nbe reversed: don't advertise the capability before it is actually\nimplemented. Why? Bisection. Between the two patches the server is buggy.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"254101","messageId":"549D0B78.402@alum.mit.edu","threadId":"38210","inReplyTo":"1419017941-7090-7-git-send-email-sbeller@google.com","subject":"Re: [PATCH 6/7] push.c: add an --atomic argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-26T07:17:12Z","receivedAt":"2014-12-26T07:17:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/19/2014 08:39 PM, Stefan Beller wrote:\n> Add a command line argument to the git push command to request atomic\n> pushes.\n> \n> [...]\n> \n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index 21b3f29..da63bdf 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=]\n> +'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=]\n> \t [--repo=] [-f | --force] [--prune] [-v | --verbose]\n> \t [-u | --set-upstream] [--signed]\n> \t [--force-with-lease[=[:]]]\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> +--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> [...]\n\nI'd like to discuss the big picture around this feature. I don't think\nthat any of these questions are blockers, with the possible exception of\nthe question of whether \"--atomic\" should fall back to non-atomic if the\nserver doesn't support atomic pushes.\n\n\n1. Should \"--atomic\" someday become the default?\n\nYou seem to imply that \"--atomic\" might become the default sometime in\nthe future. (I realize that this patch series does not propose to change\nthe default but let's talk about it anyway.) In the real world, the most\ncommon reason for an \"--atomic\" push to fail would be that somebody else\nhas pushed to a branch since our last update, resulting in a non-ff\nerror. Would I find out about such an error before or after I have\ntransferred my objects to the server?\n\nIf I only find out at the end of the transfer, then it could be a pretty\nfrustrating experience pushing a lot of references to a server over a\nslow connection. After waiting for a long transfer to complete the user\nwould find out that the push was rejected and everything has to be done\nagain from scratch. In such cases non-\"--atomic\" behavior might be\nattractive: any references that can be updated should be updated, so\nthat not *all* of the objects have to be pushed again.\n\nEven *if* \"--atomic\" becomes the default, we would certainly want to\nsupport a \"--no-atomic\" (or \"--non-atomic\"?) option to get the old\nbehavior. It might be a good idea to add that option now, so that\nforward-looking script writers can start explicitly choosing \"--atomic\"\nvs. \"--no-atomic\".\n\n\n2. Is this an option that users will want to specify via the command line?\n\nFor scripts that want to insist on \"atomic\" updates, it is no problem to\nspecify \"--atomic\" on the command line.\n\nBut supposing that \"--atomic\" is a good default for some people, it\nwould be awkward for them to have to specify it on every \"git push\"\ninvocation. It therefore might be nice to have a configuration setting\nto choose whether \"--atomic\" is the default.\n\nAlso (see above) it might be useful to set \"--atomic\" only for\nparticular servers (for example, only for those to which you have a fast\nconnection). This suggests that the \"atomic/non-atomic\" configuration\nshould be settable on a per-remote basis.\n\n\n3. What should happen if the server doesn't support atomic pushes?\n\nIt seems to me that there are four reasonable behaviors WRT atomic\npushes. I'll give them tentative names for the purposes of this discussion:\n\n\"force\" -- Insist on an atomic push, and fail if the server does not\nsupport atomic pushes\n\n\"true\" -- Use atomic push if supported by the server. If not, emit a\nwarning and fall back to non-atomic.\n\n\"auto\" -- Use atomic push if supported by the server. If not, silently\nfall back to non-atomic.\n\n\"false\" -- Push non-atomically regardless of whether the server supports\natomic pushes.\n\nTo make it practical to set \"atomic\" as a universal default, we will\nhave to be able to deal with servers that don't (yet) support atomic\npushes. Therefore, we would want to use either \"true\" or \"auto\" (as\nopposed to \"force\") as the default.\n\nEven in such a case, it would be nice for the user to be able to\nsuppress any warnings for servers that don't support atomic updates. So\nif \"true\" becomes the default, then we might want to support \"auto\" as well.\n\n(One could argue that once \"atomic\" becomes the default, then anybody\nwho has to deal with an old server should just set \"non-atomic\" as the\ndefault for that server. But even aside from the inconvenience to the\nuser, this is not a good alternative. A user who made that change\nwouldn't benefit from atomic pushes once the server is upgraded to\nsupport them.)\n\nThe command-line \"--atomic\" option is proposed to request \"force\"\nbehavior. On the one hand that makes sense; the user has explicitly\nrequested an atomic push, so the command should fail if it is not\npossible. But on the other hand, if a particular server doesn't (yet)\nsupport atomic pushes, what recourse does the user have but to run the\npush again non-atomically? So it might be more expedient for the\n\"--atomic\" option to be equivalent to \"true\" instead of \"force\". I don't\nhave a strong opinion on this either way.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"254145","messageId":"CAGZ79kY7T20T8Z0SKjv20W0fiK7LO=EKj93XU8O13Nb6bLLsOg@mail.gmail.com","threadId":"38210","inReplyTo":"549D0B78.402@alum.mit.edu","subject":"Re: [PATCH 6/7] push.c: add an --atomic argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-29T18:14:52Z","receivedAt":"2014-12-29T18:14:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 25, 2014 at 11:17 PM, Michael Haggerty wrote:\n> On 12/19/2014 08:39 PM, Stefan Beller wrote:\n>> Add a command line argument to the git push command to request atomic\n>> pushes.\n>>\n>> [...]\n>>\n>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n>> index 21b3f29..da63bdf 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=]\n>> +'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=]\n>> [--repo=] [-f | --force] [--prune] [-v | --verbose]\n>> [-u | --set-upstream] [--signed]\n>> [--force-with-lease[=[:]]]\n>> @@ -136,6 +136,11 @@ already exists on the remote side.\n>> logged. See linkgit:git-receive-pack[1] for the details\n>> on the receiving end.\n>>\n>> +--atomic::\n>> + Use an atomic transaction on the remote side if available.\n>> + Either all refs are updated, or on error, no refs are updated.\n>> + If the server does not support atomic pushes the push will fail.\n>> +\n>> [...]\n>\n> I'd like to discuss the big picture around this feature. I don't think\n> that any of these questions are blockers, with the possible exception of\n> the question of whether \"--atomic\" should fall back to non-atomic if the\n> server doesn't support atomic pushes.\n>\n>\n> 1. Should \"--atomic\" someday become the default?\n>\n> You seem to imply that \"--atomic\" might become the default sometime in\n> the future. (I realize that this patch series does not propose to change\n> the default but let's talk about it anyway.) In the real world, the most\n> common reason for an \"--atomic\" push to fail would be that somebody else\n> has pushed to a branch since our last update, resulting in a non-ff\n> error. Would I find out about such an error before or after I have\n> transferred my objects to the server?\n\nAnother idea which I picked up from later patches in Ronnies original series\nwould be to have a hidden refs directory at the server side. Then the push\ncommand would first transfer all the objects to this hidden refs space and at\nthe very end the server would just update all the refs atomically.\n\nIf that fails because somebody else has just pushed stuff to one of the\nbranches you intended to update, the changes are still at the server in the\nhidden space. So if you just did a pull/merge on that branch and then\ntry to push\nagain, you would not have to transmit the objects again as they are\nalready there\nat the server.\n\nThese changes in such a hidden space on the server side could be just normal\nrefs just not(never!) advertised to users.\n\n>\n> If I only find out at the end of the transfer, then it could be a pretty\n> frustrating experience pushing a lot of references to a server over a\n> slow connection. After waiting for a long transfer to complete the user\n> would find out that the push was rejected and everything has to be done\n> again from scratch. In such cases non-\"--atomic\" behavior might be\n> attractive: any references that can be updated should be updated, so\n> that not *all* of the objects have to be pushed again.\n>\n> Even *if* \"--atomic\" becomes the default, we would certainly want to\n> support a \"--no-atomic\" (or \"--non-atomic\"?) option to get the old\n> behavior. It might be a good idea to add that option now, so that\n> forward-looking script writers can start explicitly choosing \"--atomic\"\n> vs. \"--no-atomic\".\n\nThat's a good point. Though I am not sure where you'd want me to add the\n--no-atomic flag.\n\nI don't think we'll change send-pack as it's plumbing. So current unmaintained\nscripts should continue to work the way they always did, i.e. having\nthe --no-atomic\nbehavior. The atomic behavior made default would just be part of git\npush, which is\nporcelain which is expected to change? But there it makes sense to have a\n--no-atomic option in place.\n\n>\n>\n> 2. Is this an option that users will want to specify via the command line?\n>\n> For scripts that want to insist on \"atomic\" updates, it is no problem to\n> specify \"--atomic\" on the command line.\n>\n> But supposing that \"--atomic\" is a good default for some people, it\n> would be awkward for them to have to specify it on every \"git push\"\n> invocation. It therefore might be nice to have a configuration setting\n> to choose whether \"--atomic\" is the default.\n\nThat's true. Would it make sense to include that in this series or delay it for\nanother series? I don't want to make the series so large again so it becomes\nreviewer-unfriendly.\n\n>\n> Also (see above) it might be useful to set \"--atomic\" only for\n> particular servers (for example, only for those to which you have a fast\n> connection). This suggests that the \"atomic/non-atomic\" configuration\n> should be settable on a per-remote basis.\n\nSo on the client side you want to configure on a per-remote basis which default\nbehavior to use and also at the server side if you advertise it at all?\n\n>\n>\n> 3. What should happen if the server doesn't support atomic pushes?\n>\n> It seems to me that there are four reasonable behaviors WRT atomic\n> pushes. I'll give them tentative names for the purposes of this discussion:\n>\n> \"force\" -- Insist on an atomic push, and fail if the server does not\n> support atomic pushes\n>\n> \"true\" -- Use atomic push if supported by the server. If not, emit a\n> warning and fall back to non-atomic.\n\nI am not sure about this one. This only makes sense if atomic were the default\nand the user doesn't really care about atomicity and just wants to get things\nto the remote. But for this kind of users we want the \"auto\" option,\nso the user is\nnot bothered by some warning they don't care about.\n\nIf the user however had the intention of using atomic behavior (i.e.\nhas specified\n--atomic explicitly), the user probably wants to have the \"force\" behavior.\n\n>\n> \"auto\" -- Use atomic push if supported by the server. If not, silently\n> fall back to non-atomic.\n>\n> \"false\" -- Push non-atomically regardless of whether the server supports\n> atomic pushes.\n>\n> To make it practical to set \"atomic\" as a universal default, we will\n> have to be able to deal with servers that don't (yet) support atomic\n> pushes. Therefore, we would want to use either \"true\" or \"auto\" (as\n> opposed to \"force\") as the default.\n>\n> Even in such a case, it would be nice for the user to be able to\n> suppress any warnings for servers that don't support atomic updates. So\n> if \"true\" becomes the default, then we might want to support \"auto\" as well.\n>\n> (One could argue that once \"atomic\" becomes the default, then anybody\n> who has to deal with an old server should just set \"non-atomic\" as the\n> default for that server. But even aside from the inconvenience to the\n> user, this is not a good alternative. A user who made that change\n> wouldn't benefit from atomic pushes once the server is upgraded to\n> support them.)\n>\n> The command-line \"--atomic\" option is proposed to request \"force\"\n> behavior. On the one hand that makes sense; the user has explicitly\n> requested an atomic push, so the command should fail if it is not\n> possible. But on the other hand, if a particular server doesn't (yet)\n> support atomic pushes, what recourse does the user have but to run the\n> push again non-atomically? So it might be more expedient for the\n> \"--atomic\" option to be equivalent to \"true\" instead of \"force\". I don't\n> have a strong opinion on this either way.\n\nIt doesn't take much time to find out if the server supports atomic pushes\nas you don't have to send the objects, but just wait until the server has\nadvertised its refs and capabilities. Then the user could rerun the command\nline without having the --atomic flag set or even using --no-atomic.\n\nThanks for your detailed discussion as I wasn't thinking so far ahead yet.\n\n> I'd like to discuss the big picture around this feature. I don't think\n> that any of these questions are blockers, with the possible exception of\n> the question of whether \"--atomic\" should fall back to non-atomic if the\n> server doesn't support atomic pushes.\n\nIf we make atomic default, then we should have this kind of fallback, if not\nexplicitly suppressed (i.e. the user should experience as little\nrejection as possible,\nwarnings/hints may be ok? If the user really wants to do it\natomically, make it so)\n\nFor the current state after this patch series when atomic is not default (yet),\nwe probably don't need the fall back as we only talk about the \"force\"\noption with\nthis series.\n\n\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n>\n"},{"id":"254151","messageId":"xmqq7fxayz4u.fsf@gitster.dls.corp.google.com","threadId":"38210","inReplyTo":"549D0B78.402@alum.mit.edu","subject":"Re: [PATCH 6/7] push.c: add an --atomic argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-29T20:33:37Z","receivedAt":"2014-12-29T20:33:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I'd like to discuss the big picture around this feature. I don't think\n> that any of these questions are blockers, with the possible exception of\n> the question of whether \"--atomic\" should fall back to non-atomic if the\n> server doesn't support atomic pushes.\n>\n> 1. Should \"--atomic\" someday become the default?\n>\n> You seem to imply that \"--atomic\" might become the default sometime in\n> the future. (I realize that this patch series does not propose to change\n> the default but let's talk about it anyway.) In the real world, the most\n> common reason for an \"--atomic\" push to fail would be that somebody else\n> has pushed to a branch since our last update, resulting in a non-ff\n> error. Would I find out about such an error before or after I have\n> transferred my objects to the server?\n\nThat question is pretty much rhetorical, as certain rejections you\ncannot fundamentally implement without having the data at hand.\n\n> If I only find out at the end of the transfer, then it could be a pretty\n> frustrating experience pushing a lot of references to a server over a\n> slow connection.\n\nWe'd like to have a long overdue protocol update for fetch and push\nsoonish anyawy (perhaps in the first half of 2015) and part of that\nshould include unified logic for common ancestor negotiation between\nfetch and push [*1*]. We should be able to ease that with an\noptimization similar to quickfetch done on the fetch side once that\nhappens.\n\n> Even *if* \"--atomic\" becomes the default, we would certainly want to\n> support a \"--no-atomic\" (or \"--non-atomic\"?) option to get the old\n> behavior. It might be a good idea to add that option now, so that\n> forward-looking script writers can start explicitly choosing \"--atomic\"\n> vs. \"--no-atomic\".\n\nPerhaps. But on the other hand, pushing multiple refs at the same\ntime is a sign that they are related and need to go together. The\nreason why one but not others fails would be an indication that\nthere is somebody else pushing into the same repository and the\npusher and the other party are stepping on each other's toes, which\nshould be resolved primarily by inter-developer communication, not\nwith \"--no-atomic\" workaround.\n\n> 2. Is this an option that users will want to specify via the command line?\n>\n> For scripts that want to insist on \"atomic\" updates, it is no problem to\n> specify \"--atomic\" on the command line.\n>\n> But supposing that \"--atomic\" is a good default for some people, it\n> would be awkward for them to have to specify it on every \"git push\"\n> invocation. It therefore might be nice to have a configuration setting\n> to choose whether \"--atomic\" is the default.\n>\n> Also (see above) it might be useful to set \"--atomic\" only for\n> particular servers (for example, only for those to which you have a fast\n> connection). This suggests that the \"atomic/non-atomic\" configuration\n> should be settable on a per-remote basis.\n\nI think you are hinting to have remote.atomicPush = {yes,no} that is\nweaker than remote.$nick.atomicPush = {yes,no} or something like\nthat. I agree that would be a good direction to go.\n\n> 3. What should happen if the server doesn't support atomic pushes?\n\nIf you asked for atomic push explicitly and if the server cannot\nsupport it, push should fail.\n\nIf the only reason we are doing atomic is because in some future we\nflipped the default (i.e. no remote.*.atomicPush or --atomic option\nfrom the command line), then it might be OK to continue with a\nwarning.\n\nI however think that the automatic demotion is too much complexity\nfor such a simple option. \"Please be atomic if you can but I'd take\nnon-atomic one if you do not want to give me atomic one\" that is\nresponded by \"I'd do non-atomic then, as you are perfectly happy\nwithout\" is not very useful---such a pusher should just say \"I\naccept non-atomic\", which is what \"--no-atomic\" is for.\n\n\n[Footnotes]\n\n*1* Two other big ones are syntax change to have an explicit\n extension packets instead of hiding the capability after NUL,\n and resolving the \"who speaks first\" issue.\n"},{"id":"254201","messageId":"xmqqvbktrsom.fsf@gitster.dls.corp.google.com","threadId":"38210","inReplyTo":"549A6C4C.8000905@alum.mit.edu","subject":"Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-30T16:47:05Z","receivedAt":"2014-12-30T16:47:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> If I understand correctly, after this patch the server advertises the\n> \"atomic\" capability even though it doesn't actually have that ability\n> until a later patch. It seems to me that the order of the patches should\n> be reversed: don't advertise the capability before it is actually\n> implemented. Why? Bisection. Between the two patches the server is buggy.\n\nThat is a valid point. It also reminds us of another thing.\n\nWe would need a way to test interoperability among the three new\ncombinations (i.e. new and old receive-pack talking to new and old\n\"git push\"). We can control what the sender talks on the wire by\ngiving or not giving \"--atomic\" option on the command line, but\nthere should be a way for us to control what the receiver talks on\nthe wire, i.e. \"receivepack.pushAtomic = false\" that tells us not to\nadvertise the \"atomic push\" capability over the wire, even if you\nare running the updated \"receive-pack\" binary.\n\nThis will not only for testing. When we discover that \"atomic\"\nsupport is undesirable for whatever reason (e.g. the transaction\nmachinery may consume too many file descriptors without a good\nreason), we would need a way for users to disable it until the\nproblem is fixed.\n\nAnd of course the tests should make sure that\n\n (1) \"git push --atomic\" that talks with the receiving end that has\n receivepack.pushAtomic set to false behaves as we desire (error\n out? degrade to non-atomic? --- whichever way we decide).\n\n (2) \"git push\" that talks with the receiving end with \"atomic\" enabled\n does not do an atomic push, i.e. try to push two refs, one that\n fast forwards and the other that does not, and see one ref is\n updated while the other ref remains intact and \"git push\"\n itself reports failured.;\n\n (3) \"git push --atomic\" that talks with the receiving end with\n \"atomic\" enabled does the atomic thing.\n\namong other things.\n"}]}