{"thread":{"id":"38179","subject":"[PATCH 0/5] Add a flag to push atomically","startedAt":"2014-12-15T19:56:03Z","lastAt":"2014-12-19T10:14:20Z","messageCount":56,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"253697","messageId":"1418673368-20785-1-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":null,"subject":"[PATCH 0/5] Add a flag to push atomically","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-15T19:56:03Z","receivedAt":"2014-12-15T19:56:03Z","isPatch":true,"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\nIt was part of a longer patch series[1].\nThis series applies on top of origin/mh/reflog-expire\nIt can also be found at [2].\n\nChange since picking the series up from Ronnie.\n * other anchor point (i.e. where the series applies)\n * more tests for this feature, specially testing failures\n * drop the patch to rename ref_transaction_* to transaction_*_ref\n * slight rewording of the additional documentation\n\n[1] http://www.spinics.net/lists/git/msg241214.html\n[2] https://github.com/stefanbeller/git/tree/atomic-push-v1\n\nRonnie Sahlberg (4):\n receive-pack.c: add protocol support to negotiate atomic-push\n send-pack.c: add an --atomic-push command line argument\n receive-pack.c: use a single transaction when atomic-push is\n negotiated\n push.c: add an --atomic-push argument\n\nStefan Beller (1):\n t5543-atomic-push.sh: add basic tests for atomic pushes\n\n Documentation/git-push.txt | 8 +-\n Documentation/git-send-pack.txt | 7 +-\n Documentation/technical/protocol-capabilities.txt | 12 +-\n builtin/push.c | 2 +\n builtin/receive-pack.c | 79 +++++++--\n builtin/send-pack.c | 6 +-\n remote.h | 3 +-\n send-pack.c | 45 +++++-\n send-pack.h | 1 +\n t/t5543-atomic-push.sh | 185 ++++++++++++++++++++++\n transport.c | 5 +\n transport.h | 1 +\n 12 files changed, 327 insertions(+), 27 deletions(-)\n create mode 100755 t/t5543-atomic-push.sh\n\n-- \n2.2.0.33.gc2219e3.dirty\n"},{"id":"253700","messageId":"1418673368-20785-2-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418673368-20785-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/5] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-15T19:56:04Z","receivedAt":"2014-12-15T19:56:04Z","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 Documentation/technical/protocol-capabilities.txt | 12 ++++++++++--\n builtin/receive-pack.c | 6 +++++-\n send-pack.c | 6 ++++++\n 3 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 6d5424c..763120c 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-push', '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,13 @@ 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-push\n+-----------\n+\n+If the server sends the 'atomic-push' capability, it means it is\n+capable of accepting atomic pushes. If the pushing client requests this\n+capability, the server will update the refs in one single atomic transaction.\n+\n allow-tip-sha1-in-want\n ----------------------\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 32fc540..0c642ab 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_push;\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-push\");\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-push\"))\n+\t\t\t\tuse_atomic_push = 1;\n \t\t}\n \n \t\tif (!strcmp(line, \"push-cert\")) {\ndiff --git a/send-pack.c b/send-pack.c\nindex 949cb61..1ccc84c 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 atomic_push_supported = 0;\n+\tint atomic_push = 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-push\"))\n+\t\tatomic_push_supported = 1;\n \tif (args->push_cert) {\n \t\tint len;\n \n@@ -335,6 +339,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 (atomic_push)\n+\t\tstrbuf_addstr(&cap_buf, \" atomic-push\");\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n \n-- \n2.2.0.33.gc2219e3.dirty\n"},{"id":"253701","messageId":"1418673368-20785-3-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418673368-20785-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/5] send-pack.c: add an --atomic-push command line argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-15T19:56:05Z","receivedAt":"2014-12-15T19:56:05Z","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-push.\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 Documentation/git-send-pack.txt | 7 ++++++-\n builtin/send-pack.c | 6 +++++-\n remote.h | 3 ++-\n send-pack.c | 39 ++++++++++++++++++++++++++++++++++-----\n send-pack.h | 1 +\n transport.c | 4 ++++\n 6 files changed, 52 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 2a0de42..9296587 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-push] [:] [...]\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-push::\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..93cb17c 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-push] [:] [...]\\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-push\")) {\n+\t\t\t\targs.use_atomic_push = 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 1ccc84c..08602a8 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -190,7 +190,7 @@ 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+static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args, int *atomic_push_failed)\n {\n \tif (!ref->peer_ref && !args->send_mirror)\n \t\treturn 0;\n@@ -203,6 +203,12 @@ 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\tif (atomic_push_failed) {\n+\t\t\tfprintf(stderr, \"Atomic push failed for ref %s. \"\n+\t\t\t\t\"Status:%d\\n\", ref->name, ref->status);\n+\t\t\t*atomic_push_failed = 1;\n+\t\t}\n+\t\t/* fallthrough */\n \tcase REF_STATUS_UPTODATE:\n \t\treturn 0;\n \tdefault:\n@@ -250,7 +256,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 (!ref_update_to_be_sent(ref, args, NULL))\n \t\t\tcontinue;\n \t\tupdate_seen = 1;\n \t\tstrbuf_addf(&cert, \"%s %s %s\\n\",\n@@ -297,7 +303,7 @@ int send_pack(struct send_pack_args *args,\n \tint atomic_push_supported = 0;\n \tint atomic_push = 0;\n \tunsigned cmds_sent = 0;\n-\tint ret;\n+\tint ret, atomic_push_failed = 0;\n \tstruct async demux;\n \tconst char *push_cert_nonce = NULL;\n \n@@ -332,6 +338,11 @@ 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->use_atomic_push && !atomic_push_supported) {\n+\t\tfprintf(stderr, \"Server does not support atomic-push.\");\n+\t\treturn -1;\n+\t}\n+\tatomic_push = atomic_push_supported && args->use_atomic_push;\n \n \tif (status_report)\n \t\tstrbuf_addstr(&cap_buf, \" report-status\");\n@@ -365,7 +376,8 @@ 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 (!ref_update_to_be_sent(ref, args,\n+\t\t\targs->use_atomic_push ? &atomic_push_failed : NULL))\n \t\t\tcontinue;\n \n \t\tif (!ref->deletion)\n@@ -377,6 +389,23 @@ int send_pack(struct send_pack_args *args,\n \t\t\tref->status = REF_STATUS_EXPECTING_REPORT;\n \t}\n \n+\tif (atomic_push_failed) {\n+\t\tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\t\tif (!ref->peer_ref && !args->send_mirror)\n+\t\t\t\tcontinue;\n+\n+\t\t\tswitch (ref->status) {\n+\t\t\tcase REF_STATUS_EXPECTING_REPORT:\n+\t\t\t\tref->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n+\t\t\t\tcontinue;\n+\t\t\tdefault:\n+\t\t\t\t; /* do nothing */\n+\t\t\t}\n+\t\t}\n+\t\tfprintf(stderr, \"Atomic push failed.\");\n+\t\treturn -1;\n+\t}\n+\n \t/*\n \t * Finally, tell the other end!\n \t */\n@@ -386,7 +415,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 (!ref_update_to_be_sent(ref, args, NULL))\n \t\t\tcontinue;\n \n \t\told_hex = sha1_to_hex(ref->old_sha1);\ndiff --git a/send-pack.h b/send-pack.h\nindex 5635457..7486e65 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -11,6 +11,7 @@ struct send_pack_args {\n \t\tforce_update:1,\n \t\tuse_thin_pack:1,\n \t\tuse_ofs_delta:1,\n+\t\tuse_atomic_push:1,\n \t\tdry_run:1,\n \t\tpush_cert:1,\n \t\tstateless_rpc:1;\ndiff --git a/transport.c b/transport.c\nindex 70d38e4..e87481d 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.0.33.gc2219e3.dirty\n"},{"id":"253699","messageId":"1418673368-20785-4-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418673368-20785-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/5] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-15T19:56:06Z","receivedAt":"2014-12-15T19:56:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nUpdate 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\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n builtin/receive-pack.c | 73 +++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 58 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0c642ab..e6cf174 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -67,6 +67,8 @@ 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+struct strbuf err = STRBUF_INIT;\n+struct ref_transaction *transaction;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -832,34 +834,56 @@ 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\t\treturn \"failed to delete\";\n+\t\tif (!use_atomic_push) {\n+\t\t\tif (delete_ref(namespaced_name, old_sha1, 0)) {\n+\t\t\t\trp_error(\"failed to delete %s\", name);\n+\t\t\t\treturn \"failed to delete\";\n+\t\t\t}\n+\t\t} else {\n+\t\t\tif (ref_transaction_delete(transaction,\n+\t\t\t\t\t\t namespaced_name,\n+\t\t\t\t\t\t old_sha1,\n+\t\t\t\t\t\t 0, old_sha1 != NULL,\n+\t\t\t\t\t\t \"push\", &err)) {\n+\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\tstrbuf_release(&err);\n+\t\t\t\treturn \"failed to delete\";\n+\t\t\t}\n \t\t}\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 (!use_atomic_push) {\n+\t\t\ttransaction = ref_transaction_begin(&err);\n+\t\t\tif (!transaction) {\n+\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\tstrbuf_release(&err);\n+\t\t\t\treturn \"failed to start transaction\";\n+\t\t\t}\n+\t\t}\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 \t\t\treturn \"failed to update ref\";\n \t\t}\n \n-\t\tref_transaction_free(transaction);\n+\t\tif (!use_atomic_push) {\n+\t\t\tif (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\treturn \"failed to update ref\";\n+\t\t\t}\n+\t\t\tref_transaction_free(transaction);\n+\t\t}\n \t\tstrbuf_release(&err);\n \t\treturn NULL; /* good */\n \t}\n@@ -1059,6 +1083,16 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\tif (use_atomic_push) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"transaction error\";\n+\t\t\treturn;\n+\t\t}\n+\t}\n \tdata.cmds = commands;\n \tdata.si = si;\n \tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n@@ -1096,6 +1130,14 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n+\tif (use_atomic_push) {\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = err.buf;\n+\t\t}\n+\t\tref_transaction_free(transaction);\n+\t}\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@@ -1543,5 +1585,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tsha1_array_clear(&shallow);\n \tsha1_array_clear(&ref);\n \tfree((void *)push_cert_nonce);\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n-- \n2.2.0.33.gc2219e3.dirty\n"},{"id":"253702","messageId":"1418673368-20785-5-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418673368-20785-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/5] push.c: add an --atomic-push argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-15T19:56:07Z","receivedAt":"2014-12-15T19:56:07Z","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 Documentation/git-push.txt | 8 +++++++-\n builtin/push.c | 2 ++\n transport.c | 1 +\n transport.h | 1 +\n 4 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 21b3f29..3e0cb6e 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-push] [-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,12 @@ 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-push::\n+\tUsing atomic push. If atomic push is negotiated with the server\n+\tthen any push covering multiple refs will be atomic. Either all\n+\trefs are updated, or on error, no refs are updated. If the server\n+\tdoes 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..ae393c5 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-push\", &flags, N_(\"use atomic push, if available\"),\n+\t\t\tTRANSPORT_ATOMIC_PUSH),\n \t\tOPT_END()\n \t};\n \ndiff --git a/transport.c b/transport.c\nindex e87481d..ba1499e 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.use_atomic_push = !!(flags & TRANSPORT_ATOMIC_PUSH);\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..25fa1da 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_ATOMIC_PUSH 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.0.33.gc2219e3.dirty\n"},{"id":"253698","messageId":"1418673368-20785-6-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418673368-20785-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/5] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-15T19:56:08Z","receivedAt":"2014-12-15T19:56:08Z","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 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 | 185 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 185 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..6cbedc5\n--- /dev/null\n+++ b/t/t5543-atomic-push.sh\n@@ -0,0 +1,185 @@\n+#!/bin/sh\n+\n+test_description='pushing to a repository using the atomic push option'\n+\n+. ./test-lib.sh\n+\n+D=`pwd`\n+\n+mk_repo_pair () {\n+\trm -rf workbench upstream thirdparty &&\n+\tmkdir upstream &&\n+\t(\n+\t\tcd upstream &&\n+\t\tgit init --bare #&&\n+\t\t#git config receive.denyCurrentBranch warn\n+\t) &&\n+\tmkdir workbench &&\n+\t(\n+\t\tcd workbench &&\n+\t\tgit init &&\n+\t\tgit remote add up ../upstream\n+\t)\n+}\n+\n+test_push_failed () {\n+\tworkbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&\n+\tupstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&\n+\ttest \"$workbench_master\" != \"$upstream_master\" &&\n+\n+\tworkbench_second=$(cd workbench && git show-ref -s --verify refs/heads/second) &&\n+\tupstream_second=$(cd upstream && git show-ref -s --verify refs/heads/second) &&\n+\ttest \"$workbench_second\" != \"$upstream_second\"\n+}\n+\n+test_push_succeeded () {\n+\tworkbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&\n+\tupstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&\n+\ttest \"$workbench_master\" = \"$upstream_master\"\n+\n+\tworkbench_second=$(cd workbench && git show-ref -s --verify refs/heads/second) &&\n+\tupstream_second=$(cd upstream && git show-ref -s --verify refs/heads/second) &&\n+\ttest \"$workbench_second\" = \"$upstream_second\"\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-push --mirror up\n+\t) &&\n+\tworkbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&\n+\tupstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&\n+\ttest \"$workbench_master\" = \"$upstream_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-push --mirror up\n+\t) &&\n+\ttest_push_succeeded\n+'\n+\n+test_expect_success 'atomic push works in combination with --mirror' '\n+\tmk_repo_pair &&\n+\tls workbench &&\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-push --mirror up\n+\t) &&\n+\ttest_push_succeeded\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 checkout -b second &&\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^ &&\n+\t\tgit push --force --atomic-push up master second\n+\t) &&\n+\ttest_push_succeeded\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 locally' '\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\tgit checkout master &&\n+\t\ttest_commit five &&\n+\t\t! git push --atomic-push --all up\n+\t) &&\n+\ttest_push_failed\n+'\n+\n+test_expect_success 'atomic push fails if one branch 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\tgit clone upstream thirdparty &&\n+\t\tcd thirdparty\n+\t\tgit checkout second\n+\t\ttest_commit unrelated_changes &&\n+\t\tgit push origin 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\t! git push --atomic-push up master second\n+\t) &&\n+\ttest_push_failed\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\tgit clone upstream thirdparty &&\n+\t\tcd thirdparty\n+\t\tgit checkout second\n+\t\tgit tag test_tag &&\n+\t\tgit push --tags origin 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\t! git push --tags --atomic-push up master second\n+\t) &&\n+\ttest_push_failed\n+'\n+\n+test_done\n-- \n2.2.0.33.gc2219e3.dirty\n"},{"id":"253706","messageId":"xmqqy4q8d493.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418673368-20785-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/5] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T20:53:12Z","receivedAt":"2014-12-15T20:53:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\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\nHmph, am I reading the patch to send-pack.c correctly?\n\nIt detects if the other side supports the capability and leaves it\nin atomic_push_supported variable for later use, and also requests\nthe feature to be activated when atomic_push is set, but I see no\nlogic to link these two together, e.g. error out when atomic_push\nis true and atomic_push_supported is false (or turn it off with a\nwarning, or whatever).\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 949cb61..1ccc84c 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 atomic_push_supported = 0;\n> +\tint atomic_push = 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-push\"))\n> +\t\tatomic_push_supported = 1;\n> \tif (args->push_cert) {\n> \t\tint len;\n> \n> @@ -335,6 +339,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 (atomic_push)\n> +\t\tstrbuf_addstr(&cap_buf, \" atomic-push\");\n> \tif (agent_supported)\n> \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n"},{"id":"253707","messageId":"xmqqsiggd3vk.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418673368-20785-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/5] send-pack.c: add an --atomic-push command line argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T21:01:19Z","receivedAt":"2014-12-15T21:01:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> -static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)\n> +static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args, int *atomic_push_failed)\n\nHmph. Is \"atomic push\" so special that it deserves a separate\nparameter? When we come up with yet another mode of failure, would\nwe add another parameter to the callers to this function?\n\n> @@ -203,6 +203,12 @@ 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\tif (atomic_push_failed) {\n> +\t\t\tfprintf(stderr, \"Atomic push failed for ref %s. \"\n> +\t\t\t\t\"Status:%d\\n\", ref->name, ref->status);\n> +\t\t\t*atomic_push_failed = 1;\n\nAll other error message that come from the codepaths around here\nseem to avoid uppercase. Also maybe you want to use error() here?\n\n> +\tif (args->use_atomic_push && !atomic_push_supported) {\n> +\t\tfprintf(stderr, \"Server does not support atomic-push.\");\n> +\t\treturn -1;\n> +\t}\n\nThis check logically belongs to the previous step, no?\n\n> +\tatomic_push = atomic_push_supported && args->use_atomic_push;\n\n> \n> \tif (status_report)\n> \t\tstrbuf_addstr(&cap_buf, \" report-status\");\n> @@ -365,7 +376,8 @@ 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 (!ref_update_to_be_sent(ref, args,\n> +\t\t\targs->use_atomic_push ? &atomic_push_failed : NULL))\n> \t\t\tcontinue;\n> \n> \t\tif (!ref->deletion)\n> @@ -377,6 +389,23 @@ int send_pack(struct send_pack_args *args,\n> \t\t\tref->status = REF_STATUS_EXPECTING_REPORT;\n> \t}\n> \n> +\tif (atomic_push_failed) {\n> +\t\tfor (ref = remote_refs; ref; ref = ref->next) {\n> +\t\t\tif (!ref->peer_ref && !args->send_mirror)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tswitch (ref->status) {\n> +\t\t\tcase REF_STATUS_EXPECTING_REPORT:\n> +\t\t\t\tref->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n> +\t\t\t\tcontinue;\n> +\t\t\tdefault:\n> +\t\t\t\t; /* do nothing */\n> +\t\t\t}\n> +\t\t}\n> +\t\tfprintf(stderr, \"Atomic push failed.\");\n> +\t\treturn -1;\n\nThe same comment as the other fprintf() applies here.\n\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\nWhy dashed-words-here?\n\n> +\t\tbreak;\n> \tcase REF_STATUS_OK:\n> \t\tprint_ok_ref_status(ref, porcelain);\n> \t\tbreak;\n"},{"id":"253708","messageId":"xmqqiohcd272.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418673368-20785-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/5] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T21:37:37Z","receivedAt":"2014-12-15T21:37:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> @@ -832,34 +834,56 @@ 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\t\treturn \"failed to delete\";\n> +\t\tif (!use_atomic_push) {\n> +\t\t\tif (delete_ref(namespaced_name, old_sha1, 0)) {\n> +\t\t\t\trp_error(\"failed to delete %s\", name);\n> +\t\t\t\treturn \"failed to delete\";\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tif (ref_transaction_delete(transaction,\n> +\t\t\t\t\t\t namespaced_name,\n> +\t\t\t\t\t\t old_sha1,\n> +\t\t\t\t\t\t 0, old_sha1 != NULL,\n> +\t\t\t\t\t\t \"push\", &err)) {\n> +\t\t\t\trp_error(\"%s\", err.buf);\n> +\t\t\t\tstrbuf_release(&err);\n> +\t\t\t\treturn \"failed to delete\";\n> +\t\t\t}\n\nDoesn't the asymmetry between the above (if transaction is there,\nuse it, otherwise call delete_ref() which conceptually has its sole\noperation inside a single transaction by itself) and below (if\ntransaction is not there, create it and do its thing, and close the\ntransaction if we created it) bother you?\n\nThe above look much simpler, and if it does not switch on\nuse_atomic_push but on the presense of transaction, it would have\nbeen even better, i.e.\n\n\tif (transaction\n ? ref_transaction_delete(transaction, ...)\n : delete_ref(...)) {\n\t\terror(...);\n return \"failed to delete\";\n\t}\n\nI think it makes the code harder to read and maintain if you forced\na caller of the ref API that happen to touch only a single ref to\nmake three calls to\n\n\tref_transaction_begin();\n ref_transaction_do_one_thing();\n ref_transaction_commit();\n\ninstead of making a single call to a simple wrapper\n\n\tref_do_one_thing();\n\nI think that I saw Michael make a similar observation in a near-by\nthread.\n\nEven if you insist using transactions explicitly in the user of the\nref API, I think a better code organization is possible in this\nparticular codepath. Because execute_commands() has the loop over\nall the proposed updates, why should the update() even need to know\nhow to open a new transaction and when? In other words, can't the\ncode be more like this?\n\n\tstatic update(transaction, ...)\n {\n\t\t/* do my thing in the transaction given to me */\n compute what kind of update is needed;\n switch (kind) {\n\t\tcase delete:\n\t\t\tref_transaction_delete(transaction, ...);\n\t\t\tbreak;\n\t\tcase update:\n\t\t\tref_transaction_update(transaction, ...);\n\t\t\tbreak;\n\t\t...\n\t\t}\n\t}\n\n\texecute_commands(...)\n {\n\t\tif (atomic)\n \ttransaction = ref_transaction_begin(...);\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n\t\t\tif (!atomic) \t\n \ttransaction = ref_transaction_begin(...);\n\t\t\tupdate(transaction, ...);\n\t\t\tif (!atomic)\n \tref_transaction_commit(transaction);\n\t\t}\n\t\tif (atomic)\n \tref_transaction_commit(transaction);\n\t}\n\nThat is, update() assumes it is always in _some_ transaction, and\nexecute_commands(), which is what drives multi-ref updates, knows\nif it wants its repeated calls to update() to be in a single\ntransaction or separate transactions.\n"},{"id":"253712","messageId":"xmqqa92od1lz.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418673368-20785-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/5] push.c: add an --atomic-push argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T21:50:16Z","receivedAt":"2014-12-15T21:50:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> From: Ronnie Sahlberg \n>\n> Add a command line argument to the git push command to request atomic\n> pushes.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n\nMakes sense, modulo a nit.\n\n> +--atomic-push::\n> +\tUsing atomic push. If atomic push is negotiated with the server\n> +\tthen any push covering multiple refs will be atomic. Either all\n> +\trefs are updated, or on error, no refs are updated. If the server\n> +\tdoes not support atomic pushes the push will fail.\n\n\"git push --atomic-push\"? Why should one need to repeat the word?\n\n> \t\tOPT_BIT(0, \"signed\", &flags, N_(\"GPG sign the push\"), TRANSPORT_PUSH_CERT),\n> +\t\tOPT_BIT(0, \"atomic-push\", &flags, N_(\"use atomic push, if available\"),\n> +\t\t\tTRANSPORT_ATOMIC_PUSH),\n\nContrast between the two. It isn't \"git push --signed-push\", either ;-)\n"},{"id":"253713","messageId":"xmqq4mswczs4.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418673368-20785-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/5] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T22:29:47Z","receivedAt":"2014-12-15T22:29:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\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> option prevents any change to be pushed if just one ref cannot\n> be updated.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\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 | 185 +++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 185 insertions(+)\n> create mode 100755 t/t5543-atomic-push.sh\n>\n> diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\n> new file mode 100755\n> index 0000000..6cbedc5\n> --- /dev/null\n> +++ b/t/t5543-atomic-push.sh\n> @@ -0,0 +1,185 @@\n> +#!/bin/sh\n> +\n> +test_description='pushing to a repository using the atomic push option'\n> +\n> +. ./test-lib.sh\n> +\n> +D=`pwd`\n\n$(pwd)???\n\n> +mk_repo_pair () {\n> +\trm -rf workbench upstream thirdparty &&\n> +\tmkdir upstream &&\n> +\t(\n> +\t\tcd upstream &&\n> +\t\tgit init --bare #&&\n> +\t\t#git config receive.denyCurrentBranch warn\n\nPlease drop unused comments; they are distracting.\n\n> +\t) &&\n> +\tmkdir workbench &&\n> +\t(\n> +\t\tcd workbench &&\n> +\t\tgit init &&\n> +\t\tgit remote add up ../upstream\n> +\t)\n> +}\n\nHmph. Shouldn't you be using test_create_repo to create these, so\nthat templates are used from the build (not install) location, and\ntheir hooks are disabled?\n\n> +test_push_failed () {\n\nDoes that mean \"test_push_must_fail\"?\n\n> +\tworkbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&\n> +\tupstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&\n> +\ttest \"$workbench_master\" != \"$upstream_master\" &&\n> +\n> +\tworkbench_second=$(cd workbench && git show-ref -s --verify refs/heads/second) &&\n> +\tupstream_second=$(cd upstream && git show-ref -s --verify refs/heads/second) &&\n> +\ttest \"$workbench_second\" != \"$upstream_second\"\n\nSo the tests that use this helper always try to update master and\nsecond? Is \"they are different\" the only thing that matters? Or\nshould you be verifying \"They are left as the same value they used\nto have before the attempted push that must have failed\"?\n\nIt might be a good time to use \"-C\" option, e.g. \"git -C workbench show-ref ...\",\na bit more in our tests.\n\n> +}\n> +\n> +test_push_succeeded () {\n> +\tworkbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&\n> +\tupstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&\n> +\ttest \"$workbench_master\" = \"$upstream_master\"\n\nBroken &&-chain?\n\n> +\n> +\tworkbench_second=$(cd workbench && git show-ref -s --verify refs/heads/second) &&\n> +\tupstream_second=$(cd upstream && git show-ref -s --verify refs/heads/second) &&\n> +\ttest \"$workbench_second\" = \"$upstream_second\"\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-push --mirror up\n> +\t) &&\n> +\tworkbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&\n> +\tupstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&\n> +\ttest \"$workbench_master\" = \"$upstream_master\"\n\nHmph. It is a shame that the nice helper you created cannot be used\nhere.\n\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-push --mirror up\n> +\t) &&\n> +\ttest_push_succeeded\n> +'\n\nOK.\n\n> +test_expect_success 'atomic push works in combination with --mirror' '\n> +\tmk_repo_pair &&\n> +\tls workbench &&\n\nDebugging?\n\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-push --mirror up\n\nIt is not wrong per-se, but haven't you already tested in\ncombination with --mirror in the previous test?\n\n> +\t) &&\n> +\ttest_push_succeeded\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 checkout -b second &&\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^ &&\n> +\t\tgit push --force --atomic-push up master second\n> +\t) &&\n> +\ttest_push_succeeded\n> +'\n\nOK.\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 locally' '\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\nBroken &&-chain.\n\n> +\t\tgit reset --hard HEAD~2 &&\n> +\t\tgit checkout master &&\n> +\t\ttest_commit five &&\n> +\t\t! git push --atomic-push --all up\n\ntest_must_fail?\n\n> +\t) &&\n> +\ttest_push_failed\n\nYou not only want to see master and second in upstream different from \nthose in workbench, but they point at specific commits that the previous\nmirror push updated to.\n\nInstead of test_push_failed / test_push_succeeded, how about a\nsingle helper that takes the two commit object names you expect\nthese two branches point at? E.g.\n\n\tcheck_branches upstream master HEAD@{2} second HEAD~\n\n(I am probably miscounting HEAD@{$offset} here; this is for\nillustration only) that roughly does\n\n\tcheck_branches () {\n\t\ttarget=$1\n shift\n while test $# -ne 0\n do\n\t\t\tgit -C \"$target\" rev-parse --verify \"refs/heads/$1\" >actual &&\n\t\t\tgit rev-parse \"$2\" >expect &&\n\t\t\ttest_cmp expect actual || return 1\n\t\t\tshift\n shift\n\t\tdone\n\t}\n\nor something like that?\n\n> +test_expect_success 'atomic push fails if one branch 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\tgit clone upstream thirdparty &&\n> +\t\tcd thirdparty\n> +\t\tgit checkout second\n> +\t\ttest_commit unrelated_changes &&\n> +\t\tgit push origin 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\t! git push --atomic-push up master second\n> +\t) &&\n> +\ttest_push_failed\n> +'\n\nWhat's the value of this test? Isn't it a non-fast-forward check\nyou already tested in the previous one?\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\tgit clone upstream thirdparty &&\n> +\t\tcd thirdparty\n> +\t\tgit checkout second\n> +\t\tgit tag test_tag &&\n> +\t\tgit push --tags origin second\n> +\t) &&\n\nBroken &&-chain aside, you do not need thirdparty. Doing the \"git\ntag\" inside upstream itself should suffice.\n\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\t! git push --tags --atomic-push up master second\n\ntest_must_fail?\n\n> +\t) &&\n> +\ttest_push_failed\n> +'\n\nOne more failure mode you would want to check is what if \"update\"\nhook in the receiving repository rejected one update (but not the\nother). Something along the lines of:\n\n\t... setup ...\n\tgit -C workbench push up &&\n write_script upstream/hooks/update <<-\\EOF &&\n\t# only allow update to master from now on\n test \"$1\" = \"refs/heads/master\"\n EOF\n ... further update to master and second ...\n test_must_fail git -C workbench push up\n\tcheck_branches upstream master ... second ...\n\n\nperhaps?\n"},{"id":"253714","messageId":"CAGZ79kYJjnTaUJMhLWt5q76odqQ_zBifEgVbNLJFdv8BViCCQw@mail.gmail.com","threadId":"38179","inReplyTo":"xmqqy4q8d493.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/5] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-15T22:30:31Z","receivedAt":"2014-12-15T22:30:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 15, 2014 at 12:53 PM, Junio C Hamano wrote:\n> Hmph, am I reading the patch to send-pack.c correctly?\n>\n> It detects if the other side supports the capability and leaves it\n> in atomic_push_supported variable for later use, and also requests\n> the feature to be activated when atomic_push is set, but I see no\n> logic to link these two together, e.g. error out when atomic_push\n> is true and atomic_push_supported is false (or turn it off with a\n> warning, or whatever).\n\n\nThis is what you mean by\n\n>\n>> + if (args->use_atomic_push && !atomic_push_supported) {\n>> + fprintf(stderr, \"Server does not support atomic-push.\");\n>> + return -1;\n>> + }\n>\n> This check logically belongs to the previous step, no?\n\nfrom the next patch? If so it will be part of the next reroll.\n\nThanks,\nStefan\n"},{"id":"253715","messageId":"xmqqzjaobl0q.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418673368-20785-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 0/5] Add a flag to push atomically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T22:33:57Z","receivedAt":"2014-12-15T22:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This patch series adds a flag to git push to update the remote refs atomically.\n>\n> It was part of a longer patch series[1].\n> This series applies on top of origin/mh/reflog-expire\n> It can also be found at [2].\n>\n> Change since picking the series up from Ronnie.\n> * other anchor point (i.e. where the series applies)\n> * more tests for this feature, specially testing failures\n> * drop the patch to rename ref_transaction_* to transaction_*_ref\n> * slight rewording of the additional documentation\n>\n> [1] http://www.spinics.net/lists/git/msg241214.html\n> [2] https://github.com/stefanbeller/git/tree/atomic-push-v1\n>\n> Ronnie Sahlberg (4):\n> receive-pack.c: add protocol support to negotiate atomic-push\n> send-pack.c: add an --atomic-push command line argument\n> receive-pack.c: use a single transaction when atomic-push is\n> negotiated\n> push.c: add an --atomic-push argument\n>\n> Stefan Beller (1):\n> t5543-atomic-push.sh: add basic tests for atomic pushes\n\nThanks. I think I like where it is going.\n"},{"id":"253725","messageId":"1418755747-22506-1-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"xmqqzjaobl0q.fsf@gitster.dls.corp.google.com","subject":"[PATCHv2 1/6] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T18:49:02Z","receivedAt":"2014-12-16T18:49:02Z","isPatch":false,"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 Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--\n builtin/receive-pack.c | 6 +++++-\n send-pack.c | 11 +++++++++++\n send-pack.h | 3 ++-\n 4 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 6d5424c..68ec23d 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 single 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..2a513f4 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,11 @@ 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\tfprintf(stderr, \"Server does not support atomic push.\");\n+\t\treturn -1;\n+\t}\n+\tuse_atomic = atomic_supported && args->atomic;\n \n \tif (status_report)\n \t\tstrbuf_addstr(&cap_buf, \" report-status\");\n@@ -335,6 +344,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.0.31.gad78000.dirty\n"},{"id":"253730","messageId":"1418755747-22506-2-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418755747-22506-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 2/6] send-pack: Invert the return value of ref_update_to_be_sent","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T18:49:03Z","receivedAt":"2014-12-16T18:49:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Having the return value inverted we can have different values for\nthe error codes. This is useful in a later patch when we want to\nknow if we hit the REF_STATUS_REJECT_* case.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n New in the series. For consistency with all the other patches\n it also reads v2.\n\n send-pack.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 2a513f4..1c4ac75 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -190,10 +190,10 @@ 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+static int no_ref_update_to_be_sent(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 1;\n \n \t/* Check for statuses set by set_ref_status_for_push() */\n \tswitch (ref->status) {\n@@ -203,10 +203,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 2;\n \tcase REF_STATUS_UPTODATE:\n-\t\treturn 0;\n+\t\treturn 3;\n \tdefault:\n-\t\treturn 1;\n+\t\treturn 0;\n \t}\n }\n \n@@ -250,7 +251,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 (no_ref_update_to_be_sent(ref, args))\n \t\t\tcontinue;\n \t\tupdate_seen = 1;\n \t\tstrbuf_addf(&cert, \"%s %s %s\\n\",\n@@ -370,7 +371,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 (no_ref_update_to_be_sent(ref, args))\n \t\t\tcontinue;\n \n \t\tif (!ref->deletion)\n@@ -391,7 +392,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 (no_ref_update_to_be_sent(ref, args))\n \t\t\tcontinue;\n \n \t\told_hex = sha1_to_hex(ref->old_sha1);\n-- \n2.2.0.31.gad78000.dirty\n"},{"id":"253727","messageId":"1418755747-22506-3-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418755747-22506-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 3/6] send-pack.c: add --atomic command line argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T18:49:04Z","receivedAt":"2014-12-16T18:49:04Z","isPatch":false,"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 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 \t Junio wrote:\n \t > Hmph.  Is \"atomic push\" so special that it deserves a separate\n \t > parameter?  When we come up with yet another mode of failure, would\n \t > 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 | 36 ++++++++++++++++++++++++++++++++++--\n transport.c | 4 ++++\n 5 files changed, 51 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 1c4ac75..71b1915 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -279,6 +279,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. \"\n+\t \"status: %d\\n\", 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@@ -371,9 +395,17 @@ 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 (no_ref_update_to_be_sent(ref, args))\n+\t\tint reject_reason;\n+\t\tif ((reject_reason = no_ref_update_to_be_sent(ref, args))) {\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\tif (use_atomic && reject_reason == 2)\n+\t\t\t\treturn atomic_push_failure(args,\n+\t\t\t\t\t\t\t remote_refs,\n+\t\t\t\t\t\t\t ref);\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.0.31.gad78000.dirty\n"},{"id":"253729","messageId":"1418755747-22506-4-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418755747-22506-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T18:49:05Z","receivedAt":"2014-12-16T18:49:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nUpdate 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\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nNotes:\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\n builtin/receive-pack.c | 64 ++++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 49 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e76e5d5..0803fd2 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -67,6 +67,8 @@ 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+struct strbuf err = STRBUF_INIT;\n+struct ref_transaction *transaction;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -832,34 +834,32 @@ 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\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 \t\t\treturn \"failed to update ref\";\n \t\t}\n \n-\t\tref_transaction_free(transaction);\n \t\tstrbuf_release(&err);\n \t\treturn NULL; /* good */\n \t}\n@@ -1059,6 +1059,16 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\tif (use_atomic) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"transaction error\";\n+\t\t\treturn;\n+\t\t}\n+\t}\n \tdata.cmds = commands;\n \tdata.si = si;\n \tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n@@ -1086,8 +1096,23 @@ static void execute_commands(struct command *commands,\n \n \t\tif (cmd->skip_update)\n \t\t\tcontinue;\n-\n+\t\tif (!use_atomic) {\n+\t\t\ttransaction = ref_transaction_begin(&err);\n+\t\t\tif (!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 start transaction\";\n+\t\t\t}\n+\t\t}\n \t\tcmd->error_string = update(cmd, si);\n+\t\tif (!use_atomic)\n+\t\t\tif (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+\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@@ -1096,6 +1121,14 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n+\tif (use_atomic) {\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = err.buf;\n+\t\t}\n+\t\tref_transaction_free(transaction);\n+\t}\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@@ -1543,5 +1576,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tsha1_array_clear(&shallow);\n \tsha1_array_clear(&ref);\n \tfree((void *)push_cert_nonce);\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n-- \n2.2.0.31.gad78000.dirty\n"},{"id":"253726","messageId":"1418755747-22506-5-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418755747-22506-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 5/6] push.c: add an --atomic-push argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T18:49:06Z","receivedAt":"2014-12-16T18:49:06Z","isPatch":false,"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 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..3feacc5 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-push] [-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 the an atomic transaction on the server 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..fe0b8cc 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_(\"use a single atomic transaction at the serverside, if available\"),\n+\t\t\tTRANSPORT_ATOMIC_PUSH),\n \t\tOPT_END()\n \t};\n \ndiff --git a/transport.c b/transport.c\nindex c67feee..5b29514 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_ATOMIC_PUSH);\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..25fa1da 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_ATOMIC_PUSH 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.0.31.gad78000.dirty\n"},{"id":"253728","messageId":"1418755747-22506-6-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418755747-22506-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T18:49:07Z","receivedAt":"2014-12-16T18:49:07Z","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 \n---\n\nNotes:\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 | 176 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 176 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..6354fc0\n--- /dev/null\n+++ b/t/t5543-atomic-push.sh\n@@ -0,0 +1,176 @@\n+#!/bin/sh\n+\n+test_description='pushing to a repository using the atomic push option'\n+\n+. ./test-lib.sh\n+\n+D=`pwd`\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 && git config receive.denyCurrentBranch warn\n+\t) &&\n+\t(\n+\t\tcd workbench && git remote add up ../upstream\n+\t)\n+}\n+\n+# refname, expected value, e.g.\n+# test_ref_upstream refs/heads/master HEADS{0}\n+test_ref_upstream () {\n+\ttest \"$#\" == \"2\" # if this fails, we have a bug in this script.\n+\ttest \"$(git -C upstream rev-parse --verify $1)\" == \"$2\"\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_ref_upstream master $(git -C workbench rev-parse --verify 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_ref_upstream master $(git -C workbench rev-parse --verify master) &&\n+\ttest_ref_upstream second $(git -C workbench rev-parse --verify 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_ref_upstream master $(git -C workbench rev-parse --verify master) &&\n+\ttest_ref_upstream second $(git -C workbench rev-parse --verify 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_ref_upstream master $(git -C workbench rev-parse --verify master) &&\n+\ttest_ref_upstream second $(git -C workbench rev-parse --verify 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\t! git push --atomic --all up\n+\t) &&\n+\ttest_ref_upstream master $(git -C workbench rev-parse --verify HEAD@{7}) &&\n+\ttest_ref_upstream second $(git -C workbench rev-parse --verify 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_ref_upstream master $(git -C workbench rev-parse --verify HEAD@{3}) &&\n+\ttest_ref_upstream second $(git -C workbench rev-parse --verify HEAD@{1})\n+'\n+\n+test_expect_failure '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_ref_upstream master $(git -C workbench rev-parse --verify HEAD@{3}) &&\n+\ttest_ref_upstream second $(git -C workbench rev-parse --verify HEAD@{1})\n+'\n+\n+test_done\n-- \n2.2.0.31.gad78000.dirty\n"},{"id":"253733","messageId":"xmqqbnn3bekn.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418755747-22506-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 1/6] receive-pack.c: add protocol support to negotiate atomic-push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T19:05:28Z","receivedAt":"2014-12-16T19:05:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 949cb61..2a513f4 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,11 @@ 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\tfprintf(stderr, \"Server does not support atomic push.\");\n> +\t\treturn -1;\n\nI'd tweak this to\n\n\treturn error(\"server does not support atomic push.\");\n\nto (0) shorten, (1) make sure the message is terminated with LF,\nand (2) match the other error messages in the program.\n\nOther than that looks good.\n\nThanks.\n\n> +\t}\n> +\tuse_atomic = atomic_supported && args->atomic;\n> \n> \tif (status_report)\n> \t\tstrbuf_addstr(&cap_buf, \" report-status\");\n> @@ -335,6 +344,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> diff --git a/send-pack.h b/send-pack.h\n> index 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"},{"id":"253735","messageId":"1418757280-32412-1-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418755747-22506-6-git-send-email-sbeller@google.com","subject":"[PATCH] receive-pack: refuse all commands if one fails in atomic mode","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T19:14:40Z","receivedAt":"2014-12-16T19:14:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch will be incorporated into the right places in v3 of the series.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n I don't want to resend the patch series today to accumulate comments,\n but this makes the last test pass, which is the whole point of the series.\n \n I'll put it into the right places in a reroll.\n\n builtin/receive-pack.c | 13 ++++++++++++-\n t/t5543-atomic-push.sh | 2 +-\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0803fd2..3477f7c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1122,7 +1122,18 @@ static void execute_commands(struct command *commands,\n \t}\n \n \tif (use_atomic) {\n-\t\tif (ref_transaction_commit(transaction, &err)) {\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\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\tif (cmd->error_string)\n+\t\t\t\tbreak;\n+\t\tif (cmd) {\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tif (!cmd->error_string)\n+\t\t\t\t\tcmd->error_string = \"atomic push failure\";\n+\t\t} else if (ref_transaction_commit(transaction, &err)) {\n \t\t\trp_error(\"%s\", err.buf);\n \t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\t\t\tcmd->error_string = err.buf;\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 6354fc0..f0e54d9 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -142,7 +142,7 @@ test_expect_success 'atomic push fails if one tag fails remotely' '\n \ttest_ref_upstream second $(git -C workbench rev-parse --verify HEAD@{1})\n '\n \n-test_expect_failure 'atomic push obeys update hook preventing a branch to be pushed' '\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-- \n2.2.0.31.gad78000.dirty\n"},{"id":"253736","messageId":"xmqq7fxrbe4w.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418755747-22506-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 2/6] send-pack: Invert the return value of ref_update_to_be_sent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T19:14:55Z","receivedAt":"2014-12-16T19:14:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Having the return value inverted we can have different values for\n> the error codes. This is useful in a later patch when we want to\n> know if we hit the REF_STATUS_REJECT_* case.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\n> New in the series. For consistency with all the other patches\n> it also reads v2.\n\nSorry, but ECANNOTPARSE especially \"it also read v2\" part.\n\n> send-pack.c | 15 ++++++++-------\n> 1 file changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index 2a513f4..1c4ac75 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -190,10 +190,10 @@ 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> +static int no_ref_update_to_be_sent(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 1;\n> \n> \t/* Check for statuses set by set_ref_status_for_push() */\n> \tswitch (ref->status) {\n> @@ -203,10 +203,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 2;\n> \tcase REF_STATUS_UPTODATE:\n> -\t\treturn 0;\n> +\t\treturn 3;\n> \tdefault:\n> -\t\treturn 1;\n> +\t\treturn 0;\n\nHmmm. It used to be \"will we send this one?\" It is fine if the\nfunction wants to differenciate various kinds of error, but\n\n (1) unexplained 1, 2 and 3 looks cryptic and unmaintainable;\n\n (2) no_ prefix in the function name is usually a bad idea, because\n it easily invites \"if (!no_foo(...))\" double negation; and\n\n (3) unless there is a good reason to do otherwise, a function that\n returns 0 on success should signal an error with a negative\n return.\n\n\"static int check_to_send_update()\" or something, perhaps?\n\n\tif (check_to_send_update() < 0)\n \t/* skip as this is an error */\n continue;\n\ndoes not look too bad.\n\n> }\n> \n> @@ -250,7 +251,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 (no_ref_update_to_be_sent(ref, args))\n> \t\t\tcontinue;\n> \t\tupdate_seen = 1;\n> \t\tstrbuf_addf(&cert, \"%s %s %s\\n\",\n> @@ -370,7 +371,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 (no_ref_update_to_be_sent(ref, args))\n> \t\t\tcontinue;\n> \n> \t\tif (!ref->deletion)\n> @@ -391,7 +392,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 (no_ref_update_to_be_sent(ref, args))\n> \t\t\tcontinue;\n> \n> \t\told_hex = sha1_to_hex(ref->old_sha1);\n"},{"id":"253737","messageId":"CAPig+cRkPZ8-MQyDs_S-3UXy2JUP6nLzjCE-pmpOa4Z4gyQKEg@mail.gmail.com","threadId":"38179","inReplyTo":"1418755747-22506-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-16T19:29:05Z","receivedAt":"2014-12-16T19:29:05Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 16, 2014 at 1:49 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n>\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>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index e76e5d5..0803fd2 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -67,6 +67,8 @@ 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> +struct strbuf err = STRBUF_INIT;\n> +struct ref_transaction *transaction;\n\nShould these be static?\n\n> static enum deny_action parse_deny_action(const char *var, const char *value)\n> {\n> @@ -1059,6 +1059,16 @@ static void execute_commands(struct command *commands,\n> return;\n> }\n>\n> + if (use_atomic) {\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + cmd->error_string = \"transaction error\";\n> + return;\n> + }\n> + }\n\nNot seen in this diff, but just below this point, the pre-receive hook\nis invoked. If it \"declines\", then execute_commands() returns without\nreleasing the transaction which was begun here. Is that correct\nbehavior?\n\nFor robustness, it might also be sane to release the 'err' strbuf at\nthis early return (though the current code does not strictly leak it).\n\n> data.cmds = commands;\n> data.si = si;\n> if (check_everything_connected(iterate_receive_command_list, 0, &data))\n> @@ -1086,8 +1096,23 @@ static void execute_commands(struct command *commands,\n>\n> if (cmd->skip_update)\n> continue;\n> -\n> + if (!use_atomic) {\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + cmd->error_string = \"failed to start transaction\";\n\nHere, you assign cmd->error_string...\n\n> + }\n> + }\n> cmd->error_string = update(cmd, si);\n\nand then immediately overwrite it here. Is it correct to invoke\nupdate() when ref_transaction_begin() has failed? Seems fishy.\n\n> + if (!use_atomic)\n> + if (ref_transaction_commit(transaction, &err)) {\n> + ref_transaction_free(transaction);\n\nShouldn't you be freeing the transaction outside of this conditional\nrather than only in case of failure?\n\n> + rp_error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + cmd->error_string = \"failed to update ref\";\n> + }\n> +\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> @@ -1096,6 +1121,14 @@ static void execute_commands(struct command *commands,\n> }\n> }\n>\n> + if (use_atomic) {\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + cmd->error_string = err.buf;\n> + }\n> + ref_transaction_free(transaction);\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> @@ -1543,5 +1576,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> sha1_array_clear(&shallow);\n> sha1_array_clear(&ref);\n> free((void *)push_cert_nonce);\n> + strbuf_release(&err);\n> return 0;\n> }\n> --\n> 2.2.0.31.gad78000.dirty\n"},{"id":"253738","messageId":"xmqq1tnzbddu.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418755747-22506-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 3/6] send-pack.c: add --atomic command line argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T19:31:09Z","receivedAt":"2014-12-16T19:31:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\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> \t Junio wrote:\n> \t > Hmph. Is \"atomic push\" so special that it deserves a separate\n> \t > parameter? When we come up with yet another mode of failure, would\n> \t > we add another parameter to the callers to this function?\n\nI suspect that you completely mis-read me.\n\nIf you add an extra arg that is *specifically* for atomic push, like\nthis:\n\n -static int update_to_send(...);\n +static int update_to_send(..., int *atomic_push_failed);\n \nwhat signal does it send to the next person who may want to add\na new \"nuclear push\" option? Should the patch look like\n\n -static int update_to_send(..., int *atomic_push_failed);\n +static int update_to_send(..., int *atomic_push_failed, int *nuclear_push_failed);\n\nby adding yet another separate variable for error reporting?\n\nI would have understood if the new variable was named something like\n\"failure_reason\", which may be set to PUSH_FAILURE_ATOMIC when an\natomic push failure was detected. Making the helper return the\nreason why the push failed would be another way, like you did in the\n2/6 patch in this round.\n\nEither way, the next person would know to add a code to do his\n\"nuclear push\" and set the reason to PUSH_FAILURE_NUCLEAR when it\nfails, instead of piling yet another error reporting variable that\nis specific to the feature.\n\nThis is all about code maintainability, which is very different from\n\"who cares about error reporting\".\n\n> diff --git a/remote.h b/remote.h\n> index 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> ...\n> \tfor (ref = remote_refs; ref; ref = ref->next) {\n> -\t\tif (no_ref_update_to_be_sent(ref, args))\n> +\t\tint reject_reason;\n> +\t\tif ((reject_reason = no_ref_update_to_be_sent(ref, args))) {\n\nWe avoid assignment inside a conditional.\n\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\tif (use_atomic && reject_reason == 2)\n> +\t\t\t\treturn atomic_push_failure(args,\n> +\t\t\t\t\t\t\t remote_refs,\n> +\t\t\t\t\t\t\t ref);\n> \t\t\tcontinue;\n\nPerhaps\n\n\t\tswitch (check_to_send_update(...)) {\n case 0: /* no error */\n \tbreak;\n\t\tcase -REF_STATUS_ATOMIC_PUSH_FAILED: \n \treturn atomic_push_failure(args, remote_refs, ref);\n \tbreak;\n\t\tdefault:\n\t\t\tcontinue;\n\t\t}\n\nor something?\n"},{"id":"253739","messageId":"CAPig+cR5x=GsZ6Gg_i==o0WKgqU1PP9uQ6QAgNkF8SBOvRCJ5g@mail.gmail.com","threadId":"38179","inReplyTo":"1418755747-22506-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 5/6] push.c: add an --atomic-push argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-16T19:33:20Z","receivedAt":"2014-12-16T19:33:20Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 16, 2014 at 1:49 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n>\n> Add a command line argument to the git push command to request atomic\n> pushes.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\n> Changes v1 -> v2\n> It's --atomic now! (dropping the -push)\n>\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index 21b3f29..3feacc5 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-push] [-n | --dry-run] [--receive-pack=]\n\ns/atomic-push/atomic/\n\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 the an atomic transaction on the server side if available.\n\ns/the an/an/\n\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> --receive-pack=::\n> --exec=::\n> Path to the 'git-receive-pack' program on the remote\n> diff --git a/builtin/push.c b/builtin/push.c\n> index a076b19..fe0b8cc 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> OPT_BIT(0, \"follow-tags\", &flags, N_(\"push missing but relevant tags\"),\n> TRANSPORT_PUSH_FOLLOW_TAGS),\n> OPT_BIT(0, \"signed\", &flags, N_(\"GPG sign the push\"), TRANSPORT_PUSH_CERT),\n> + OPT_BIT(0, \"atomic\", &flags, N_(\"use a single atomic transaction at the serverside, if available\"),\n\n\"single atomic\" sounds awfully redundant.\n\n\"serverside\" is odd. Perhaps \"server side\" or merely \"remote\" or \"remote side\".\n\n> + TRANSPORT_ATOMIC_PUSH),\n> OPT_END()\n> };\n>\n> diff --git a/transport.c b/transport.c\n> index c67feee..5b29514 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> args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n> args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n> args.push_cert = !!(flags & TRANSPORT_PUSH_CERT);\n> + args.atomic = !!(flags & TRANSPORT_ATOMIC_PUSH);\n> args.url = transport->url;\n>\n> ret = send_pack(&args, data->fd, data->conn, remote_refs,\n> diff --git a/transport.h b/transport.h\n> index 3e0091e..25fa1da 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_ATOMIC_PUSH 4096\n\nFor consistency with existing names, should this be TRANSPORT_PUSH_ATOMIC?\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> --\n> 2.2.0.31.gad78000.dirty\n"},{"id":"253741","messageId":"xmqqwq5r9yli.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418755747-22506-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T19:35:53Z","receivedAt":"2014-12-16T19:35:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\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\nI am obviously biased, but I find that the new code structure and\nseparation of responsibility between update() and execute()\nfunctions a lot clearer than the previous one.\n\nThanks.\n\n> builtin/receive-pack.c | 64 ++++++++++++++++++++++++++++++++++++++------------\n> 1 file changed, 49 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index e76e5d5..0803fd2 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -67,6 +67,8 @@ 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> +struct strbuf err = STRBUF_INIT;\n> +struct ref_transaction *transaction;\n> \n> static enum deny_action parse_deny_action(const char *var, const char *value)\n> {\n> @@ -832,34 +834,32 @@ 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\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> \t\t\treturn \"failed to update ref\";\n> \t\t}\n> \n> -\t\tref_transaction_free(transaction);\n> \t\tstrbuf_release(&err);\n> \t\treturn NULL; /* good */\n> \t}\n> @@ -1059,6 +1059,16 @@ static void execute_commands(struct command *commands,\n> \t\treturn;\n> \t}\n> \n> +\tif (use_atomic) {\n> +\t\ttransaction = ref_transaction_begin(&err);\n> +\t\tif (!transaction) {\n> +\t\t\terror(\"%s\", err.buf);\n> +\t\t\tstrbuf_release(&err);\n> +\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> +\t\t\t\tcmd->error_string = \"transaction error\";\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> \tdata.cmds = commands;\n> \tdata.si = si;\n> \tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n> @@ -1086,8 +1096,23 @@ static void execute_commands(struct command *commands,\n> \n> \t\tif (cmd->skip_update)\n> \t\t\tcontinue;\n> -\n> +\t\tif (!use_atomic) {\n> +\t\t\ttransaction = ref_transaction_begin(&err);\n> +\t\t\tif (!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 start transaction\";\n> +\t\t\t}\n> +\t\t}\n> \t\tcmd->error_string = update(cmd, si);\n> +\t\tif (!use_atomic)\n> +\t\t\tif (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> +\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> @@ -1096,6 +1121,14 @@ static void execute_commands(struct command *commands,\n> \t\t}\n> \t}\n> \n> +\tif (use_atomic) {\n> +\t\tif (ref_transaction_commit(transaction, &err)) {\n> +\t\t\trp_error(\"%s\", err.buf);\n> +\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> +\t\t\t\tcmd->error_string = err.buf;\n> +\t\t}\n> +\t\tref_transaction_free(transaction);\n> +\t}\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> @@ -1543,5 +1576,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> \tsha1_array_clear(&shallow);\n> \tsha1_array_clear(&ref);\n> \tfree((void *)push_cert_nonce);\n> +\tstrbuf_release(&err);\n> \treturn 0;\n> }\n"},{"id":"253742","messageId":"xmqqsigf9ykv.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418755747-22506-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 5/6] push.c: add an --atomic-push argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T19:36:16Z","receivedAt":"2014-12-16T19:36:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> From: Ronnie Sahlberg \n>\n> Add a command line argument to the git push command to request atomic\n> pushes.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\n> Changes v1 -> v2\n> \tIt's --atomic now! (dropping the -push)\n\nAlso from the subject line ;-)\n\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>\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index 21b3f29..3feacc5 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-push] [-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 the an atomic transaction on the server 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\n> diff --git a/builtin/push.c b/builtin/push.c\n> index a076b19..fe0b8cc 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_(\"use a single atomic transaction at the serverside, if available\"),\n> +\t\t\tTRANSPORT_ATOMIC_PUSH),\n> \t\tOPT_END()\n> \t};\n> \n> diff --git a/transport.c b/transport.c\n> index c67feee..5b29514 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_ATOMIC_PUSH);\n> \targs.url = transport->url;\n> \n> \tret = send_pack(&args, data->fd, data->conn, remote_refs,\n> diff --git a/transport.h b/transport.h\n> index 3e0091e..25fa1da 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_ATOMIC_PUSH 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"},{"id":"253743","messageId":"CAPig+cQyrTT6Ejrg_e2HveJGrn6qCnRWPBuJZKY91b+SzO6vPw@mail.gmail.com","threadId":"38179","inReplyTo":"1418755747-22506-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-16T19:37:23Z","receivedAt":"2014-12-16T19:37:23Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 16, 2014 at 1:49 PM, Stefan Beller 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> option prevents any change to be pushed if just one ref cannot\n> be updated.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\n> new file mode 100755\n> index 0000000..6354fc0\n> --- /dev/null\n> +++ b/t/t5543-atomic-push.sh\n> @@ -0,0 +1,176 @@\n> +#!/bin/sh\n> +\n> +test_description='pushing to a repository using the atomic push option'\n> +\n> +. ./test-lib.sh\n> +\n> +D=`pwd`\n\nJunio already mentioned this previously, but this should be $(pwd)\nrather than `pwd`.\n\nHowever, more importantly, why is this variable declared at all since\nit is never used by the script?\n\n> +mk_repo_pair () {\n> + rm -rf workbench upstream &&\n> + test_create_repo upstream &&\n> + test_create_repo workbench &&\n> + (\n> + cd upstream && git config receive.denyCurrentBranch warn\n\nThis would be slightly easier to read if split over two lines.\n\n> + ) &&\n> + (\n> + cd workbench && git remote add up ../upstream\n\nDitto.\n\n> + )\n> +}\n> +\n"},{"id":"253745","messageId":"xmqqoar39y3l.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418755747-22506-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T19:46:38Z","receivedAt":"2014-12-16T19:46:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +test_description='pushing to a repository using the atomic push option'\n> +\n> +. ./test-lib.sh\n> +\n> +D=`pwd`\n\n$(pwd)?\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 && git config receive.denyCurrentBranch warn\n> +\t) &&\n\nI was wondering how you would do this part after suggesting use of\ntest_create_repo, knowing very well that one of them was a bare one\n;-).\n\nWe might want to extend test_create_repo to allow creating a bare\nrepository, but this is also OK.\n\n> +\t(\n> +\t\tcd workbench && git remote add up ../upstream\n> +\t)\n> +}\n\n> +# refname, expected value, e.g.\n> +# test_ref_upstream refs/heads/master HEADS{0}\n> +test_ref_upstream () {\n> +\ttest \"$#\" == \"2\" # if this fails, we have a bug in this script.\n\nThis is not C; \"test $# = 2\" (notice a single equal sign). And you\ndo not need dq-pair around these.\n\n> +\ttest \"$(git -C upstream rev-parse --verify $1)\" == \"$2\"\n> +}\n\nSeeing that all callers of test_ref_upstream computes $2 as\n\n\tgit -C workbench rev-parse --verify \n\nI have a feeling that\n\n> +\ttest_ref_upstream second second\n\nwould be easier for them to write than\n\n> +\ttest_ref_upstream second $(git -C workbench rev-parse --verify second)\n\nThat is\n\n# refname in upstream and expected value from workbench\n# E.g. \"test_ref_upstream master HEAD\" makes sure that HEAD in\n# workbench matches the master branch in upstream repository.\ntest_ref_upstream () {\n\ttest $# = 2 &&\n test \"$(git -C upstream rev-parse --verify \"$1\")\" == \\\n\t\t\"$(git -C workbench rev-parse --verify \"$2\")\"\n}\n\nor something. We may however want to do the usual\n\n\ttest $# = 2 &&\n\tgit -C upstream rev-parse --verify \"$1\" >expect &&\n\tgit -C workbench rev-parse --verify \"$2\" >actual &&\n test_cmp expect actual\n\nthough.\n"},{"id":"253746","messageId":"CAGZ79kZnGKNTpRZndgG_CjAC9gqafGXoq8rFOWuHZkaqY_LK7w@mail.gmail.com","threadId":"38179","inReplyTo":"xmqqoar39y3l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T19:57:04Z","receivedAt":"2014-12-16T19:57:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 16, 2014 at 11:46 AM, Junio C Hamano wrote:\n\n>\n> Seeing that all callers of test_ref_upstream computes $2 as\n>\n> git -C workbench rev-parse --verify \n>\n\nOnly in the first tests, where this should be the case after push.\nIn the failure tests, we go with HEAD@{N} which needs to be computed\ninside the workbench repo.\n\nAlternatively we could check if the reflog of upstream has a certain\nnumber of entries\nwhich would indicate the push was not recorded (i.e. not performed?)\n\nI think we should keep it similar to this one.\n"},{"id":"253747","messageId":"xmqqk31r9w2w.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418755747-22506-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T20:30:15Z","receivedAt":"2014-12-16T20:30:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\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\nIt is tricky to arrange a test to fail a fast-forward check on the\nreceiver side, because the local test is done by reading from the\nother side, not relying on your remote tracking branches. The usual\nflow is:\n\n pusher says to the receiver \"I want to push\"\n\n receiver says to the pusher \"Here are the tips of my refs\"\n\n pusher thinks: \"Ah, I was about to update their master branch\n\t\twith this commit, but what they have is (1) not\n\t\tknown to me so by definition I cannot fast-forward,\n\t\tor (2) known to me and I can definitely tell I am\n\t\tnot fast-forwarding it, so I'd locally fail this\n\t\tpush\".\n\nYou need to invent a way to successfully race with an ongoing push.\nAfter receiver gives the tips of its refs (all of which are\nancestors of what is going to be pushed) but before the pusher\nfinishes the \"thinking\" above, you would somehow update the refs at\nthe receiver so that the push will not fast-forward. \n\nSuch a raced flow would look like:\n\n pusher says to the receiver \"I want to push\"\n\n receiver says to the pusher \"Here are the tips of my refs\"\n\n pusher thinks: \"OK, everything I'll send will fast-forward\"\n pusher thinks: \"Let's start generating a packfile\"\n\n you intervene and update receiver's 'master' at this point.\n\n pusher send a pack and tells the receiver \"I want to update your\n\t\tmaster from OLD to NEW\".\n\n receiver thinks: \"Huh, that OLD is not where my 'master' is\"\n\n recevier says to the pusher \"No fast-forward\".\n\nBut I do not think it is practical to run such a test.\n\nRejecting on the receiver's end using pre-receive or update hook\nshould be testable and should be tested.\n\nThanks.\n"},{"id":"253748","messageId":"CAPig+cQu48Woi-TbbSe1=hsnh1EgGzV781gV_tZQwjVFoHy2Bg@mail.gmail.com","threadId":"38179","inReplyTo":"CAPig+cRkPZ8-MQyDs_S-3UXy2JUP6nLzjCE-pmpOa4Z4gyQKEg@mail.gmail.com","subject":"Re: [PATCHv2 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-16T20:30:38Z","receivedAt":"2014-12-16T20:30:38Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 16, 2014 at 2:29 PM, Eric Sunshine wrote:\n> On Tue, Dec 16, 2014 at 1:49 PM, Stefan Beller wrote:\n>> ---\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index e76e5d5..0803fd2 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1059,6 +1059,16 @@ static void execute_commands(struct command *commands,\n>> return;\n>> }\n>>\n>> + if (use_atomic) {\n>> + transaction = ref_transaction_begin(&err);\n>> + if (!transaction) {\n>> + error(\"%s\", err.buf);\n>> + strbuf_release(&err);\n>> + for (cmd = commands; cmd; cmd = cmd->next)\n>> + cmd->error_string = \"transaction error\";\n>> + return;\n>> + }\n>> + }\n>\n> Not seen in this diff, but just below this point, the pre-receive hook\n> is invoked. If it \"declines\", then execute_commands() returns without\n> releasing the transaction which was begun here. Is that correct\n> behavior?\n>\n> For robustness, it might also be sane to release the 'err' strbuf at\n> this early return (though the current code does not strictly leak it).\n\nTo clarify: By \"this early return\", I mean the early return taken when\npre-receive hook declines.\n"},{"id":"253749","messageId":"xmqqfvcf9vzn.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418757280-32412-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] receive-pack: refuse all commands if one fails in atomic mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T20:32:12Z","receivedAt":"2014-12-16T20:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This patch will be incorporated into the right places in v3 of the series.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\n> I don't want to resend the patch series today to accumulate comments,\n> but this makes the last test pass, which is the whole point of the series.\n> \n> I'll put it into the right places in a reroll.\n>\n> builtin/receive-pack.c | 13 ++++++++++++-\n> t/t5543-atomic-push.sh | 2 +-\n> 2 files changed, 13 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 0803fd2..3477f7c 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1122,7 +1122,18 @@ static void execute_commands(struct command *commands,\n> \t}\n> \n> \tif (use_atomic) {\n> -\t\tif (ref_transaction_commit(transaction, &err)) {\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\n\t/*\n * The first line of our multi-line comment\n * has only opening slash-asterisk and nothing else.\n * The last line has asterisk-slash and nothing else.\n */\n\n> +\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> +\t\t\tif (cmd->error_string)\n> +\t\t\t\tbreak;\n> +\t\tif (cmd) {\n> +\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> +\t\t\t\tif (!cmd->error_string)\n> +\t\t\t\t\tcmd->error_string = \"atomic push failure\";\n> +\t\t} else if (ref_transaction_commit(transaction, &err)) {\n> \t\t\trp_error(\"%s\", err.buf);\n> \t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> \t\t\t\tcmd->error_string = err.buf;\n\nMakes sense.\n\n> diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\n> index 6354fc0..f0e54d9 100755\n> --- a/t/t5543-atomic-push.sh\n> +++ b/t/t5543-atomic-push.sh\n> @@ -142,7 +142,7 @@ test_expect_success 'atomic push fails if one tag fails remotely' '\n> \ttest_ref_upstream second $(git -C workbench rev-parse --verify HEAD@{1})\n> '\n> \n> -test_expect_failure 'atomic push obeys update hook preventing a branch to be pushed' '\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"},{"id":"253750","messageId":"CAGZ79ka5o1fx_-rQfbrSngJOeD4wTSQm=ZfeeU-LAkxREKA9ew@mail.gmail.com","threadId":"38179","inReplyTo":"xmqqk31r9w2w.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T20:36:15Z","receivedAt":"2014-12-16T20:36:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 16, 2014 at 12:30 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\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>\n> It is tricky to arrange a test to fail a fast-forward check on the\n> receiver side, because the local test is done by reading from the\n> other side, not relying on your remote tracking branches. The usual\n> flow is:\n>\n> pusher says to the receiver \"I want to push\"\n>\n> receiver says to the pusher \"Here are the tips of my refs\"\n>\n> pusher thinks: \"Ah, I was about to update their master branch\n> with this commit, but what they have is (1) not\n> known to me so by definition I cannot fast-forward,\n> or (2) known to me and I can definitely tell I am\n> not fast-forwarding it, so I'd locally fail this\n> push\".\n>\n> You need to invent a way to successfully race with an ongoing push.\n> After receiver gives the tips of its refs (all of which are\n> ancestors of what is going to be pushed) but before the pusher\n> finishes the \"thinking\" above, you would somehow update the refs at\n> the receiver so that the push will not fast-forward.\n>\n> Such a raced flow would look like:\n>\n> pusher says to the receiver \"I want to push\"\n>\n> receiver says to the pusher \"Here are the tips of my refs\"\n>\n> pusher thinks: \"OK, everything I'll send will fast-forward\"\n> pusher thinks: \"Let's start generating a packfile\"\n>\n> you intervene and update receiver's 'master' at this point.\n>\n> pusher send a pack and tells the receiver \"I want to update your\n> master from OLD to NEW\".\n>\n> receiver thinks: \"Huh, that OLD is not where my 'master' is\"\n>\n> recevier says to the pusher \"No fast-forward\".\n>\n> But I do not think it is practical to run such a test.\n>\n> Rejecting on the receiver's end using pre-receive or update hook\n> should be testable and should be tested.\n>\n> Thanks.\n>\n\nYes, that was my understanding as well. I just messed up mentally.\n\nNow that the update hook test is in place, we do have tests from the local\nand the remote side rejecting, so it should be fine now.\n"},{"id":"253751","messageId":"xmqqbnn39vhj.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"CAPig+cR5x=GsZ6Gg_i==o0WKgqU1PP9uQ6QAgNkF8SBOvRCJ5g@mail.gmail.com","subject":"Re: [PATCHv2 5/6] push.c: add an --atomic-push argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T20:43:04Z","receivedAt":"2014-12-16T20:43:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> +--atomic::\n>> + Use the an atomic transaction on the server side if available.\n>\n> s/the an/an/\n> ...\n>> + OPT_BIT(0, \"atomic\", &flags, N_(\"use a single atomic transaction at the serverside, if available\"),\n>\n> \"single atomic\" sounds awfully redundant.\n>\n> \"serverside\" is odd. Perhaps \"server side\" or merely \"remote\" or \"remote side\".\n> ...\n>> diff --git a/transport.h b/transport.h\n>> index 3e0091e..25fa1da 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_ATOMIC_PUSH 4096\n>\n> For consistency with existing names, should this be TRANSPORT_PUSH_ATOMIC?\n\nAs always, thanks for a careful reading. I missed everything you\npointed out (I agree with you).\n"},{"id":"253752","messageId":"xmqq7fxr9vc7.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"CAGZ79kZnGKNTpRZndgG_CjAC9gqafGXoq8rFOWuHZkaqY_LK7w@mail.gmail.com","subject":"Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-16T20:46:16Z","receivedAt":"2014-12-16T20:46:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Dec 16, 2014 at 11:46 AM, Junio C Hamano wrote:\n>\n>>\n>> Seeing that all callers of test_ref_upstream computes $2 as\n>>\n>> git -C workbench rev-parse --verify \n>>\n>\n> Only in the first tests, where this should be the case after push.\n> In the failure tests, we go with HEAD@{N} which needs to be computed\n> inside the workbench repo.\n\nAren't we saying the same thing? The suggested alternative is to do\nthe \"git -C workbench rev-parse\" bit inside the helper.\n"},{"id":"253753","messageId":"CAGZ79kb6JqJRJYhx=ypo+3WJAbu1uy_LSCPc+a+3=wW5EKWhZw@mail.gmail.com","threadId":"38179","inReplyTo":"xmqq7fxr9vc7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-16T20:51:47Z","receivedAt":"2014-12-16T20:51:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 16, 2014 at 12:46 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Tue, Dec 16, 2014 at 11:46 AM, Junio C Hamano wrote:\n>>\n>>>\n>>> Seeing that all callers of test_ref_upstream computes $2 as\n>>>\n>>> git -C workbench rev-parse --verify \n>>>\n>>\n>> Only in the first tests, where this should be the case after push.\n>> In the failure tests, we go with HEAD@{N} which needs to be computed\n>> inside the workbench repo.\n>\n> Aren't we saying the same thing? The suggested alternative is to do\n> the \"git -C workbench rev-parse\" bit inside the helper.\n\nNow I am saying the same. (I finally understood what you were saying)\n\nWill incorporate in a reroll.\n"},{"id":"253774","messageId":"1418841177-12152-1-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"xmqqzjaobl0q.fsf@gitster.dls.corp.google.com","subject":"[PATCHv3 0/6] atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T18:32:51Z","receivedAt":"2014-12-17T18:32:51Z","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[2].\n\nChanges v3->v3 are annotated in each patch.\n\n[2] https://github.com/stefanbeller/git/tree/atomic-push-v3\n\nRonnie Sahlberg (4):\n receive-pack.c: add protocol support to negotiate atomic-push\n send-pack.c: add --atomic command line argument\n receive-pack.c: use a single transaction when atomic-push is\n negotiated\n push.c: add an --atomic argument\n\nStefan Beller (2):\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 | 2 +\n builtin/receive-pack.c | 89 +++++++++--\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, 349 insertions(+), 31 deletions(-)\n create mode 100755 t/t5543-atomic-push.sh\n\n-- \n2.2.0.31.gad78000.dirty\n"},{"id":"253770","messageId":"1418841177-12152-2-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418841177-12152-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 1/6] receive-pack.c: add protocol support to negotiate atomic","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T18:32:52Z","receivedAt":"2014-12-17T18:32:52Z","isPatch":false,"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 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..68ec23d 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 single 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..e2bdd89 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\treturn error(\"server does not support atomic push.\");\n+\t}\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.0.31.gad78000.dirty\n"},{"id":"253776","messageId":"1418841177-12152-3-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418841177-12152-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 2/6] send-pack: Rename ref_update_to_be_sent to check_to_send_update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T18:32:53Z","receivedAt":"2014-12-17T18:32:53Z","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 \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 send-pack.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex e2bdd89..77e4201 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.0.31.gad78000.dirty\n"},{"id":"253773","messageId":"1418841177-12152-4-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418841177-12152-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 3/6] send-pack.c: add --atomic command line argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T18:32:54Z","receivedAt":"2014-12-17T18:32:54Z","isPatch":false,"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 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 77e4201..a696d5c 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. \"\n+\t \"status: %d\\n\", 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.0.31.gad78000.dirty\n"},{"id":"253771","messageId":"1418841177-12152-5-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418841177-12152-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T18:32:55Z","receivedAt":"2014-12-17T18:32:55Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nUpdate 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\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nNotes:\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 builtin/receive-pack.c | 83 +++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 68 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e76e5d5..c942a3b 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@@ -1052,6 +1055,7 @@ static void execute_commands(struct command *commands,\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n \tstruct iterate_data data;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (unpacker_error) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next)\n@@ -1059,6 +1063,16 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\tif (use_atomic) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"transaction error\";\n+\t\t\treturn;\n+\t\t}\n+\t}\n \tdata.cmds = commands;\n \tdata.si = si;\n \tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n@@ -1086,8 +1100,25 @@ static void execute_commands(struct command *commands,\n \n \t\tif (cmd->skip_update)\n \t\t\tcontinue;\n-\n+\t\tif (!use_atomic) {\n+\t\t\ttransaction = ref_transaction_begin(&err);\n+\t\t\tif (!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 start transaction\";\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t}\n \t\tcmd->error_string = update(cmd, si);\n+\t\tif (!use_atomic)\n+\t\t\tif (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\treturn;\n+\t\t\t}\n+\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@@ -1096,10 +1127,32 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n+\tif (use_atomic) {\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+\t\tif (cmd) {\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tif (!cmd->error_string)\n+\t\t\t\t\tcmd->error_string = \"atomic push failure\";\n+\t\t} else if (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = err.buf;\n+\t\t}\n+\t\tref_transaction_free(transaction);\n+\t}\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.0.31.gad78000.dirty\n"},{"id":"253772","messageId":"1418841177-12152-6-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418841177-12152-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 5/6] push.c: add an --atomic argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T18:32:56Z","receivedAt":"2014-12-17T18:32:56Z","isPatch":false,"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 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..5731a0d 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_(\"use an atomic transaction remote\"),\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.0.31.gad78000.dirty\n"},{"id":"253775","messageId":"1418841177-12152-7-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"1418841177-12152-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T18:32:57Z","receivedAt":"2014-12-17T18:32:57Z","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 \n---\n\nNotes:\n Changes in v3:\n * test_refs for checking \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.0.31.gad78000.dirty\n"},{"id":"253784","messageId":"xmqqioh999cv.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418841177-12152-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 2/6] send-pack: Rename ref_update_to_be_sent to check_to_send_update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-17T22:53:20Z","receivedAt":"2014-12-17T22:53:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This renames ref_update_to_be_sent to check_to_send_update and inverts\n> the meaning of the return value. Having the return value inverted we\n> can have different values for the error codes. This is useful in a\n> later patch when we want to know if we hit the CHECK_REF_STATUS_REJECTED\n> case.\n\nMakes sense; I would have defined these constants negative, but that\ncan be cleaned up if somebody cares deeply enough later.\n\nWill queue. Thanks.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\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>\n> diff --git a/send-pack.c b/send-pack.c\n> index e2bdd89..77e4201 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"},{"id":"253785","messageId":"xmqqegrx98dy.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418841177-12152-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 3/6] send-pack.c: add --atomic command line argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-17T23:14:17Z","receivedAt":"2014-12-17T23:14:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\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\nLooks sensible. Thanks.\n"},{"id":"253787","messageId":"xmqqa92l97u1.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"1418841177-12152-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-17T23:26:14Z","receivedAt":"2014-12-17T23:26:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> @@ -1086,8 +1100,25 @@ static void execute_commands(struct command *commands,\n> \n> \t\tif (cmd->skip_update)\n> \t\t\tcontinue;\n> -\n> +\t\tif (!use_atomic) {\n> +\t\t\ttransaction = ref_transaction_begin(&err);\n> +\t\t\tif (!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 start transaction\";\n> +\t\t\t\treturn;\n> +\t\t\t}\n> +\t\t}\n> \t\tcmd->error_string = update(cmd, si);\n> +\t\tif (!use_atomic)\n> +\t\t\tif (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\treturn;\n> +\t\t\t}\n\nHmm, should the code even attempt to commit if update() returned a\nnon NULL, signaling a failure?\n\nOr would we want to do this instead?\n\n\tif (cmd->error_string)\n \tgoto transaction_abort;\n\telse if (!use_atomic) {\n\t\tif (ref_transaction_commit(...)) {\n\t\t\t...\n cmd->error_string = \"...\";\n return;\n \t}\n\t}\n\nand then ...\n\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> @@ -1096,10 +1127,32 @@ static void execute_commands(struct command *commands,\n> \t\t}\n> \t}\n> \n> +\tif (use_atomic) {\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> +\t\tif (cmd) {\n> +\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> +\t\t\t\tif (!cmd->error_string)\n> +\t\t\t\t\tcmd->error_string = \"atomic push failure\";\n> +\t\t} else if (ref_transaction_commit(transaction, &err)) {\n> +\t\t\trp_error(\"%s\", err.buf);\n> +\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> +\t\t\t\tcmd->error_string = err.buf;\n> +\t\t}\n\n... have the label to jump to here:\n\n\ttransaction_abort:\n\n> +\t\tref_transaction_free(transaction);\n\nI was confused by the fact that you did not have any call to\ntransaction-abort, until I realized that there is no such API\nfunction and ref_transaction_free() serves that \"don't commit,\nroll it back\" purpose.\n\n> +\t}\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"},{"id":"253790","messageId":"CAGZ79kZCTb5Uj34Kj-qnQEWmD+4=3U8LyeHxQWmLg-ozJr7Azg@mail.gmail.com","threadId":"38179","inReplyTo":"xmqqa92l97u1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-17T23:58:24Z","receivedAt":"2014-12-17T23:58:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 17, 2014 at 3:26 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> @@ -1086,8 +1100,25 @@ static void execute_commands(struct command *commands,\n>>\n>> if (cmd->skip_update)\n>> continue;\n>> -\n>> + if (!use_atomic) {\n>> + transaction = ref_transaction_begin(&err);\n>> + if (!transaction) {\n>> + rp_error(\"%s\", err.buf);\n>> + strbuf_release(&err);\n>> + cmd->error_string = \"failed to start transaction\";\n>> + return;\n>> + }\n>> + }\n>> cmd->error_string = update(cmd, si);\n>> + if (!use_atomic)\n>> + if (ref_transaction_commit(transaction, &err)) {\n>> + ref_transaction_free(transaction);\n>> + rp_error(\"%s\", err.buf);\n>> + strbuf_release(&err);\n>> + cmd->error_string = \"failed to update ref\";\n>> + return;\n>> + }\n>\n> Hmm, should the code even attempt to commit if update() returned a\n> non NULL, signaling a failure?\n>\n> Or would we want to do this instead?\n\nThis would change the current behavior. In the case of !atomic we want\nto consider all commands and not stop early.\n\nSo maybe more\nif (!cmd->error_string) {\n if (!use_atomic\n && ref_transaction_commit(...)) {\n ...\n }\n} else {\n if (use_atomic)\n goto check_atomic_commit;\n}\n\nand the check_atomic_commit label is replacing the loop to check:\n- for (cmd = commands; cmd; cmd = cmd->next)\n- if (cmd->error_string)\n- break;\n+ check_atomic_commit:\n"},{"id":"253808","messageId":"xmqqppbg7uxk.fsf@gitster.dls.corp.google.com","threadId":"38179","inReplyTo":"CAGZ79kZCTb5Uj34Kj-qnQEWmD+4=3U8LyeHxQWmLg-ozJr7Azg@mail.gmail.com","subject":"Re: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-18T17:02:31Z","receivedAt":"2014-12-18T17:02:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This would change the current behavior. In the case of !atomic we want\n> to consider all commands and not stop early.\n\nQuite right.\n\n> So maybe more\n> if (!cmd->error_string) {\n> if (!use_atomic\n> && ref_transaction_commit(...)) {\n> ...\n> }\n> } else {\n> if (use_atomic)\n> goto check_atomic_commit;\n> }\n\nDon't you need to rollback/abort/free in one-at-a-time mode and did\nnot commit because error_string is already set, though?\n"},{"id":"253811","messageId":"1418924706-9843-1-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"xmqqppbg7uxk.fsf@gitster.dls.corp.google.com","subject":"[PATCHv4 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-18T17:45:06Z","receivedAt":"2014-12-18T17:45:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nUpdate 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\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nv4 of the series only resends this patch as the others did not get any \nfeedback to improve.\n \nNotes:\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\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 builtin/receive-pack.c | 83 +++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 69 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e76e5d5..4952d91 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@@ -1052,6 +1055,7 @@ static void execute_commands(struct command *commands,\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n \tstruct iterate_data data;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (unpacker_error) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next)\n@@ -1059,6 +1063,16 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\tif (use_atomic) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"transaction error\";\n+\t\t\treturn;\n+\t\t}\n+\t}\n \tdata.cmds = commands;\n \tdata.si = si;\n \tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n@@ -1087,7 +1101,30 @@ static void execute_commands(struct command *commands,\n \t\tif (cmd->skip_update)\n \t\t\tcontinue;\n \n+\t\tif (!use_atomic) {\n+\t\t\ttransaction = ref_transaction_begin(&err);\n+\t\t\tif (!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 start transaction\";\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n \t\tcmd->error_string = update(cmd, si);\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\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@@ -1096,10 +1133,28 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n+\tif (use_atomic) {\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = err.buf;\n+\t\t}\n+\t\tref_transaction_free(transaction);\n+\t}\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+\treturn;\n+\n+atomic_failure:\n+\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\tif (!cmd->error_string)\n+\t\t\tcmd->error_string = \"atomic push failure\";\n+\n+\tref_transaction_free(transaction);\n }\n \n static struct command **queue_command(struct command **tail,\n-- \n2.2.0.31.gad78000.dirty\n"},{"id":"253817","messageId":"CAPig+cS9hJBga7BU-YC3bNG23Tb30kQsXydwGyRYb1T_0fiVqw@mail.gmail.com","threadId":"38179","inReplyTo":"1418924706-9843-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-18T22:26:55Z","receivedAt":"2014-12-18T22:26:55Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 18, 2014 at 12:45 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n>\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>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index e76e5d5..4952d91 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1059,6 +1063,16 @@ static void execute_commands(struct command *commands,\n> return;\n> }\n>\n> + if (use_atomic) {\n> + transaction = ref_transaction_begin(&err);\n\nRepeating from my earlier review[1]: If the 'pre-receive' hook\n\"declines\", then this transaction is left dangling (and its resources\nleaked).\n\n> + if (!transaction) {\n> + error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + cmd->error_string = \"transaction error\";\n\nThe !use_atomic case (below), calls this error \"failed to start\ntransaction\", not merely \"transaction error\".\n\nFurthermore, in the use_atomic case (also below), when a commit fails,\nyou assign err.buf to cmd->error_string rather than a generic\n\"transaction error\" message. What differs between these cases which\nmakes the generic message preferable here over the more specific\nerr.buf message?\n\n> + return;\n> + }\n> + }\n> data.cmds = commands;\n> data.si = si;\n> if (check_everything_connected(iterate_receive_command_list, 0, &data))\n> @@ -1087,7 +1101,30 @@ static void execute_commands(struct command *commands,\n> if (cmd->skip_update)\n> continue;\n>\n> + if (!use_atomic) {\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + cmd->error_string = \"failed to start transaction\";\n> + continue;\n> + }\n> + }\n> cmd->error_string = update(cmd, si);\n> + if (!cmd->error_string) {\n> + if (!use_atomic\n> + && ref_transaction_commit(transaction, &err)) {\n> + ref_transaction_free(transaction);\n\nRepeating from my earlier review[1]: This is leaking 'transaction' for\neach successful commit (and only freeing it upon commit error).\n\n> + rp_error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + cmd->error_string = \"failed to update ref\";\n> + }\n> + } else if (use_atomic) {\n> + goto atomic_failure;\n\nSee commentary below.\n\n> + } else {\n> + ref_transaction_free(transaction);\n> + }\n> +\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> @@ -1096,10 +1133,28 @@ static void execute_commands(struct command *commands,\n> }\n> }\n>\n> + if (use_atomic) {\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + cmd->error_string = err.buf;\n\nAt the end of this function, strbuf_release(&err) is invoked, which\nleaves all these cmd->error_strings dangling.\n\n> + }\n> + ref_transaction_free(transaction);\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> + return;\n> +\n> +atomic_failure:\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + if (!cmd->error_string)\n> + cmd->error_string = \"atomic push failure\";\n> +\n> + ref_transaction_free(transaction);\n\ngoto's can help simplify error-handling when multiple conditional\nbranches need to perform common cleanup, however, this label\ncorresponds to only a single goto statement. Placing the cleanup code\nfor that one case so far away from the point where the condition is\ndetected actually obfuscates the code slightly rather than clarifying\nit. It would be a bit easier to understand the logic if this cleanup\n(plus a 'return') was inlined directly at the location of the 'goto'.\n\nBy the way, you employ the idiom of iterating over 'commands' and\nassigning error_string often enough, that it might be worthwhile to\nwrap it in a function. In that case, inlining the above cleanup code\nin place of the 'goto' would make it even easier to read since it\nwould be a mere two-liner.\n\n> }\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/261455\n"},{"id":"253820","messageId":"1418948521-30787-1-git-send-email-sbeller@google.com","threadId":"38179","inReplyTo":"CAPig+cS9hJBga7BU-YC3bNG23Tb30kQsXydwGyRYb1T_0fiVqw@mail.gmail.com","subject":"[PATCHv5 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-19T00:22:01Z","receivedAt":"2014-12-19T00:22:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nUpdate 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\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nNotes:\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 | 81 +++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 67 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e76e5d5..ebce2fa 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@@ -1052,6 +1055,7 @@ static void execute_commands(struct command *commands,\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n \tstruct iterate_data data;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (unpacker_error) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next)\n@@ -1080,6 +1084,17 @@ static void execute_commands(struct command *commands,\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n \n \tchecked_connectivity = 1;\n+\n+\tif (use_atomic) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"transaction failed to start\";\n+\t\t\treturn;\n+\t\t}\n+\t}\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (cmd->error_string)\n \t\t\tcontinue;\n@@ -1087,7 +1102,36 @@ static void execute_commands(struct command *commands,\n \t\tif (cmd->skip_update)\n \t\t\tcontinue;\n \n+\t\tif (!use_atomic) {\n+\t\t\ttransaction = ref_transaction_begin(&err);\n+\t\t\tif (!transaction) {\n+\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\tstrbuf_release(&err);\n+\t\t\t\tcmd->error_string = \"transaction failed to start\";\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n \t\tcmd->error_string = update(cmd, si);\n+\t\tif (!cmd->error_string) {\n+\t\t\tif (!use_atomic) {\n+\t\t\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\t\tstrbuf_release(&err);\n+\t\t\t\t\tcmd->error_string = \"failed to update ref\";\n+\t\t\t\t}\n+\t\t\t\tref_transaction_free(transaction);\n+\t\t\t}\n+\t\t} else {\n+\t\t\tref_transaction_free(transaction);\n+\t\t\tif (use_atomic) {\n+\t\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\t\tif (!cmd->error_string)\n+\t\t\t\t\t\tcmd->error_string = \"atomic push failure\";\n+\t\t\t\tstrbuf_release(&err);\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t}\n+\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@@ -1096,10 +1140,19 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n+\tif (use_atomic) {\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\t\t\tcmd->error_string = \"atomic transaction failed\";\n+\t\t}\n+\t\tref_transaction_free(transaction);\n+\t}\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":"253821","messageId":"CAPig+cT1HGF+Vjc8Jag7UcsXUYegbdYjNAtJYgGgreR3tXrR0g@mail.gmail.com","threadId":"38179","inReplyTo":"1418841177-12152-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 1/6] receive-pack.c: add protocol support to negotiate atomic","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-19T01:05:38Z","receivedAt":"2014-12-19T01:05:38Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 17, 2014 at 1:32 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..68ec23d 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 single atomic transaction. Either all refs are\n\n\"one single atomic\" sounds awfully redundant.\n\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 32fc540..e76e5d5 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,\n> \"Perhaps you should specify a branch such as 'master'.\\n\");\n> return 0;\n> }\n> + if (args->atomic && !atomic_supported) {\n> + return error(\"server does not support atomic push.\");\n\nJust above this code, the 'args->push_cert' check uses die() rather\nthan error() when the remote side fails to support the requested\nfeature.\n\n> + }\n> + use_atomic = atomic_supported && args->atomic;\n>\n> if (status_report)\n> strbuf_addstr(&cap_buf, \" report-status\");\n"},{"id":"253822","messageId":"CAPig+cSHp+4Q3yM8EeG-b3VqofuuhAjaKRMrjJ=5jGutY2eKhw@mail.gmail.com","threadId":"38179","inReplyTo":"1418841177-12152-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 3/6] send-pack.c: add --atomic command line argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-19T01:22:38Z","receivedAt":"2014-12-19T01:22:38Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 17, 2014 at 1:32 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/send-pack.c b/send-pack.c\n> index 77e4201..a696d5c 100644\n> --- a/send-pack.c\n> +++ b/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. \"\n> + \"status: %d\\n\", failing_ref->name, failing_ref->status);\n> + return -1;\n\nerror() returns -1, so:\n\n return error(...);\n\nAlso, why split the string literal like that? It's not warranted by\nthe indentation level. Instead:\n\n return error(\"atomic push failed for ref %s. status: %d\\n\",\n failing_ref->name, failing_ref->status);\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> @@ -373,9 +397,21 @@ int send_pack(struct send_pack_args *args,\n> * the pack data.\n> */\n> for (ref = remote_refs; ref; ref = ref->next) {\n> - if (check_to_send_update(ref, args) < 0)\n> + switch (check_to_send_update(ref, args)) {\n> + case 0: /* no error */\n> + break;\n> + case -CHECK_REF_STATUS_REJECTED:\n\nI realize that Junio said that this could wait for cleanup patch by\nsomeone later, but I'd argue that defining these constants now with\ntheir proper negative values would be beneficial:\n\nFirst, it is a potential maintenance burden that each person working\non the code later must remember to negate the constants each time they\nare used. Forgetting to do so will lead to incorrect behavior.\n\nSecond, negating these constants at point-of-use implies incorrectly\nthat there is some valid use-case for the non-negated forms. It's\nmisleading and confusing.\n\n> + /*\n> + * When we know the server would reject a ref update if\n> + * we were to send it and we're trying to send the refs\n> + * atomically, abort the whole operation.\n> + */\n> + if (use_atomic)\n> + return atomic_push_failure(args, remote_refs, ref);\n> + /* Fallthrough for non atomic case. */\n> + default:\n> continue;\n> -\n> + }\n> if (!ref->deletion)\n> need_pack_data = 1;\n"},{"id":"253823","messageId":"CAPig+cSHKsrJPF8eA-iHb+=CziaDcjwE58DhKYNG53Y8EhPZug@mail.gmail.com","threadId":"38179","inReplyTo":"1418841177-12152-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 5/6] push.c: add an --atomic argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-19T01:29:17Z","receivedAt":"2014-12-19T01:29:17Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 17, 2014 at 1:32 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n>\n> Add a command line argument to the git push command to request atomic\n> pushes.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/push.c b/builtin/push.c\n> index a076b19..5731a0d 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> OPT_BIT(0, \"follow-tags\", &flags, N_(\"push missing but relevant tags\"),\n> TRANSPORT_PUSH_FOLLOW_TAGS),\n> OPT_BIT(0, \"signed\", &flags, N_(\"GPG sign the push\"), TRANSPORT_PUSH_CERT),\n> + OPT_BIT(0, \"atomic\", &flags, N_(\"use an atomic transaction remote\"),\n\nUnable to parse. Perhaps:\n\n \"use an atomic transaction on remote side\"\n\nor\n\n \"use an atomic transaction on the remote\"\n\nor\n\n \"request atomic transaction on remote side\"\n\n> + TRANSPORT_PUSH_ATOMIC),\n> OPT_END()\n> };\n>\n"},{"id":"253831","messageId":"CAPig+cSqHW9MGsPW9DCFZ8_Xz+WUc1TBpih2xvPq1NSkJQMM5Q@mail.gmail.com","threadId":"38179","inReplyTo":"1418948521-30787-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv5 4/6] receive-pack.c: use a single ref_transaction for atomic pushes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-19T10:14:20Z","receivedAt":"2014-12-19T10:14:20Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 18, 2014 at 7:22 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n>\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>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index e76e5d5..ebce2fa 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1087,7 +1102,36 @@ static void execute_commands(struct command *commands,\n> if (cmd->skip_update)\n> continue;\n>\n> + if (!use_atomic) {\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + cmd->error_string = \"transaction failed to start\";\n> + continue;\n\nFor this failure, you 'continue' the loop.\n\n> + }\n> + }\n> cmd->error_string = update(cmd, si);\n> + if (!cmd->error_string) {\n> + if (!use_atomic) {\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + cmd->error_string = \"failed to update ref\";\n\nHowever, for this failure, you fall through...\n\n> + }\n> + ref_transaction_free(transaction);\n> + }\n> + } else {\n> + ref_transaction_free(transaction);\n> + if (use_atomic) {\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + if (!cmd->error_string)\n> + cmd->error_string = \"atomic push failure\";\n> + strbuf_release(&err);\n> + return;\n> + }\n> + }\n> +\n> if (shallow_update && !cmd->error_string &&\n\nAnd here must check if an error occurred in some code above.\n\nThis is ugly and inconsistent, and feels as if the new code was\nplopped into the middle of this function without concern for overall\nflow, thus negatively impacting maintainability and readability. It\ncould be made a bit cleaner by either (1) consistently using\n'continue' for the non-atomic error cases, or (2) moving the \"shallow\"\nhandling up into the conditional where you _know_ that no error has\noccurred (error_string is NULL).\n\nHowever, this issue is symptomatic of a larger problem. Prior to this\npatch, execute_commands() was relatively straight-forward and easy to\nread and understand. With the patch, and its interleaved atomic and\nnon-atomic cases, the logic flow is a mess: it places a high cognitive\nload on the reader and is difficult to maintain and to do correctly in\nthe first place (as already evidenced).\n\nHave you considered refactoring the code to implement the atomic and\nnon-atomic cases as distinct single-purpose helper functions of\nexecute_commands()? It should be possible to do so with very little\nduplicated code between the two functions, and the result would likely\nbe much more readable and maintainable.\n\n> si->shallow_ref[cmd->index]) {\n> error(\"BUG: connectivity check has not been run on ref %s\",\n> @@ -1096,10 +1140,19 @@ static void execute_commands(struct command *commands,\n> }\n> }\n>\n> + if (use_atomic) {\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + cmd->error_string = \"atomic transaction failed\";\n> + }\n> + ref_transaction_free(transaction);\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> 2.2.1.62.g3f15098\n"}]}