{"thread":{"id":"38262","subject":"[PATCHv8 1/9] receive-pack.c: add documentation for atomic push support","startedAt":"2014-12-30T02:36:38Z","lastAt":"2014-12-30T21:15:10Z","messageCount":24,"participants":["Stefan Beller","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"254184","messageId":"1419907007-19387-1-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":null,"subject":"[PATCHv8 0/9] atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:38Z","receivedAt":"2014-12-30T02:36:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The patch\n [PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes\nwas dropped and redone as 3 separate patches. This wasn't just done for doing it,\nbut the end result has also changed. We have more smaller functions doing\none thing instead of these larger functions. Thanks for the ideas, Eric!\n\nAlso the advertisement of the atomic capabilites was moved to a later new patch\nin this series. This helps when you want to bisect this series later.\nThanks Michael for pointing this out!\n\nThanks,\nStefan\nRonnie Sahlberg (3):\n receive-pack.c: add documentation for atomic push support\n send-pack.c: add --atomic command line argument\n push.c: add an --atomic argument\n\nStefan Beller (6):\n send-pack: rename ref_update_to_be_sent to check_to_send_update\n receive-pack.c: simplify execute_commands\n receive-pack.c: move transaction handling in a central place\n receive-pack.c: add execute_commands_atomic function\n receive-pack.c: enable atomic push protocol support\n t5543-atomic-push.sh: add basic tests for atomic pushes\n\n Documentation/git-push.txt | 7 +-\n Documentation/git-send-pack.txt | 7 +-\n Documentation/technical/protocol-capabilities.txt | 13 +-\n builtin/push.c | 5 +\n builtin/receive-pack.c | 168 +++++++++++++++-----\n builtin/send-pack.c | 6 +-\n remote.h | 3 +-\n send-pack.c | 65 +++++++-\n send-pack.h | 3 +-\n t/t5543-atomic-push.sh | 178 ++++++++++++++++++++++\n transport.c | 5 +\n transport.h | 1 +\n 12 files changed, 410 insertions(+), 51 deletions(-)\n create mode 100755 t/t5543-atomic-push.sh\n\n-- \n2.2.1.62.g3f15098\n"},{"id":"254176","messageId":"1419907007-19387-2-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 1/9] receive-pack.c: add documentation for atomic push support","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:39Z","receivedAt":"2014-12-30T02:36:39Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nThis documents the protocol option 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. The capability is also not yet advertised\nby receive-pack as git doesn't know how to handle it yet.\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nNotes:\n Changes v1 -> v2:\n \t* Name it atomic instead of atomic-push. The name changes affects\n \t the flags send over the wire as well as the flags in\n \t struct send_pack_args\n \t* Add check, which was part of the later patch here:\n \tif (args->atomic && !atomic_supported) {\n \t\tfprintf(stderr, \"Server does not support atomic push.\");\n \t\treturn -1;\n \t}\n \n v2 -> v3:\n More concise error reporting as suggested by Junio\n -\t\tfprintf(stderr, \"Server does not support atomic push.\");\n -\t\treturn -1;\n +\t\treturn error(\"server does not support atomic push.\");\n \n skipped v4 v5\n \n v6:\n \t* s/one single atomic transaction./one atomic transaction./\n \t* die(_( instead of return error(\n \n v7:\n \t* Don't advertise the atomic push capability yet. We cannot handle it at\n \t this point.\n \t* reword commit message\n \n v8:\n \tno changes\n\n Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--\n builtin/receive-pack.c | 3 +++\n send-pack.c | 10 ++++++++++\n send-pack.h | 3 ++-\n 4 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 6d5424c..4f8a7bf 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client requested\n and server advertised. As a consequence of these rules, server MUST\n NOT advertise capabilities it does not understand.\n \n-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities\n-are sent and recognized by the receive-pack (push to server) process.\n+The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n+capabilities are sent and recognized by the receive-pack (push to server)\n+process.\n \n The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n by both upload-pack and receive-pack protocols. The 'agent' capability\n@@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress\n reporting if the local progress reporting is also being suppressed\n (e.g., via `push -q`, or if stderr does not go to a tty).\n \n+atomic\n+------\n+\n+If the server sends the 'atomic' capability it is capable of accepting\n+atomic pushes. If the pushing client requests this capability, the server\n+will update the refs in one atomic transaction. Either all refs are\n+updated or none.\n+\n allow-tip-sha1-in-want\n ----------------------\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 32fc540..4e8eaf7 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@@ -1179,6 +1180,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..6d0c159 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 = 0;\n+\tint atomic_supported = 0;\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n@@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,\n \t\tagent_supported = 1;\n \tif (server_supports(\"no-thin\"))\n \t\targs->use_thin_pack = 0;\n+\tif (server_supports(\"atomic\"))\n+\t\tatomic_supported = 1;\n \tif (args->push_cert) {\n \t\tint len;\n \n@@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\"Perhaps you should specify a branch such as 'master'.\\n\");\n \t\treturn 0;\n \t}\n+\tif (args->atomic && !atomic_supported)\n+\t\tdie(_(\"server does not support --atomic push\"));\n+\n+\tuse_atomic = atomic_supported && args->atomic;\n \n \tif (status_report)\n \t\tstrbuf_addstr(&cap_buf, \" report-status\");\n@@ -335,6 +343,8 @@ int send_pack(struct send_pack_args *args,\n \t\tstrbuf_addstr(&cap_buf, \" side-band-64k\");\n \tif (quiet_supported && (args->quiet || !args->progress))\n \t\tstrbuf_addstr(&cap_buf, \" quiet\");\n+\tif (use_atomic)\n+\t\tstrbuf_addstr(&cap_buf, \" atomic\");\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n \ndiff --git a/send-pack.h b/send-pack.h\nindex 5635457..b664648 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -13,7 +13,8 @@ struct send_pack_args {\n \t\tuse_ofs_delta:1,\n \t\tdry_run:1,\n \t\tpush_cert:1,\n-\t\tstateless_rpc:1;\n+\t\tstateless_rpc:1,\n+\t\tatomic:1;\n };\n \n int send_pack(struct send_pack_args *args,\n-- \n2.2.1.62.g3f15098\n"},{"id":"254177","messageId":"1419907007-19387-3-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 2/9] send-pack: rename ref_update_to_be_sent to check_to_send_update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:40Z","receivedAt":"2014-12-30T02:36:40Z","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 skipped v4 v5\n \n v6:\n * negative #define'd values\n \n v7, v8:\n * no changes\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 6d0c159..b7d8e01 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -190,10 +190,13 @@ static void advertise_shallow_grafts_buf(struct strbuf *sb)\n \tfor_each_commit_graft(advertise_shallow_grafts_cb, sb);\n }\n \n-static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)\n+#define CHECK_REF_NO_PUSH -1\n+#define CHECK_REF_STATUS_REJECTED -2\n+#define CHECK_REF_UPTODATE -3\n+static int check_to_send_update(const struct ref *ref, const struct send_pack_args *args)\n {\n \tif (!ref->peer_ref && !args->send_mirror)\n-\t\treturn 0;\n+\t\treturn CHECK_REF_NO_PUSH;\n \n \t/* Check for statuses set by set_ref_status_for_push() */\n \tswitch (ref->status) {\n@@ -203,10 +206,11 @@ static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_a\n \tcase REF_STATUS_REJECT_NEEDS_FORCE:\n \tcase REF_STATUS_REJECT_STALE:\n \tcase REF_STATUS_REJECT_NODELETE:\n+\t\treturn CHECK_REF_STATUS_REJECTED;\n \tcase REF_STATUS_UPTODATE:\n-\t\treturn 0;\n+\t\treturn CHECK_REF_UPTODATE;\n \tdefault:\n-\t\treturn 1;\n+\t\treturn 0;\n \t}\n }\n \n@@ -250,7 +254,7 @@ static int generate_push_cert(struct strbuf *req_buf,\n \tstrbuf_addstr(&cert, \"\\n\");\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tif (!ref_update_to_be_sent(ref, args))\n+\t\tif (check_to_send_update(ref, args) < 0)\n \t\t\tcontinue;\n \t\tupdate_seen = 1;\n \t\tstrbuf_addf(&cert, \"%s %s %s\\n\",\n@@ -369,7 +373,7 @@ int send_pack(struct send_pack_args *args,\n \t * the pack data.\n \t */\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tif (!ref_update_to_be_sent(ref, args))\n+\t\tif (check_to_send_update(ref, args) < 0)\n \t\t\tcontinue;\n \n \t\tif (!ref->deletion)\n@@ -390,7 +394,7 @@ int send_pack(struct send_pack_args *args,\n \t\tif (args->dry_run || args->push_cert)\n \t\t\tcontinue;\n \n-\t\tif (!ref_update_to_be_sent(ref, args))\n+\t\tif (check_to_send_update(ref, args) < 0)\n \t\t\tcontinue;\n \n \t\told_hex = sha1_to_hex(ref->old_sha1);\n-- \n2.2.1.62.g3f15098\n"},{"id":"254183","messageId":"1419907007-19387-4-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 3/9] send-pack.c: add --atomic command line argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:41Z","receivedAt":"2014-12-30T02:36:41Z","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 skipped v4 v5\n \n v6:\n * realign to one error string:\n +\terror(\"atomic push failed for ref %s. status: %d\\n\",\n +\t failing_ref->name, failing_ref->status);\n \n * Use correct value now (negative defined from previous patch)\n \n v7:\n * return error(...); instead of error(...); return -1;\n \n v8:\n \tno changes\n\n Documentation/git-send-pack.txt | 7 ++++++-\n builtin/send-pack.c | 6 +++++-\n remote.h | 3 ++-\n send-pack.c | 39 +++++++++++++++++++++++++++++++++++++--\n transport.c | 4 ++++\n 5 files changed, 54 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 b7d8e01..e8f60df 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -282,6 +282,29 @@ free_return:\n \treturn update_seen;\n }\n \n+\n+static int atomic_push_failure(struct send_pack_args *args,\n+\t\t\t struct ref *remote_refs,\n+\t\t\t struct ref *failing_ref)\n+{\n+\tstruct ref *ref;\n+\t/* Mark other refs as failed */\n+\tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\tif (!ref->peer_ref && !args->send_mirror)\n+\t\t\tcontinue;\n+\n+\t\tswitch (ref->status) {\n+\t\tcase REF_STATUS_EXPECTING_REPORT:\n+\t\t\tref->status = REF_STATUS_ATOMIC_PUSH_FAILED;\n+\t\t\tcontinue;\n+\t\tdefault:\n+\t\t\t; /* do nothing */\n+\t\t}\n+\t}\n+\treturn error(\"atomic push failed for ref %s. status: %d\\n\",\n+\t\t failing_ref->name, failing_ref->status);\n+}\n+\n int send_pack(struct send_pack_args *args,\n \t int fd[], struct child_process *conn,\n \t struct ref *remote_refs,\n@@ -373,9 +396,21 @@ int send_pack(struct send_pack_args *args,\n \t * the pack data.\n \t */\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tif (check_to_send_update(ref, args) < 0)\n+\t\tswitch (check_to_send_update(ref, args)) {\n+\t\tcase 0: /* no error */\n+\t\t\tbreak;\n+\t\tcase CHECK_REF_STATUS_REJECTED:\n+\t\t\t/*\n+\t\t\t * When we know the server would reject a ref update if\n+\t\t\t * we were to send it and we're trying to send the refs\n+\t\t\t * atomically, abort the whole operation.\n+\t\t\t */\n+\t\t\tif (use_atomic)\n+\t\t\t\treturn atomic_push_failure(args, remote_refs, ref);\n+\t\t\t/* Fallthrough for non atomic case. */\n+\t\tdefault:\n \t\t\tcontinue;\n-\n+\t\t}\n \t\tif (!ref->deletion)\n \t\t\tneed_pack_data = 1;\n \ndiff --git a/transport.c b/transport.c\nindex 70d38e4..c67feee 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -728,6 +728,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \t\t\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n \t\t\t\t\t\t \"remote failed to report status\", porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_ATOMIC_PUSH_FAILED:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"atomic push failed\", porcelain);\n+\t\tbreak;\n \tcase REF_STATUS_OK:\n \t\tprint_ok_ref_status(ref, porcelain);\n \t\tbreak;\n-- \n2.2.1.62.g3f15098\n"},{"id":"254185","messageId":"1419907007-19387-5-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:42Z","receivedAt":"2014-12-30T02:36:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"No functional changes intended.\n\nThis commit shortens execute_commands by moving some parts of the code\nto extra functions.\n\nSuggested-by: Eric Sunshine \nSigned-off-by: Stefan Beller \n---\n\nNotes:\n v8: no change\n \n v7:\n new in v7 as in v7 I'd split up the previous\n [PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes\n as suggested by Eric.\n \n This is pretty much\n > patch 1: Factor out code into helper functions which will be needed by\n > the upcoming atomic and non-atomic worker functions. Example helpers:\n > 'cmd->error_string' and cmd->skip_update' check; and the\n > 'si->shallow_ref[cmd->index]' check and handling.\n\n builtin/receive-pack.c | 49 ++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 32 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4e8eaf7..06eb287 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)\n \t}\n }\n \n+static int should_process_cmd(struct command *cmd)\n+{\n+\tif (cmd->error_string)\n+\t\treturn 0;\n+\tif (cmd->skip_update)\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n+static void check_shallow_bugs(struct command *commands,\n+\t\t\t struct shallow_info *si)\n+{\n+\tstruct command *cmd;\n+\tint checked_connectivity = 1;\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (!should_process_cmd(cmd))\n+\t\t\tcontinue;\n+\n+\t\tif (shallow_update && si->shallow_ref[cmd->index]) {\n+\t\t\terror(\"BUG: connectivity check has not been run on ref %s\",\n+\t\t\t cmd->ref_name);\n+\t\t\tchecked_connectivity = 0;\n+\t\t}\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+}\n+\n static void execute_commands(struct command *commands,\n \t\t\t const char *unpacker_error,\n \t\t\t struct shallow_info *si)\n {\n-\tint checked_connectivity;\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n \tstruct iterate_data data;\n@@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,\n \tfree(head_name_to_free);\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n \n-\tchecked_connectivity = 1;\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (cmd->error_string)\n-\t\t\tcontinue;\n-\n-\t\tif (cmd->skip_update)\n+\t\tif (!should_process_cmd(cmd))\n \t\t\tcontinue;\n \n \t\tcmd->error_string = update(cmd, si);\n-\t\tif (shallow_update && !cmd->error_string &&\n-\t\t si->shallow_ref[cmd->index]) {\n-\t\t\terror(\"BUG: connectivity check has not been run on ref %s\",\n-\t\t\t cmd->ref_name);\n-\t\t\tchecked_connectivity = 0;\n-\t\t}\n \t}\n-\n-\tif (shallow_update && !checked_connectivity)\n-\t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n-\t\t \"If there are errors, try to remove \"\n-\t\t \"the reported refs above\");\n+\tcheck_shallow_bugs(commands, si);\n }\n \n static struct command **queue_command(struct command **tail,\n-- \n2.2.1.62.g3f15098\n"},{"id":"254178","messageId":"1419907007-19387-6-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 5/9] receive-pack.c: move transaction handling in a central place","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:43Z","receivedAt":"2014-12-30T02:36:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"No functional changes intended.\nThis moves all code related to transactions into the execute_commands_loop\nfunction which was factored out of execute_commands. This includes\nbeginning and committing the transaction as well as dealing with the\nerrors which may occur during the begin and commit phase of a transaction.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Stefan Beller \n---\n\nNotes:\n v8:\n move execute_commands_loop before execute_commands, so it compiles/links\n without warnings.\n \n v7:\n new in v7, this is part of the previous \"[PATCH 4/7]\n receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes\"\n \n This covers the suggestion of patch 2 and 3 by Eric\n > patch 2: Factor out the main 'for' loop of execute_commands() into a\n > new function. This new function will eventually become\n > execute_commands_non_atomic(). At this point, execute_commands() is\n > pretty much in its final form with the exception of the upcoming 'if\n > (use_atomic)' conditional.\n > patch 3: Morph the function extracted in patch 2 into\n > execute_commands_non_atomic() by adding transaction handling inside\n > the 'for' loop (and applying the changes from the early part of the\n > patch which go along with that).\n\n builtin/receive-pack.c | 69 ++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 50 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 06eb287..5f44466 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@@ -822,6 +823,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@@ -831,35 +833,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@@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,\n \t\t \"the reported refs above\");\n }\n \n+static void execute_commands_loop(struct command *commands,\n+\t\t\t\t struct shallow_info *si)\n+{\n+\tstruct command *cmd;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (!should_process_cmd(cmd))\n+\t\t\tcontinue;\n+\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tcmd->error_string = \"transaction failed to start\";\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tcmd->error_string = update(cmd, si);\n+\n+\t\tif (!cmd->error_string\n+\t\t && ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tcmd->error_string = \"failed to update ref\";\n+\t\t}\n+\t\tref_transaction_free(transaction);\n+\t}\n+\n+\tstrbuf_release(&err);\n+}\n+\n static void execute_commands(struct command *commands,\n \t\t\t const char *unpacker_error,\n \t\t\t struct shallow_info *si)\n@@ -1107,12 +1142,8 @@ static void execute_commands(struct command *commands,\n \tfree(head_name_to_free);\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (!should_process_cmd(cmd))\n-\t\t\tcontinue;\n+\texecute_commands_loop(commands, si);\n \n-\t\tcmd->error_string = update(cmd, si);\n-\t}\n \tcheck_shallow_bugs(commands, si);\n }\n \n-- \n2.2.1.62.g3f15098\n"},{"id":"254182","messageId":"1419907007-19387-7-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:44Z","receivedAt":"2014-12-30T02:36:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Update receive-pack to use an atomic transaction iff the client negotiated\nthat it wanted atomic push. This leaves the default behavior to be the old\nnon-atomic one ref at a time update. This is to cause as little disruption\nas possible to existing clients. It is unknown if there are client scripts\nthat depend on the old non-atomic behavior so we make it opt-in for now.\n\nIf it turns out over time that there are no client scripts that depend on the\nold behavior we can change git to default to use atomic pushes and instead\noffer an opt-out argument for people that do not want atomic pushes.\n\nInspired-by: Ronnie Sahlberg \nHelped-by: Eric Sunshine \nSigned-off-by: Stefan Beller \n---\n\nNotes:\n Changes in v8:\n \tremoved superflous \"}\" to make it compile again\n \n Changes in v7:\n \tEric suggested to replace \"[PATCH 4/7] receive-pack.c:\n \treceive-pack.c: use a single ref_transaction for atomic pushes\"\n \tby smaller patches\n \tThis is the last patch replacing said large commit.\n \n Changes in v6:\n \tThis is a complete rewrite of the patch essentially.\n \tEric suggested to split up the flow into functions, so\n \tit is easier to read. It's more lines of code, but indeed\n \tit's easier to follow. Thanks Eric!\n \n \tNote there is another patch following this one\n \tmoving the helper functions above execute_commands.\n \tI just choose the order of the functions in this patch\n \tto have a better diff (just inserting the call to the function\n \texecute_commands_non_atomic and that function directly follows.)\n \tThe next patch of the series will move that up.\n \n \tBecause of the rewrite and the fixes of the previous five\n \tversions there is not much left of Ronnies original patch,\n \tso I'll claim authorship of this one.\n \n Changes v1 -> v2:\n \t* update(...) assumes to be always in a transaction\n \t* Caring about when to begin/commit transactions is put\n \t into execute_commands\n v2->v3:\n \t* meditated about the error flow. Now we always construct a local\n \t strbuf err if required. Then the flow is easier to follow and\n \t destruction of it is performed nearby.\n \t* early return in execute_commands if transaction_begin fails.\n \n v3->v4:\n \t* revamp logic again. This should keep the non atomic behavior\n \t as is (in case of error say so, in non error case just free the\n \t transaction). In the atomic case we either do nothing (when no error),\n \t or abort with the goto.\n \n \t\tif (!cmd->error_string) {\n \t\t\tif (!use_atomic\n \t\t\t && ref_transaction_commit(transaction, &err)) {\n \t\t\t\tref_transaction_free(transaction);\n \t\t\t\trp_error(\"%s\", err.buf);\n \t\t\t\tstrbuf_release(&err);\n \t\t\t\tcmd->error_string = \"failed to update ref\";\n \t\t\t}\n \t\t} else if (use_atomic) {\n \t\t\tgoto atomic_failure;\n \t\t} else {\n \t\t\tref_transaction_free(transaction);\n \t\t}\n \n \t * Having the goto directly there when checking for cmd->error_string,\n \t we don't need to do it again, so the paragraph explaining the error\n \t checking is gone as well. (Previous patch had the following, this is\n \t put at the end of the function, where the goto jumps to and the comment\n \t has been dropped.\n +\t\t/*\n +\t\t * update(...) may abort early (i.e. because the hook refused to\n +\t\t * update that ref) which then doesn't even record a transaction\n +\t\t * regarding that ref. Make sure all commands are without error\n +\t\t * and then commit atomically.\n +\t\t */\n +\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n +\t\t\tif (cmd->error_string)\n +\t\t\t\tbreak;\n \n v4->v5:\n Eric wrote:\n > Repeating from my earlier review[1]: If the 'pre-receive' hook\n > \"declines\", then this transaction is left dangling (and its resources\n > leaked).\n \n You're right. The initialization of the transaction is now\n near the actual loop after the pre receive hook.\n \n > The !use_atomic case (below), calls this error \"failed to start\n > transaction\", not merely \"transaction error\".\n \n ok, now both are \"transaction failed to start\".\n In all cases where these generic errors are reported,\n we do have a rp_error(...) with details.\n \n > Furthermore, in the use_atomic case (also below), when a commit fails,\n > you assign err.buf to cmd->error_string rather than a generic\n > \"transaction error\" message. What differs between these cases which\n > makes the generic message preferable here over the more specific\n > err.buf message?\n \n They are the same now.\n \n > Repeating from my earlier review[1]: This is leaking 'transaction' for\n > each successful commit (and only freeing it upon commit error).\n \n Right. I thought I had it covered with the else clause. Of course not.\n \n > At the end of this function, strbuf_release(&err) is invoked, which\n > leaves all these cmd->error_strings dangling.\n \n I removed all assignments of err.buf now.\n \n > goto's can help simplify error-handling when multiple conditional\n > branches need to perform common cleanup, however, this label\n > corresponds to only a single goto statement.\n \n moved up again.\n\n builtin/receive-pack.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 49 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 5f44466..35a2264 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1076,8 +1076,8 @@ static void check_shallow_bugs(struct command *commands,\n \t\t \"the reported refs above\");\n }\n \n-static void execute_commands_loop(struct command *commands,\n-\t\t\t\t struct shallow_info *si)\n+static void execute_commands_non_atomic(struct command *commands,\n+\t\t\t\t\tstruct shallow_info *si)\n {\n \tstruct command *cmd;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -1104,7 +1104,50 @@ static void execute_commands_loop(struct command *commands,\n \t\t}\n \t\tref_transaction_free(transaction);\n \t}\n+\tstrbuf_release(&err);\n+}\n+\n+static void execute_commands_atomic(struct command *commands,\n+\t\t\t\t\tstruct shallow_info *si)\n+{\n+\tstruct command *cmd;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tconst char *reported_error = \"atomic push failure\";\n+\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\tstrbuf_reset(&err);\n+\t\treported_error = \"transaction failed to start\";\n+\t\tgoto failure;\n+\t}\n+\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (!should_process_cmd(cmd))\n+\t\t\tcontinue;\n \n+\t\tcmd->error_string = update(cmd, si);\n+\n+\t\tif (cmd->error_string)\n+\t\t\tgoto failure;\n+\t}\n+\n+\tif (ref_transaction_commit(transaction, &err)) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\treported_error = \"atomic transaction failed\";\n+\t\tgoto failure;\n+\t}\n+\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\n+\treturn;\n+\n+failure:\n+\tfor (cmd = commands; cmd; cmd = cmd->next)\n+\t\tif (!cmd->error_string)\n+\t\t\tcmd->error_string = reported_error;\n+\n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n }\n \n@@ -1142,7 +1185,10 @@ static void execute_commands(struct command *commands,\n \tfree(head_name_to_free);\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n \n-\texecute_commands_loop(commands, si);\n+\tif (use_atomic)\n+\t\texecute_commands_atomic(commands, si);\n+\telse\n+\t\texecute_commands_non_atomic(commands, si);\n \n \tcheck_shallow_bugs(commands, si);\n }\n-- \n2.2.1.62.g3f15098\n"},{"id":"254180","messageId":"1419907007-19387-8-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 7/9] receive-pack.c: enable atomic push protocol support","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:45Z","receivedAt":"2014-12-30T02:36:45Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This enables the atomic protocol option as implemented in the previous\npatches.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n v8:\n no changes\n \n v7:\n * new with v7 of the patch series.\n * this was part of the first patch in the series, moved back here\n for bisectability\n\n builtin/receive-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 35a2264..61eba4e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -173,7 +173,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-- \n2.2.1.62.g3f15098\n"},{"id":"254179","messageId":"1419907007-19387-9-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 8/9] push.c: add an --atomic argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:46Z","receivedAt":"2014-12-30T02:36:46Z","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 v8:\n no changes\n \n v7:\n \tUse OPT_BOOL instead of OPT_BIT.\n \tThis allows for --no-atomic option on the command line.\n v6:\n -\t\tOPT_BIT(0, \"atomic\", &flags, N_(\"use an atomic transaction remote\"),\n +\t\tOPT_BIT(0, \"atomic\", &flags, N_(\"request atomic transaction on remote side\"),\n \n skipped v4 v5\n \n v2->v3:\n \t* s/atomic-push/atomic/\n \t* s/the an/an/\n \t* no serverside, but just remote instead\n \t* TRANSPORT_PUSH_ATOMIC instead of TRANSPORT_ATOMIC_PUSH\n \n Changes v1 -> v2\n \tIt's --atomic now! (dropping the -push)\n\n Documentation/git-push.txt | 7 ++++++-\n builtin/push.c | 5 +++++\n transport.c | 1 +\n transport.h | 1 +\n 4 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 21b3f29..4764fcf 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects\n SYNOPSIS\n --------\n [verse]\n-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=]\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+--[no-]atomic::\n+\tUse an atomic transaction on the remote side if available.\n+\tEither all refs are updated, or on error, no refs are updated.\n+\tIf the server does not support atomic pushes the push will fail.\n+\n --receive-pack=::\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..8f1d945 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -487,6 +487,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tint flags = 0;\n \tint tags = 0;\n \tint rc;\n+\tint atomic = 0;\n \tconst char *repo = NULL;\t/* default repository */\n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n@@ -518,6 +519,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"follow-tags\", &flags, N_(\"push missing but relevant tags\"),\n \t\t\tTRANSPORT_PUSH_FOLLOW_TAGS),\n \t\tOPT_BIT(0, \"signed\", &flags, N_(\"GPG sign the push\"), TRANSPORT_PUSH_CERT),\n+\t\tOPT_BOOL(0, \"atomic\", &atomic, N_(\"request atomic transaction on remote side\")),\n \t\tOPT_END()\n \t};\n \n@@ -533,6 +535,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tif (tags)\n \t\tadd_refspec(\"refs/tags/*\");\n \n+\tif (atomic)\n+\t\tflags |= TRANSPORT_PUSH_ATOMIC;\n+\n \tif (argc > 0) {\n \t\trepo = argv[0];\n \t\tset_refspecs(argv + 1, argc - 1, repo);\ndiff --git a/transport.c b/transport.c\nindex c67feee..1373152 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -830,6 +830,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n \targs.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n \targs.push_cert = !!(flags & TRANSPORT_PUSH_CERT);\n+\targs.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);\n \targs.url = transport->url;\n \n \tret = send_pack(&args, data->fd, data->conn, remote_refs,\ndiff --git a/transport.h b/transport.h\nindex 3e0091e..18d2cf8 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -125,6 +125,7 @@ struct transport {\n #define TRANSPORT_PUSH_NO_HOOK 512\n #define TRANSPORT_PUSH_FOLLOW_TAGS 1024\n #define TRANSPORT_PUSH_CERT 2048\n+#define TRANSPORT_PUSH_ATOMIC 4096\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n-- \n2.2.1.62.g3f15098\n"},{"id":"254181","messageId":"1419907007-19387-10-git-send-email-sbeller@google.com","threadId":"38262","inReplyTo":"1419907007-19387-1-git-send-email-sbeller@google.com","subject":"[PATCHv8 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T02:36:47Z","receivedAt":"2014-12-30T02:36:47Z","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 v7, v8: no changes\n \n v6:\n the same as v2, so I can resend the whole series as v6\n \n v3 v4 v5 were skipped\n \n Changes v1 -> v2:\n > Please drop unused comments; they are distracting.\n \n ok\n \n > It is not wrong per-se, but haven't you already tested in\n > combination with --mirror in the previous test?\n \n I fixed the previous tests, so that there is no --mirror\n and --atomic together. There is still a first --mirror push\n for setup and a second with --atomic though\n \n > check_branches upstream master HEAD@{2} second HEAD~\n \n A similar function test_ref_upstream is introduced.\n \n > What's the value of this test? Isn't it a non-fast-forward check\n > you already tested in the previous one?\n \n I messed up there. Originally I wanted to test the 2 different\n stages of rejection. A non-fast-forward check is done locally and\n we don't even try pushing. But I also want to test if we locally\n thing all is good, but the server refuses a ref to update.\n This is now done with the last test named 'atomic push obeys\n update hook preventing a branch to be pushed'. And that still fails.\n \n I'll investigate that, while still sending out the series for another\n review though.\n \n * Redone the test helper, there is test_ref_upstream now.\n This tests explicitely for SHA1 values of the ref.\n (It's needed in the last test for example. The git push fails,\n but still modifies the ref :/ )\n * checked all && chains and repaired them\n * sometimes make use of git -C \n \n Notes v1:\n Originally Ronnie had a similar patch prepared. But as I added\n more tests and cleaned up the existing tests (e.g. use test_commit\n instead of \"echo one >file && gitadd file && git commit -a -m 'one'\",\n removal of dead code), the file has changed so much that I'd rather\n take ownership.\n\n t/t5543-atomic-push.sh | 178 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 178 insertions(+)\n create mode 100755 t/t5543-atomic-push.sh\n\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nnew file mode 100755\nindex 0000000..b81a542\n--- /dev/null\n+++ b/t/t5543-atomic-push.sh\n@@ -0,0 +1,178 @@\n+#!/bin/sh\n+\n+test_description='pushing to a repository using the atomic push option'\n+\n+. ./test-lib.sh\n+\n+mk_repo_pair () {\n+\trm -rf workbench upstream &&\n+\ttest_create_repo upstream &&\n+\ttest_create_repo workbench &&\n+\t(\n+\t\tcd upstream &&\n+\t\tgit config receive.denyCurrentBranch warn\n+\t) &&\n+\t(\n+\t\tcd workbench &&\n+\t\tgit remote add up ../upstream\n+\t)\n+}\n+\n+# Compare the ref ($1) in upstream with a ref value from workbench ($2)\n+# i.e. test_refs second HEAD@{2}\n+test_refs () {\n+\ttest $# = 2 &&\n+\tgit -C upstream rev-parse --verify \"$1\" >expect &&\n+\tgit -C workbench rev-parse --verify \"$2\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'atomic push works for a single branch' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit push --atomic up master\n+\t) &&\n+\ttest_refs master master\n+'\n+\n+test_expect_success 'atomic push works for two branches' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit branch second &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit checkout second &&\n+\t\ttest_commit three &&\n+\t\tgit push --atomic up master second\n+\t) &&\n+\ttest_refs master master &&\n+\ttest_refs second second\n+'\n+\n+test_expect_success 'atomic push works in combination with --mirror' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second &&\n+\t\ttest_commit two &&\n+\t\tgit push --atomic --mirror up\n+\t) &&\n+\ttest_refs master master &&\n+\ttest_refs second second\n+'\n+\n+test_expect_success 'atomic push works in combination with --force' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit branch second master &&\n+\t\ttest_commit two_a &&\n+\t\tgit checkout second &&\n+\t\ttest_commit two_b &&\n+\t\ttest_commit three_b &&\n+\t\ttest_commit four &&\n+\t\tgit push --mirror up &&\n+\t\t# The actual test is below\n+\t\tgit checkout master &&\n+\t\ttest_commit three_a &&\n+\t\tgit checkout second &&\n+\t\tgit reset --hard HEAD^ &&\n+\t\tgit push --force --atomic up master second\n+\t) &&\n+\ttest_refs master master &&\n+\ttest_refs second second\n+'\n+\n+# set up two branches where master can be pushed but second can not\n+# (non-fast-forward). Since second can not be pushed the whole operation\n+# will fail and leave master untouched.\n+test_expect_success 'atomic push fails if one branch fails' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second master &&\n+\t\ttest_commit two &&\n+\t\ttest_commit three &&\n+\t\ttest_commit four &&\n+\t\tgit push --mirror up &&\n+\t\tgit reset --hard HEAD~2 &&\n+\t\ttest_commit five &&\n+\t\tgit checkout master &&\n+\t\ttest_commit six &&\n+\t\ttest_must_fail git push --atomic --all up\n+\t) &&\n+\ttest_refs master HEAD@{7} &&\n+\ttest_refs second HEAD@{4}\n+'\n+\n+test_expect_success 'atomic push fails if one tag fails remotely' '\n+\t# prepare the repo\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second master &&\n+\t\ttest_commit two &&\n+\t\tgit push --mirror up\n+\t) &&\n+\t# a third party modifies the server side:\n+\t(\n+\t\tcd upstream &&\n+\t\tgit checkout second &&\n+\t\tgit tag test_tag second\n+\t) &&\n+\t# see if we can now push both branches.\n+\t(\n+\t\tcd workbench &&\n+\t\tgit checkout master &&\n+\t\ttest_commit three &&\n+\t\tgit checkout second &&\n+\t\ttest_commit four &&\n+\t\tgit tag test_tag &&\n+\t\ttest_must_fail git push --tags --atomic up master second\n+\t) &&\n+\ttest_refs master HEAD@{3} &&\n+\ttest_refs second HEAD@{1}\n+'\n+\n+test_expect_success 'atomic push obeys update hook preventing a branch to be pushed' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second master &&\n+\t\ttest_commit two &&\n+\t\tgit push --mirror up\n+\t) &&\n+\t(\n+\t\tcd upstream &&\n+\t\tHOOKDIR=\"$(git rev-parse --git-dir)/hooks\" &&\n+\t\tHOOK=\"$HOOKDIR/update\" &&\n+\t\tmkdir -p \"$HOOKDIR\" &&\n+\t\twrite_script \"$HOOK\" <<-\\EOF\n+\t\t\t# only allow update to master from now on\n+\t\t\ttest \"$1\" = \"refs/heads/master\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd workbench &&\n+\t\tgit checkout master &&\n+\t\ttest_commit three &&\n+\t\tgit checkout second &&\n+\t\ttest_commit four &&\n+\t\ttest_must_fail git push --atomic up master second\n+\t) &&\n+\ttest_refs master HEAD@{3} &&\n+\ttest_refs second HEAD@{1}\n+'\n+\n+test_done\n-- \n2.2.1.62.g3f15098\n"},{"id":"254187","messageId":"CAPig+cT9JVkDvUUsbx9HW8HpakCb9SkoQf3LJZc4h3TQTt2ZXQ@mail.gmail.com","threadId":"38262","inReplyTo":"1419907007-19387-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T06:11:38Z","receivedAt":"2014-12-30T06:11:38Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n> No functional changes intended.\n\nThis is useful to know but is of secondary importance, thus should be\nplaced after the explanation and justification of the change.\n\n> Subject: receive-pack.c: simplify execute_commands\n> This commit shortens execute_commands by moving some parts of the code\n> to extra functions.\n\nThe _real_ reason for moving these bits of code into their own\nfunctions is that you intend to introduce additional callers in\nsubsequent patches. That is what the commit message (including\nsubject) should be conveying to the reader.\n\nThe claimed simplification is questionable and not of particular\nimportance; and it's misleading to paint it as a goal of the patch.\nConsequently, you could drop mention of it altogether.\n\nMore below.\n\n> Suggested-by: Eric Sunshine \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 4e8eaf7..06eb287 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)\n> }\n> }\n>\n> +static int should_process_cmd(struct command *cmd)\n> +{\n> + if (cmd->error_string)\n> + return 0;\n> + if (cmd->skip_update)\n> + return 0;\n> + return 1;\n\nAlternately, depending upon the polarity of your brain, you could\ncollapse the entire function body to:\n\n return !cmd->error_string && !cmd->skip_update;\n\nor:\n\n return !(cmd->error_string || cmd->skip_update);\n\n> +}\n> +\n> +static void check_shallow_bugs(struct command *commands,\n> + struct shallow_info *si)\n> +{\n> + struct command *cmd;\n> + int checked_connectivity = 1;\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (!should_process_cmd(cmd))\n> + continue;\n> +\n> + if (shallow_update && si->shallow_ref[cmd->index]) {\n\nHere, inside the loop, you check 'shallow_update'...\n\n> + error(\"BUG: connectivity check has not been run on ref %s\",\n> + cmd->ref_name);\n> + checked_connectivity = 0;\n> + }\n> + }\n> + if (shallow_update && !checked_connectivity)\n\nAnd here, after the loop, you check 'shallow_update'.\n\nBut, if you examine the overall logic, you will find that this\nfunction does _nothing_ at all when 'shallow_update' is false (other\nthan uselessly looping through 'commands'). Therefore, either check\n'shallow_update' just once at the beginning of the function and exit\nearly if false, or have the caller check 'shallow_update' and only\ninvoke check_shallow_bugs() if true.\n\nAlso, since nothing happens between them, the two conditionals inside\nthe loop can be coalesced:\n\n if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {\n\n> + error(\"BUG: run 'git fsck' for safety.\\n\"\n> + \"If there are errors, try to remove \"\n> + \"the reported refs above\");\n\nIn v6, you considered this a fatal error in the atomic case, which\ncaused the entire transaction to be rolled back. However, in this\nversion, this error has no effect whatsoever on the atomic\ntransaction, which is a rather significant behavioral departure. Which\nis correct? (This is a genuine question; not at all rhetorical.) If\nfailing the entire transaction is the correct thing to do, then this\nis going to need some more work.\n\n> +}\n> +\n> static void execute_commands(struct command *commands,\n> const char *unpacker_error,\n> struct shallow_info *si)\n> {\n> - int checked_connectivity;\n> struct command *cmd;\n> unsigned char sha1[20];\n> struct iterate_data data;\n> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,\n> free(head_name_to_free);\n> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>\n> - checked_connectivity = 1;\n> for (cmd = commands; cmd; cmd = cmd->next) {\n> - if (cmd->error_string)\n> - continue;\n> -\n> - if (cmd->skip_update)\n> + if (!should_process_cmd(cmd))\n> continue;\n>\n> cmd->error_string = update(cmd, si);\n> - if (shallow_update && !cmd->error_string &&\n> - si->shallow_ref[cmd->index]) {\n> - error(\"BUG: connectivity check has not been run on ref %s\",\n> - cmd->ref_name);\n> - checked_connectivity = 0;\n> - }\n> }\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> + check_shallow_bugs(commands, si);\n> }\n>\n> static struct command **queue_command(struct command **tail,\n> --\n> 2.2.1.62.g3f15098\n"},{"id":"254188","messageId":"CAPig+cQ7GhrZqXH3zRYTWjwuWKwDNNwLRKG516dXS6et2TURgw@mail.gmail.com","threadId":"38262","inReplyTo":"1419907007-19387-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T07:08:04Z","receivedAt":"2014-12-30T07:08:04Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n> Subject: receive-pack.c: add documentation for atomic push support\n\nThis patch is doing a lot more than merely adding documentation. It's\nalso updating send-pack and receive-pack to be able to negotiate the\nnew protocol capability \"atomic\". The fact that you removed the actual\nadvertisement of \"atomic\" from this patch doesn't turn it into a\ndocumentation-only patch.\n\nWhen Michael raised the issue of the server being \"broken\" due to\nadvertising a capability which it does not yet implement, his\nrecommendation[1] was to reorder the patches, not to split out the one\ntiny bit (capability advertisement) from the larger change. Was there\nan insurmountable conflict which prevented you from reordering the\npatches? This is a genuine question since splitting off advertisement\n-- and only advertisement -- to a patch later in the series smells\nlike a least-resistance approach, rather than a proper solution to the\nissue Michael raised.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/261793\n\n> This documents the protocol option 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. The capability is also not yet advertised\n> by receive-pack as git doesn't know how to handle it yet.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\n> index 6d5424c..4f8a7bf 100644\n> --- a/Documentation/technical/protocol-capabilities.txt\n> +++ b/Documentation/technical/protocol-capabilities.txt\n> @@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client requested\n> and server advertised. As a consequence of these rules, server MUST\n> NOT advertise capabilities it does not understand.\n>\n> -The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities\n> -are sent and recognized by the receive-pack (push to server) process.\n> +The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n> +capabilities are sent and recognized by the receive-pack (push to server)\n> +process.\n>\n> The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n> by both upload-pack and receive-pack protocols. The 'agent' capability\n> @@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress\n> reporting if the local progress reporting is also being suppressed\n> (e.g., via `push -q`, or if stderr does not go to a tty).\n>\n> +atomic\n> +------\n> +\n> +If the server sends the 'atomic' capability it is capable of accepting\n> +atomic pushes. If the pushing client requests this capability, the server\n> +will update the refs in one atomic transaction. Either all refs are\n> +updated or none.\n> +\n> allow-tip-sha1-in-want\n> ----------------------\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 32fc540..4e8eaf7 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> @@ -1179,6 +1180,8 @@ static struct command *read_head_info(struct sha1_array *shallow)\n> use_sideband = LARGE_PACKET_MAX;\n> if (parse_feature_request(feature_list, \"quiet\"))\n> quiet = 1;\n> + if (parse_feature_request(feature_list, \"atomic\"))\n> + use_atomic = 1;\n> }\n>\n> if (!strcmp(line, \"push-cert\")) {\n> diff --git a/send-pack.c b/send-pack.c\n> index 949cb61..6d0c159 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> int use_sideband = 0;\n> int quiet_supported = 0;\n> int agent_supported = 0;\n> + int use_atomic = 0;\n> + int atomic_supported = 0;\n> unsigned cmds_sent = 0;\n> int ret;\n> struct async demux;\n> @@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,\n> agent_supported = 1;\n> if (server_supports(\"no-thin\"))\n> args->use_thin_pack = 0;\n> + if (server_supports(\"atomic\"))\n> + atomic_supported = 1;\n> if (args->push_cert) {\n> int len;\n>\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> + die(_(\"server does not support --atomic push\"));\n> +\n> + use_atomic = atomic_supported && args->atomic;\n>\n> if (status_report)\n> strbuf_addstr(&cap_buf, \" report-status\");\n> @@ -335,6 +343,8 @@ int send_pack(struct send_pack_args *args,\n> strbuf_addstr(&cap_buf, \" side-band-64k\");\n> if (quiet_supported && (args->quiet || !args->progress))\n> strbuf_addstr(&cap_buf, \" quiet\");\n> + if (use_atomic)\n> + strbuf_addstr(&cap_buf, \" atomic\");\n> if (agent_supported)\n> strbuf_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> use_ofs_delta:1,\n> dry_run:1,\n> push_cert:1,\n> - stateless_rpc:1;\n> + stateless_rpc:1,\n> + atomic:1;\n> };\n>\n> int send_pack(struct send_pack_args *args,\n> --\n> 2.2.1.62.g3f15098\n"},{"id":"254189","messageId":"CAPig+cQEO+LKZJDzE-sUmo+td=djsOdjx-ouQEMRTTkOBMwzjA@mail.gmail.com","threadId":"38262","inReplyTo":"1419907007-19387-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T07:46:03Z","receivedAt":"2014-12-30T07:46:03Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n> No functional changes intended.\n>\n> This commit shortens execute_commands by moving some parts of the code\n> to extra functions.\n>\n> Suggested-by: Eric Sunshine \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 4e8eaf7..06eb287 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)\n> }\n> }\n>\n> +static void check_shallow_bugs(struct command *commands,\n> + struct shallow_info *si)\n> +{\n> + struct command *cmd;\n> + int checked_connectivity = 1;\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (!should_process_cmd(cmd))\n> + continue;\n> +\n> + if (shallow_update && si->shallow_ref[cmd->index]) {\n\nAnother issue: In the original code, 'si->shallow_ref[cmd->index]' is\nonly checked if cmd->error_string is NULL, but here you check it\nunconditionally, despite the commit message claiming no functional\nchanges. Did you verify that such a behavioral change is benign? (This\nis a genuine question.)\n\n> + error(\"BUG: connectivity check has not been run on ref %s\",\n> + cmd->ref_name);\n> + checked_connectivity = 0;\n> + }\n> + }\n> + if (shallow_update && !checked_connectivity)\n> + error(\"BUG: run 'git fsck' for safety.\\n\"\n> + \"If there are errors, try to remove \"\n> + \"the reported refs above\");\n> +}\n> +\n> static void execute_commands(struct command *commands,\n> const char *unpacker_error,\n> struct shallow_info *si)\n> {\n> - int checked_connectivity;\n> struct command *cmd;\n> unsigned char sha1[20];\n> struct iterate_data data;\n> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,\n> free(head_name_to_free);\n> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>\n> - checked_connectivity = 1;\n> for (cmd = commands; cmd; cmd = cmd->next) {\n> - if (cmd->error_string)\n> - continue;\n> -\n> - if (cmd->skip_update)\n> + if (!should_process_cmd(cmd))\n> continue;\n>\n> cmd->error_string = update(cmd, si);\n> - if (shallow_update && !cmd->error_string &&\n> - si->shallow_ref[cmd->index]) {\n> - error(\"BUG: connectivity check has not been run on ref %s\",\n> - cmd->ref_name);\n> - checked_connectivity = 0;\n> - }\n> }\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> + check_shallow_bugs(commands, si);\n> }\n>\n> static struct command **queue_command(struct command **tail,\n> --\n> 2.2.1.62.g3f15098\n"},{"id":"254190","messageId":"CAGZ79kZ_yiA7tMv49qp3OOMfOmwdaDtWs_Gt9V359xCKUE_SJg@mail.gmail.com","threadId":"38262","inReplyTo":"CAPig+cQ7GhrZqXH3zRYTWjwuWKwDNNwLRKG516dXS6et2TURgw@mail.gmail.com","subject":"Re: [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T08:33:22Z","receivedAt":"2014-12-30T08:33:22Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 29, 2014 at 11:08 PM, Eric Sunshine wrote:\n> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>> Subject: receive-pack.c: add documentation for atomic push support\n>\n> This patch is doing a lot more than merely adding documentation. It's\n> also updating send-pack and receive-pack to be able to negotiate the\n> new protocol capability \"atomic\". The fact that you removed the actual\n> advertisement of \"atomic\" from this patch doesn't turn it into a\n> documentation-only patch.\n\nThat's true, though the code itself is dead code and doing nothing in\nthat patch.\n\n>\n> When Michael raised the issue of the server being \"broken\" due to\n> advertising a capability which it does not yet implement, his\n> recommendation[1] was to reorder the patches, not to split out the one\n> tiny bit (capability advertisement) from the larger change. Was there\n> an insurmountable conflict which prevented you from reordering the\n> patches? This is a genuine question since splitting off advertisement\n> -- and only advertisement -- to a patch later in the series smells\n> like a least-resistance approach, rather than a proper solution to the\n> issue Michael raised.\n\nWell there was no syntactical problem (i.e. the interactive rebase\nwent flawless),\nbut rather a semantic dependency. The reordered patches would not compile\nas we'd heavily depend on the use_atomic variable.\n\nOf course that could have been introduced where required, but at the\ntime it did\nnot look appealing to me.\n\nI'll reword the commit message header to mention the negotiation part as well.\n\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/261793\n>\n>> This documents the protocol option 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. The capability is also not yet advertised\n>> by receive-pack as git doesn't know how to handle it yet.\n>>\n>> Signed-off-by: Ronnie Sahlberg \n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\n>> index 6d5424c..4f8a7bf 100644\n>> --- a/Documentation/technical/protocol-capabilities.txt\n>> +++ b/Documentation/technical/protocol-capabilities.txt\n>> @@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client requested\n>> and server advertised. As a consequence of these rules, server MUST\n>> NOT advertise capabilities it does not understand.\n>>\n>> -The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities\n>> -are sent and recognized by the receive-pack (push to server) process.\n>> +The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n>> +capabilities are sent and recognized by the receive-pack (push to server)\n>> +process.\n>>\n>> The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized\n>> by both upload-pack and receive-pack protocols. The 'agent' capability\n>> @@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress\n>> reporting if the local progress reporting is also being suppressed\n>> (e.g., via `push -q`, or if stderr does not go to a tty).\n>>\n>> +atomic\n>> +------\n>> +\n>> +If the server sends the 'atomic' capability it is capable of accepting\n>> +atomic pushes. If the pushing client requests this capability, the server\n>> +will update the refs in one atomic transaction. Either all refs are\n>> +updated or none.\n>> +\n>> allow-tip-sha1-in-want\n>> ----------------------\n>>\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 32fc540..4e8eaf7 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>> @@ -1179,6 +1180,8 @@ static struct command *read_head_info(struct sha1_array *shallow)\n>> use_sideband = LARGE_PACKET_MAX;\n>> if (parse_feature_request(feature_list, \"quiet\"))\n>> quiet = 1;\n>> + if (parse_feature_request(feature_list, \"atomic\"))\n>> + use_atomic = 1;\n>> }\n>>\n>> if (!strcmp(line, \"push-cert\")) {\n>> diff --git a/send-pack.c b/send-pack.c\n>> index 949cb61..6d0c159 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>> int use_sideband = 0;\n>> int quiet_supported = 0;\n>> int agent_supported = 0;\n>> + int use_atomic = 0;\n>> + int atomic_supported = 0;\n>> unsigned cmds_sent = 0;\n>> int ret;\n>> struct async demux;\n>> @@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,\n>> agent_supported = 1;\n>> if (server_supports(\"no-thin\"))\n>> args->use_thin_pack = 0;\n>> + if (server_supports(\"atomic\"))\n>> + atomic_supported = 1;\n>> if (args->push_cert) {\n>> int len;\n>>\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>> + die(_(\"server does not support --atomic push\"));\n>> +\n>> + use_atomic = atomic_supported && args->atomic;\n>>\n>> if (status_report)\n>> strbuf_addstr(&cap_buf, \" report-status\");\n>> @@ -335,6 +343,8 @@ int send_pack(struct send_pack_args *args,\n>> strbuf_addstr(&cap_buf, \" side-band-64k\");\n>> if (quiet_supported && (args->quiet || !args->progress))\n>> strbuf_addstr(&cap_buf, \" quiet\");\n>> + if (use_atomic)\n>> + strbuf_addstr(&cap_buf, \" atomic\");\n>> if (agent_supported)\n>> strbuf_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>> use_ofs_delta:1,\n>> dry_run:1,\n>> push_cert:1,\n>> - stateless_rpc:1;\n>> + stateless_rpc:1,\n>> + atomic:1;\n>> };\n>>\n>> int send_pack(struct send_pack_args *args,\n>> --\n>> 2.2.1.62.g3f15098\n"},{"id":"254191","messageId":"CAPig+cSYD+gBdEZ9TzWdeTEufzH6eJTbt=ZVS5imMJGjWnUFPA@mail.gmail.com","threadId":"38262","inReplyTo":"1419907007-19387-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T08:36:55Z","receivedAt":"2014-12-30T08:36:55Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n> Subject: receive-pack.c: move transaction handling in a central place\n\nThis is very generic, and doesn't really explain what this patch is\nabout. (See below.)\n\n> No functional changes intended.\n\nSecondary information can be demoted to the end of the commit message.\n\n> This moves all code related to transactions into the execute_commands_loop\n> function which was factored out of execute_commands. This includes\n> beginning and committing the transaction as well as dealing with the\n> errors which may occur during the begin and commit phase of a transaction.\n\nThis explains what you're doing, but not why. The purpose of this\nchange is that a subsequent patch will be adding another mode of\noperation (\"atomic\") to execute_commands() which differs from the\nexisting mode (\"non-atomic\") implemented by its main loop. In its\nhigh-level role, execute_commands() does not need to know or care\nabout the low-level details of each mode of operation. Therefore, as\npreparation for introducing a new mode, you're factoring out the\nexisting mode into its own stand-alone function.\n\n> Helped-by: Eric Sunshine \n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\n> This covers the suggestion of patch 2 and 3 by Eric\n> > patch 2: Factor out the main 'for' loop of execute_commands() into a\n> > new function. This new function will eventually become\n> > execute_commands_non_atomic(). At this point, execute_commands() is\n> > pretty much in its final form with the exception of the upcoming 'if\n> > (use_atomic)' conditional.\n> > patch 3: Morph the function extracted in patch 2 into\n> > execute_commands_non_atomic() by adding transaction handling inside\n> > the 'for' loop (and applying the changes from the early part of the\n> > patch which go along with that).\n\nThis patch is still rather heavyweight. My suggestion[1] for making\nthese particular changes across two patches was quite deliberate. The\nproblem with combining them into a single patch is that you're\nperforming both code movement and functional changes at the same time.\n\nOn its own, pure code movement is easy to review.\n\nOn its own, code changes are as easy or difficult to review as the\nchanges themselves.\n\nWhen combined, however, the review effort is greater than the sum of\nthe efforts of reviewing them separately. Partly this is because the\ncombined changes have a noisier diff. If you move the code in one\npatch, and then change it in a second one, the changes pop out --\nthey're quite obvious. On the other hand, when they are combined, the\nreviewer has to deliberately and painstakingly search out the changes,\nwhich is difficult and error-prone. Combining movement and code\nchanges into a single patch also places greater cognitive load on the\nreviewer due to the necessity of keeping a more complex mental\nscoreboard relating to the different types of changes.\n\nMore below.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/261706\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 06eb287..5f44466 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,\n> \"the reported refs above\");\n> }\n>\n> +static void execute_commands_loop(struct command *commands,\n> + struct shallow_info *si)\n\nStyle: Indent the wrapped line to align with the text following the\n'(' in the first line.\n\nIt's safe to say that the code which you extracted from\nexecute_commands() handled the non-atomic case, and it's safe to say\nthat this new function implements the non-atomic case. Therefore, it\nwould be truthful to call this function execute_commands_nonatomic().\nNo need to invent the name execute_commands_loop().\n\n> +{\n> + struct command *cmd;\n> + struct strbuf err = STRBUF_INIT;\n> +\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (!should_process_cmd(cmd))\n> + continue;\n> +\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_reset(&err);\n> + cmd->error_string = \"transaction failed to start\";\n> + continue;\n> + }\n> +\n> + cmd->error_string = update(cmd, si);\n> +\n> + if (!cmd->error_string\n> + && ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_reset(&err);\n> + cmd->error_string = \"failed to update ref\";\n> + }\n> + ref_transaction_free(transaction);\n> + }\n> +\n> + strbuf_release(&err);\n> +}\n> +\n> static void execute_commands(struct command *commands,\n> const char *unpacker_error,\n> struct shallow_info *si)\n> @@ -1107,12 +1142,8 @@ static void execute_commands(struct command *commands,\n> free(head_name_to_free);\n> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>\n> - for (cmd = commands; cmd; cmd = cmd->next) {\n> - if (!should_process_cmd(cmd))\n> - continue;\n> + execute_commands_loop(commands, si);\n>\n> - cmd->error_string = update(cmd, si);\n> - }\n> check_shallow_bugs(commands, si);\n> }\n>\n> --\n> 2.2.1.62.g3f15098\n"},{"id":"254192","messageId":"CAGZ79kbthoUbeHEGUv4QSGwAsbLjEPhFFLgGMi++3iPqU5bvyQ@mail.gmail.com","threadId":"38262","inReplyTo":"CAPig+cT9JVkDvUUsbx9HW8HpakCb9SkoQf3LJZc4h3TQTt2ZXQ@mail.gmail.com","subject":"Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T08:41:26Z","receivedAt":"2014-12-30T08:41:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"reply to all instead of reply to Eric only.\n\nOn Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine wrote:\n> In v6, you considered this a fatal error in the atomic case, which\n> caused the entire transaction to be rolled back. However, in this\n> version, this error has no effect whatsoever on the atomic\n> transaction, which is a rather significant behavioral departure. Which\n> is correct? (This is a genuine question; not at all rhetorical.) If\n> failing the entire transaction is the correct thing to do, then this\n> is going to need some more work.\n\nI don't know. in v6 I thought *any* error would stop and abort the atomic\ncommit. An atomic commit is either completely free of failures or it\ndoesn't work out.\n\nHowever that warning doesn't seem to have any effect as of now\n(apart from warning the user obviously), and is about the (highly\nunlikely?) case of\na bug in git (or a provided helper script/hook?), I thought we can go\nwith it as well.\nThe transaction wen't through so all is fine. The thing the warning is\nabout is about\nreachability checks for the shallow case, so I don't see why that\nshould fail the transaction.\n"},{"id":"254193","messageId":"CAGZ79kYvv_r8TahfoE5g_5nBQhjsAC3tHvxUUBE4PtKLy8sGhQ@mail.gmail.com","threadId":"38262","inReplyTo":"CAPig+cQEO+LKZJDzE-sUmo+td=djsOdjx-ouQEMRTTkOBMwzjA@mail.gmail.com","subject":"Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T08:42:10Z","receivedAt":"2014-12-30T08:42:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 29, 2014 at 11:46 PM, Eric Sunshine wrote:\n> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>> No functional changes intended.\n>>\n>> This commit shortens execute_commands by moving some parts of the code\n>> to extra functions.\n>>\n>> Suggested-by: Eric Sunshine \n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 4e8eaf7..06eb287 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)\n>> }\n>> }\n>>\n>> +static void check_shallow_bugs(struct command *commands,\n>> + struct shallow_info *si)\n>> +{\n>> + struct command *cmd;\n>> + int checked_connectivity = 1;\n>> + for (cmd = commands; cmd; cmd = cmd->next) {\n>> + if (!should_process_cmd(cmd))\n>> + continue;\n>> +\n>> + if (shallow_update && si->shallow_ref[cmd->index]) {\n>\n> Another issue: In the original code, 'si->shallow_ref[cmd->index]' is\n> only checked if cmd->error_string is NULL, but here you check it\n> unconditionally, despite the commit message claiming no functional\n> changes. Did you verify that such a behavioral change is benign? (This\n> is a genuine question.)\n\nThe error != NULL check is done in if (!should_process_cmd(cmd))\n\n>\n>> + error(\"BUG: connectivity check has not been run on ref %s\",\n>> + cmd->ref_name);\n>> + checked_connectivity = 0;\n>> + }\n>> + }\n>> + if (shallow_update && !checked_connectivity)\n>> + error(\"BUG: run 'git fsck' for safety.\\n\"\n>> + \"If there are errors, try to remove \"\n>> + \"the reported refs above\");\n>> +}\n>> +\n>> static void execute_commands(struct command *commands,\n>> const char *unpacker_error,\n>> struct shallow_info *si)\n>> {\n>> - int checked_connectivity;\n>> struct command *cmd;\n>> unsigned char sha1[20];\n>> struct iterate_data data;\n>> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,\n>> free(head_name_to_free);\n>> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>>\n>> - checked_connectivity = 1;\n>> for (cmd = commands; cmd; cmd = cmd->next) {\n>> - if (cmd->error_string)\n>> - continue;\n>> -\n>> - if (cmd->skip_update)\n>> + if (!should_process_cmd(cmd))\n>> continue;\n>>\n>> cmd->error_string = update(cmd, si);\n>> - if (shallow_update && !cmd->error_string &&\n>> - si->shallow_ref[cmd->index]) {\n>> - error(\"BUG: connectivity check has not been run on ref %s\",\n>> - cmd->ref_name);\n>> - checked_connectivity = 0;\n>> - }\n>> }\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>> + check_shallow_bugs(commands, si);\n>> }\n>>\n>> static struct command **queue_command(struct command **tail,\n>> --\n>> 2.2.1.62.g3f15098\n"},{"id":"254194","messageId":"CAPig+cTY3UQsOEut7mqUze1or1i3Hx9=2inrToOFrRKEo08=XQ@mail.gmail.com","threadId":"38262","inReplyTo":"1419907007-19387-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T08:57:52Z","receivedAt":"2014-12-30T08:57:52Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n> Update receive-pack to use an atomic transaction iff the client negotiated\n> that it wanted atomic push.\n\nThis first line seems germane to this patch...\n\n> 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\nHowever, the remainder feels like it belongs with some other patch,\nsuch as a patch which introduces an --atomic option.\n\n> Inspired-by: Ronnie Sahlberg \n> Helped-by: Eric Sunshine \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 5f44466..35a2264 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1076,8 +1076,8 @@ static void check_shallow_bugs(struct command *commands,\n> \"the reported refs above\");\n> }\n>\n> -static void execute_commands_loop(struct command *commands,\n> - struct shallow_info *si)\n> +static void execute_commands_non_atomic(struct command *commands,\n> + struct shallow_info *si)\n\nStyle: Indent the wrapped line to align with the text following the\n'(' in the first line.\n\n> {\n> struct command *cmd;\n> struct strbuf err = STRBUF_INIT;\n> @@ -1104,7 +1104,50 @@ static void execute_commands_loop(struct command *commands,\n> }\n> ref_transaction_free(transaction);\n> }\n> + strbuf_release(&err);\n> +}\n> +\n> +static void execute_commands_atomic(struct command *commands,\n> + struct shallow_info *si)\n\nStyle: Indentation.\n\n> +{\n> + struct command *cmd;\n> + struct strbuf err = STRBUF_INIT;\n> + const char *reported_error = \"atomic push failure\";\n> +\n> + transaction = ref_transaction_begin(&err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_reset(&err);\n> + reported_error = \"transaction failed to start\";\n> + goto failure;\n> + }\n> +\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (!should_process_cmd(cmd))\n> + continue;\n>\n> + cmd->error_string = update(cmd, si);\n> +\n> + if (cmd->error_string)\n> + goto failure;\n> + }\n> +\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + reported_error = \"atomic transaction failed\";\n> + goto failure;\n> + }\n> +\n> + ref_transaction_free(transaction);\n> + strbuf_release(&err);\n> + return;\n\nMinor comment: This cleanup code is repeated in both the success and\nfail branches. It _might_ (or not) be a bit cleaner and more\nmaintainable to replace the above three lines with:\n\n goto done;\n\n> +\n> +failure:\n> + for (cmd = commands; cmd; cmd = cmd->next)\n> + if (!cmd->error_string)\n> + cmd->error_string = reported_error;\n\nAnd add a 'done' label here:\n\n done:\n\n> + ref_transaction_free(transaction);\n> strbuf_release(&err);\n> }\n>\n> @@ -1142,7 +1185,10 @@ static void execute_commands(struct command *commands,\n> free(head_name_to_free);\n> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>\n> - execute_commands_loop(commands, si);\n> + if (use_atomic)\n> + execute_commands_atomic(commands, si);\n> + else\n> + execute_commands_non_atomic(commands, si);\n>\n> check_shallow_bugs(commands, si);\n> }\n> --\n> 2.2.1.62.g3f15098\n"},{"id":"254195","messageId":"CAPig+cSuf0yXE1BE=tCTDrcxRMEZ_pU9VU_Q9F+Jxk-xNrPdgg@mail.gmail.com","threadId":"38262","inReplyTo":"CAGZ79kZ_yiA7tMv49qp3OOMfOmwdaDtWs_Gt9V359xCKUE_SJg@mail.gmail.com","subject":"Re: [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T09:09:54Z","receivedAt":"2014-12-30T09:09:54Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 30, 2014 at 3:33 AM, Stefan Beller wrote:\n> On Mon, Dec 29, 2014 at 11:08 PM, Eric Sunshine wrote:\n>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>>> Subject: receive-pack.c: add documentation for atomic push support\n>> When Michael raised the issue of the server being \"broken\" due to\n>> advertising a capability which it does not yet implement, his\n>> recommendation[1] was to reorder the patches, not to split out the one\n>> tiny bit (capability advertisement) from the larger change. Was there\n>> an insurmountable conflict which prevented you from reordering the\n>> patches? This is a genuine question since splitting off advertisement\n>> -- and only advertisement -- to a patch later in the series smells\n>> like a least-resistance approach, rather than a proper solution to the\n>> issue Michael raised.\n>\n> Well there was no syntactical problem (i.e. the interactive rebase\n> went flawless),\n> but rather a semantic dependency. The reordered patches would not compile\n> as we'd heavily depend on the use_atomic variable.\n>\n> Of course that could have been introduced where required, but at the\n> time it did\n> not look appealing to me.\n>\n> I'll reword the commit message header to mention the negotiation part as well.\n\nThis still smells like a least-resistance (lazy) approach. You\nshouldn't need the stand-alone patch enabling \"atomic\" advertisement\njust to address Michael's concern. His suggestion[1] to reorder the\npatches sounds sensible and correct. Toward that end, moving the\ndeclaration of 'use_atomic' to the patch where it is first needed\nmakes sense, even if the machinery for manipulating that variable\narrives with a later patch.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/261793\n"},{"id":"254196","messageId":"CAPig+cQEfGj8dH+FFOR4UYbNgovhFy_pP8szSpS+0MyNdOvJrw@mail.gmail.com","threadId":"38262","inReplyTo":"CAGZ79kYvv_r8TahfoE5g_5nBQhjsAC3tHvxUUBE4PtKLy8sGhQ@mail.gmail.com","subject":"Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T09:10:08Z","receivedAt":"2014-12-30T09:10:08Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 30, 2014 at 3:42 AM, Stefan Beller wrote:\n> On Mon, Dec 29, 2014 at 11:46 PM, Eric Sunshine wrote:\n>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>>> No functional changes intended.\n>>> +static void check_shallow_bugs(struct command *commands,\n>>> + struct shallow_info *si)\n>>> +{\n>>> + struct command *cmd;\n>>> + int checked_connectivity = 1;\n>>> + for (cmd = commands; cmd; cmd = cmd->next) {\n>>> + if (!should_process_cmd(cmd))\n>>> + continue;\n>>> +\n>>> + if (shallow_update && si->shallow_ref[cmd->index]) {\n>>\n>> Another issue: In the original code, 'si->shallow_ref[cmd->index]' is\n>> only checked if cmd->error_string is NULL, but here you check it\n>> unconditionally, despite the commit message claiming no functional\n>> changes. Did you verify that such a behavioral change is benign? (This\n>> is a genuine question.)\n>\n> The error != NULL check is done in if (!should_process_cmd(cmd))\n\nRight. Thanks for setting me straight on that.\n"},{"id":"254208","messageId":"CAGZ79kYufUcj858HGjJPTDGJgmTRv-YVR30zOtAURpP00NWEeQ@mail.gmail.com","threadId":"38262","inReplyTo":"CAPig+cSYD+gBdEZ9TzWdeTEufzH6eJTbt=ZVS5imMJGjWnUFPA@mail.gmail.com","subject":"Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T18:45:32Z","receivedAt":"2014-12-30T18:45:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 30, 2014 at 12:36 AM, Eric Sunshine wrote:\n> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>> Subject: receive-pack.c: move transaction handling in a central place\n>\n> This is very generic, and doesn't really explain what this patch is\n> about. (See below.)\n>\n>> No functional changes intended.\n>\n> Secondary information can be demoted to the end of the commit message.\n\nI think this would help in case there is a subtle bug introduced with\nsuch a commit.\nIf you bisect it 2 years later and then ask yourself if that subtle\nbehavior was\nintentional (can easily happen if the commit message is\nshort/unclear). This would then\ntell you it's definitely a bug introduced. I believe to have seen such\na comment\nsomewhere in the history, but I cannot find it again. I'll drop it\ninto the notes for now.\n\n>\n>> This moves all code related to transactions into the execute_commands_loop\n>> function which was factored out of execute_commands. This includes\n>> beginning and committing the transaction as well as dealing with the\n>> errors which may occur during the begin and commit phase of a transaction.\n>\n> This explains what you're doing, but not why. The purpose of this\n> change is that a subsequent patch will be adding another mode of\n> operation (\"atomic\") to execute_commands() which differs from the\n> existing mode (\"non-atomic\") implemented by its main loop. In its\n> high-level role, execute_commands() does not need to know or care\n> about the low-level details of each mode of operation. Therefore, as\n> preparation for introducing a new mode, you're factoring out the\n> existing mode into its own stand-alone function.\n>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> Notes:\n>> This covers the suggestion of patch 2 and 3 by Eric\n>> > patch 2: Factor out the main 'for' loop of execute_commands() into a\n>> > new function. This new function will eventually become\n>> > execute_commands_non_atomic(). At this point, execute_commands() is\n>> > pretty much in its final form with the exception of the upcoming 'if\n>> > (use_atomic)' conditional.\n>> > patch 3: Morph the function extracted in patch 2 into\n>> > execute_commands_non_atomic() by adding transaction handling inside\n>> > the 'for' loop (and applying the changes from the early part of the\n>> > patch which go along with that).\n>\n> This patch is still rather heavyweight. My suggestion[1] for making\n> these particular changes across two patches was quite deliberate. The\n> problem with combining them into a single patch is that you're\n> performing both code movement and functional changes at the same time.\n>\n> On its own, pure code movement is easy to review.\n>\n> On its own, code changes are as easy or difficult to review as the\n> changes themselves.\n\nI need to hammer this mantra in my brain. Sorry for messing this up again.\n\n>\n> When combined, however, the review effort is greater than the sum of\n> the efforts of reviewing them separately. Partly this is because the\n> combined changes have a noisier diff. If you move the code in one\n> patch, and then change it in a second one, the changes pop out --\n> they're quite obvious. On the other hand, when they are combined, the\n> reviewer has to deliberately and painstakingly search out the changes,\n> which is difficult and error-prone. Combining movement and code\n> changes into a single patch also places greater cognitive load on the\n> reviewer due to the necessity of keeping a more complex mental\n> scoreboard relating to the different types of changes.\n>\n> More below.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/261706\n>\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 06eb287..5f44466 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,\n>> \"the reported refs above\");\n>> }\n>>\n>> +static void execute_commands_loop(struct command *commands,\n>> + struct shallow_info *si)\n>\n> Style: Indent the wrapped line to align with the text following the\n> '(' in the first line.\n\nThat's true. I have found this problem myself at another patch by\nMichael lately.\nIf you apply the patch it is correctly aligned. If you view the patch\nhowever it is missaligned.\nBecause of the leading plus sign the line in which the function\nsignature starts is\nindented by one character. The other lines starting with a tab indent\nonly to 8 character.\n\nThis would explain a difference of only a few chars, but not so many.\nI'll check the indentation here.\n\n>\n> It's safe to say that the code which you extracted from\n> execute_commands() handled the non-atomic case, and it's safe to say\n> that this new function implements the non-atomic case. Therefore, it\n> would be truthful to call this function execute_commands_nonatomic().\n> No need to invent the name execute_commands_loop().\n\nok.\n\n>\n>> +{\n>> + struct command *cmd;\n>> + struct strbuf err = STRBUF_INIT;\n>> +\n>> + for (cmd = commands; cmd; cmd = cmd->next) {\n>> + if (!should_process_cmd(cmd))\n>> + continue;\n>> +\n>> + transaction = ref_transaction_begin(&err);\n>> + if (!transaction) {\n>> + rp_error(\"%s\", err.buf);\n>> + strbuf_reset(&err);\n>> + cmd->error_string = \"transaction failed to start\";\n>> + continue;\n>> + }\n>> +\n>> + cmd->error_string = update(cmd, si);\n>> +\n>> + if (!cmd->error_string\n>> + && ref_transaction_commit(transaction, &err)) {\n>> + rp_error(\"%s\", err.buf);\n>> + strbuf_reset(&err);\n>> + cmd->error_string = \"failed to update ref\";\n>> + }\n>> + ref_transaction_free(transaction);\n>> + }\n>> +\n>> + strbuf_release(&err);\n>> +}\n>> +\n>> static void execute_commands(struct command *commands,\n>> const char *unpacker_error,\n>> struct shallow_info *si)\n>> @@ -1107,12 +1142,8 @@ static void execute_commands(struct command *commands,\n>> free(head_name_to_free);\n>> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>>\n>> - for (cmd = commands; cmd; cmd = cmd->next) {\n>> - if (!should_process_cmd(cmd))\n>> - continue;\n>> + execute_commands_loop(commands, si);\n>>\n>> - cmd->error_string = update(cmd, si);\n>> - }\n>> check_shallow_bugs(commands, si);\n>> }\n>>\n>> --\n>> 2.2.1.62.g3f15098\n"},{"id":"254216","messageId":"CAGZ79kYds-oo-Q0=R6ny=CpZ3ja0__6CDm-wmamnSp3TBWcKdQ@mail.gmail.com","threadId":"38262","inReplyTo":"CAPig+cT9JVkDvUUsbx9HW8HpakCb9SkoQf3LJZc4h3TQTt2ZXQ@mail.gmail.com","subject":"Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-30T20:33:48Z","receivedAt":"2014-12-30T20:33:48Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine wrote:\n> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>> No functional changes intended.\n>\n> This is useful to know but is of secondary importance, thus should be\n> placed after the explanation and justification of the change.\n>\n>> Subject: receive-pack.c: simplify execute_commands\n>> This commit shortens execute_commands by moving some parts of the code\n>> to extra functions.\n>\n> The _real_ reason for moving these bits of code into their own\n> functions is that you intend to introduce additional callers in\n> subsequent patches. That is what the commit message (including\n> subject) should be conveying to the reader.\n>\n> The claimed simplification is questionable and not of particular\n> importance; and it's misleading to paint it as a goal of the patch.\n> Consequently, you could drop mention of it altogether.\n>\n> More below.\n>\n>> Suggested-by: Eric Sunshine \n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 4e8eaf7..06eb287 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)\n>> }\n>> }\n>>\n>> +static int should_process_cmd(struct command *cmd)\n>> +{\n>> + if (cmd->error_string)\n>> + return 0;\n>> + if (cmd->skip_update)\n>> + return 0;\n>> + return 1;\n>\n> Alternately, depending upon the polarity of your brain, you could\n> collapse the entire function body to:\n>\n> return !cmd->error_string && !cmd->skip_update;\n>\n> or:\n>\n> return !(cmd->error_string || cmd->skip_update);\n\nI did not want to change the structure of the code when moving it around as\nyou noted in another email that helps on reviewing.\nAnd having the exact same conditions in an if and then return instead\nof continue\nmakes me think it is easier to review than if I also introduce that shortening?\nI can do so in a follow up.\n\n>\n>> +}\n>> +\n>> +static void check_shallow_bugs(struct command *commands,\n>> + struct shallow_info *si)\n>> +{\n>> + struct command *cmd;\n>> + int checked_connectivity = 1;\n>> + for (cmd = commands; cmd; cmd = cmd->next) {\n>> + if (!should_process_cmd(cmd))\n>> + continue;\n>> +\n>> + if (shallow_update && si->shallow_ref[cmd->index]) {\n>\n> Here, inside the loop, you check 'shallow_update'...\n>\n>> + error(\"BUG: connectivity check has not been run on ref %s\",\n>> + cmd->ref_name);\n>> + checked_connectivity = 0;\n>> + }\n>> + }\n>> + if (shallow_update && !checked_connectivity)\n>\n> And here, after the loop, you check 'shallow_update'.\n>\n> But, if you examine the overall logic, you will find that this\n> function does _nothing_ at all when 'shallow_update' is false (other\n> than uselessly looping through 'commands'). Therefore, either check\n> 'shallow_update' just once at the beginning of the function and exit\n> early if false, or have the caller check 'shallow_update' and only\n> invoke check_shallow_bugs() if true.\n>\n> Also, since nothing happens between them, the two conditionals inside\n> the loop can be coalesced:\n>\n> if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {\n>\n>> + error(\"BUG: run 'git fsck' for safety.\\n\"\n>> + \"If there are errors, try to remove \"\n>> + \"the reported refs above\");\n\nThat's a good idea. I'll definitely add that. But as said above, I\nalso feel that this\nshould rather go in a follow up patch as it twists the logic and this\npatch is all\nabout moving the code.\n\n>\n> In v6, you considered this a fatal error in the atomic case, which\n> caused the entire transaction to be rolled back. However, in this\n> version, this error has no effect whatsoever on the atomic\n> transaction, which is a rather significant behavioral departure. Which\n> is correct? (This is a genuine question; not at all rhetorical.) If\n> failing the entire transaction is the correct thing to do, then this\n> is going to need some more work.\n>\n>> +}\n>> +\n>> static void execute_commands(struct command *commands,\n>> const char *unpacker_error,\n>> struct shallow_info *si)\n>> {\n>> - int checked_connectivity;\n>> struct command *cmd;\n>> unsigned char sha1[20];\n>> struct iterate_data data;\n>> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,\n>> free(head_name_to_free);\n>> head_name = head_name_to_free = resolve_refdup(\"HEAD\", 0, sha1, NULL);\n>>\n>> - checked_connectivity = 1;\n>> for (cmd = commands; cmd; cmd = cmd->next) {\n>> - if (cmd->error_string)\n>> - continue;\n>> -\n>> - if (cmd->skip_update)\n>> + if (!should_process_cmd(cmd))\n>> continue;\n>>\n>> cmd->error_string = update(cmd, si);\n>> - if (shallow_update && !cmd->error_string &&\n>> - si->shallow_ref[cmd->index]) {\n>> - error(\"BUG: connectivity check has not been run on ref %s\",\n>> - cmd->ref_name);\n>> - checked_connectivity = 0;\n>> - }\n>> }\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>> + check_shallow_bugs(commands, si);\n>> }\n>>\n>> static struct command **queue_command(struct command **tail,\n>> --\n>> 2.2.1.62.g3f15098\n"},{"id":"254217","messageId":"CAPig+cRWLKKw-AX_YqsKxLNUUCyMDmm_9AzkhUJGaYQhFmi=pQ@mail.gmail.com","threadId":"38262","inReplyTo":"CAGZ79kYufUcj858HGjJPTDGJgmTRv-YVR30zOtAURpP00NWEeQ@mail.gmail.com","subject":"Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T20:33:54Z","receivedAt":"2014-12-30T20:33:54Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 30, 2014 at 1:45 PM, Stefan Beller wrote:\n> On Tue, Dec 30, 2014 at 12:36 AM, Eric Sunshine wrote:\n>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>>> No functional changes intended.\n>>\n>> Secondary information can be demoted to the end of the commit message.\n>\n> I think this would help in case there is a subtle bug introduced with\n> such a commit.\n> If you bisect it 2 years later and then ask yourself if that subtle\n> behavior was\n> intentional (can easily happen if the commit message is\n> short/unclear). This would then\n> tell you it's definitely a bug introduced. I believe to have seen such\n> a comment\n> somewhere in the history, but I cannot find it again. I'll drop it\n> into the notes for now.\n\nI think you misread or misunderstood my comment, which was that\nsecondary information should follow primary information in the commit\nmessage. I did not suggest moving it below the \"---\" line.\n\n\"No functional changes intended\" is meaningful and belongs in the\ncommit message, but it's not as important as the explanation and\njustification of the change.\n\n>>> +static void execute_commands_loop(struct command *commands,\n>>> + struct shallow_info *si)\n>>\n>> Style: Indent the wrapped line to align with the text following the\n>> '(' in the first line.\n>\n> That's true. I have found this problem myself at another patch by\n> Michael lately.\n> If you apply the patch it is correctly aligned. If you view the patch\n> however it is missaligned.\n> Because of the leading plus sign the line in which the function\n> signature starts is\n> indented by one character. The other lines starting with a tab indent\n> only to 8 character.\n\nYou're right. This one does align properly. Sorry for the noise.\n\n(Of the two pointed out in patch 6/9, the first one also aligns\ncorrectly; but the second does not.)\n"},{"id":"254222","messageId":"CAPig+cS0RYgmMzhkgMj=G939DADNG29dPQP2BQJQxVYS5d6ThQ@mail.gmail.com","threadId":"38262","inReplyTo":"CAGZ79kYds-oo-Q0=R6ny=CpZ3ja0__6CDm-wmamnSp3TBWcKdQ@mail.gmail.com","subject":"Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-30T21:15:10Z","receivedAt":"2014-12-30T21:15:10Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 30, 2014 at 3:33 PM, Stefan Beller wrote:\n> On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine wrote:\n>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller wrote:\n>>> +static int should_process_cmd(struct command *cmd)\n>>> +{\n>>> + if (cmd->error_string)\n>>> + return 0;\n>>> + if (cmd->skip_update)\n>>> + return 0;\n>>> + return 1;\n>>\n>> Alternately, depending upon the polarity of your brain, you could\n>> collapse the entire function body to:\n>>\n>> return !cmd->error_string && !cmd->skip_update;\n>>\n>> or:\n>>\n>> return !(cmd->error_string || cmd->skip_update);\n>\n> I did not want to change the structure of the code when moving it around as\n> you noted in another email that helps on reviewing.\n> And having the exact same conditions in an if and then return instead\n> of continue\n> makes me think it is easier to review than if I also introduce that shortening?\n> I can do so in a follow up.\n\nIt's a judgment call. When composing this review, I actually had a\nparenthetical comment here saying that collapsing the body of the\nfunction (as illustrated above) could be done as a patch separate from\nthe code movement, but I removed the comment just before sending the\nemail. This is such a simple change that the cognitive overhead from\ncombining the code movement and simplification is so slight that it\ndoesn't necessary warrant burdening reviewers with yet another patch.\nTherefore, although I very slightly favor splitting it into two\npatches, in this particular case, a single patch is not a great\nburden, so I don't feel strongly one way or the other. Use your best\njudgment.\n\n>>> +}\n>>> +\n>>> +static void check_shallow_bugs(struct command *commands,\n>>> + struct shallow_info *si)\n>>> +{\n>>> + struct command *cmd;\n>>> + int checked_connectivity = 1;\n>>> + for (cmd = commands; cmd; cmd = cmd->next) {\n>>> + if (!should_process_cmd(cmd))\n>>> + continue;\n>>> +\n>>> + if (shallow_update && si->shallow_ref[cmd->index]) {\n>>\n>> Here, inside the loop, you check 'shallow_update'...\n>>\n>>> + error(\"BUG: connectivity check has not been run on ref %s\",\n>>> + cmd->ref_name);\n>>> + checked_connectivity = 0;\n>>> + }\n>>> + }\n>>> + if (shallow_update && !checked_connectivity)\n>>\n>> And here, after the loop, you check 'shallow_update'.\n>>\n>> But, if you examine the overall logic, you will find that this\n>> function does _nothing_ at all when 'shallow_update' is false (other\n>> than uselessly looping through 'commands'). Therefore, either check\n>> 'shallow_update' just once at the beginning of the function and exit\n>> early if false, or have the caller check 'shallow_update' and only\n>> invoke check_shallow_bugs() if true.\n>\n> That's a good idea. I'll definitely add that. But as said above, I\n> also feel that this\n> should rather go in a follow up patch as it twists the logic and this\n> patch is all\n> about moving the code.\n\nHere, too, when composing the review, I had a parenthetical comment\nstating that this improvement to the code might warrant a followup\npatch after the code movement patch. However, this case is a bit\ndifferent because the combined code in the new function, although not\nactively wrong, is uselessly iterating a do-nothing loop when\n'shallow_update' is false, and even though not actively incorrect, it\n_feels_ weird to commit such pointless code, only to fix it up later.\nDue to that \"weird\" feeling, I didn't feel entirely comfortable\nrecommending splitting the change into two patches, so I removed the\ncomment before sending the email. Hence, this again is judgment call.\nTry to find a balance which feels right, while taking the review\nprocess into consideration.\n"}]}