From a6a843196869659ee55238e474be29018987c4bf Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Wed, 7 Jan 2015 19:23:15 -0800 Subject: receive-pack.c: shorten the execute_commands loop over all commands Make the main "execute_commands" loop in receive-pack easier to read by splitting out some steps into helper functions. The new helper 'should_process_cmd' checks if a ref update is unnecessary, whether due to an error having occurred or for another reason. The helper 'warn_if_skipped_connectivity_check' warns if we have forgotten to run a connectivity check on a ref which is shallow for the client which would be a bug. This will help us to duplicate less code in a later patch when we make a second copy of the "execute_commands" loop. No functional change intended. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) (limited to 'builtin/receive-pack.c') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 32fc540ef3..2ebaf6695f 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1042,11 +1042,34 @@ static void reject_updates_to_hidden(struct command *commands) } } +static int should_process_cmd(struct command *cmd) +{ + return !cmd->error_string && !cmd->skip_update; +} + +static void warn_if_skipped_connectivity_check(struct command *commands, + struct shallow_info *si) +{ + struct command *cmd; + int checked_connectivity = 1; + + for (cmd = commands; cmd; cmd = cmd->next) { + if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) { + error("BUG: connectivity check has not been run on ref %s", + cmd->ref_name); + checked_connectivity = 0; + } + } + if (!checked_connectivity) + error("BUG: run 'git fsck' for safety.\n" + "If there are errors, try to remove " + "the reported refs above"); +} + static void execute_commands(struct command *commands, const char *unpacker_error, struct shallow_info *si) { - int checked_connectivity; struct command *cmd; unsigned char sha1[20]; struct iterate_data data; @@ -1077,27 +1100,15 @@ static void execute_commands(struct command *commands, free(head_name_to_free); head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL); - checked_connectivity = 1; for (cmd = commands; cmd; cmd = cmd->next) { - if (cmd->error_string) - continue; - - if (cmd->skip_update) + if (!should_process_cmd(cmd)) continue; cmd->error_string = update(cmd, si); - if (shallow_update && !cmd->error_string && - si->shallow_ref[cmd->index]) { - error("BUG: connectivity check has not been run on ref %s", - cmd->ref_name); - checked_connectivity = 0; - } } - if (shallow_update && !checked_connectivity) - error("BUG: run 'git fsck' for safety.\n" - "If there are errors, try to remove " - "the reported refs above"); + if (shallow_update) + warn_if_skipped_connectivity_check(commands, si); } static struct command **queue_command(struct command **tail, -- cgit v1.2.1 From b6a4788586d8d1e88eee298c4a9a571416e50e3a Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Wed, 7 Jan 2015 19:23:16 -0800 Subject: receive-pack.c: die instead of error in case of possible future bug Discussion on the previous patch revealed we rather want to err on the safe side. To do so we need to stop receive-pack in case of the possible future bug when connectivity is not checked on a shallow push. Also while touching that code we considered that removing the reported refs may be harmful in some situations. Sound the message more like a "This Cannot Happen, Please Investigate!" instead of giving advice to remove refs. Suggested-by: Jonathan Nieder Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'builtin/receive-pack.c') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 2ebaf6695f..3bdb1586d6 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1061,9 +1061,7 @@ static void warn_if_skipped_connectivity_check(struct command *commands, } } if (!checked_connectivity) - error("BUG: run 'git fsck' for safety.\n" - "If there are errors, try to remove " - "the reported refs above"); + die("BUG: connectivity check skipped???"); } static void execute_commands(struct command *commands, -- cgit v1.2.1 From a1a261457c0577f5e0620fcc2b803999a6d5b8cf Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Wed, 7 Jan 2015 19:23:17 -0800 Subject: receive-pack.c: move iterating over all commands outside execute_commands This commit allows us in a later patch to easily distinguish between the non atomic way to update the received refs and the atomic way which is introduced in a later patch. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) (limited to 'builtin/receive-pack.c') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 3bdb1586d6..0ccfb3d689 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1064,6 +1064,18 @@ static void warn_if_skipped_connectivity_check(struct command *commands, die("BUG: connectivity check skipped???"); } +static void execute_commands_non_atomic(struct command *commands, + struct shallow_info *si) +{ + struct command *cmd; + for (cmd = commands; cmd; cmd = cmd->next) { + if (!should_process_cmd(cmd)) + continue; + + cmd->error_string = update(cmd, si); + } +} + static void execute_commands(struct command *commands, const char *unpacker_error, struct shallow_info *si) @@ -1098,12 +1110,7 @@ static void execute_commands(struct command *commands, free(head_name_to_free); head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL); - for (cmd = commands; cmd; cmd = cmd->next) { - if (!should_process_cmd(cmd)) - continue; - - cmd->error_string = update(cmd, si); - } + execute_commands_non_atomic(commands, si); if (shallow_update) warn_if_skipped_connectivity_check(commands, si); -- cgit v1.2.1 From 222368c6456211a3b2054ce4651cb58703886965 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Wed, 7 Jan 2015 19:23:18 -0800 Subject: receive-pack.c: move transaction handling in a central place This moves all code related to transactions into the execute_commands_non_atomic function. This includes beginning and committing the transaction as well as dealing with the errors which may occur during the begin and commit phase of a transaction. No functional changes intended. Helped-by: Eric Sunshine Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 51 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 14 deletions(-) (limited to 'builtin/receive-pack.c') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 0ccfb3d689..96e56a704d 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -66,6 +66,7 @@ static const char *NONCE_SLOP = "SLOP"; static const char *nonce_status; static long nonce_stamp_slop; static unsigned long nonce_stamp_slop_limit; +static struct ref_transaction *transaction; static enum deny_action parse_deny_action(const char *var, const char *value) { @@ -821,6 +822,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) } if (is_null_sha1(new_sha1)) { + struct strbuf err = STRBUF_INIT; if (!parse_object(old_sha1)) { old_sha1 = NULL; if (ref_exists(name)) { @@ -830,35 +832,36 @@ static const char *update(struct command *cmd, struct shallow_info *si) cmd->did_not_exist = 1; } } - if (delete_ref(namespaced_name, old_sha1, 0)) { - rp_error("failed to delete %s", name); + if (ref_transaction_delete(transaction, + namespaced_name, + old_sha1, + 0, old_sha1 != NULL, + "push", &err)) { + rp_error("%s", err.buf); + strbuf_release(&err); return "failed to delete"; } + strbuf_release(&err); return NULL; /* good */ } else { struct strbuf err = STRBUF_INIT; - struct ref_transaction *transaction; - if (shallow_update && si->shallow_ref[cmd->index] && update_shallow_ref(cmd, si)) return "shallow error"; - transaction = ref_transaction_begin(&err); - if (!transaction || - ref_transaction_update(transaction, namespaced_name, - new_sha1, old_sha1, 0, 1, "push", - &err) || - ref_transaction_commit(transaction, &err)) { - ref_transaction_free(transaction); - + if (ref_transaction_update(transaction, + namespaced_name, + new_sha1, old_sha1, + 0, 1, "push", + &err)) { rp_error("%s", err.buf); strbuf_release(&err); + return "failed to update ref"; } - - ref_transaction_free(transaction); strbuf_release(&err); + return NULL; /* good */ } } @@ -1068,12 +1071,32 @@ static void execute_commands_non_atomic(struct command *commands, struct shallow_info *si) { struct command *cmd; + struct strbuf err = STRBUF_INIT; + for (cmd = commands; cmd; cmd = cmd->next) { if (!should_process_cmd(cmd)) continue; + transaction = ref_transaction_begin(&err); + if (!transaction) { + rp_error("%s", err.buf); + strbuf_reset(&err); + cmd->error_string = "transaction failed to start"; + continue; + } + cmd->error_string = update(cmd, si); + + if (!cmd->error_string + && ref_transaction_commit(transaction, &err)) { + rp_error("%s", err.buf); + strbuf_reset(&err); + cmd->error_string = "failed to update ref"; + } + ref_transaction_free(transaction); } + + strbuf_release(&err); } static void execute_commands(struct command *commands, -- cgit v1.2.1 From 68deed298ac4c257cd72d6bee543f651cb10d669 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Wed, 7 Jan 2015 19:23:19 -0800 Subject: receive-pack.c: add execute_commands_atomic function This introduces the new function execute_commands_atomic which will use one atomic transaction for all updates. The default behavior is still the old non atomic way, one ref at a time. This is to cause as little disruption as possible to existing clients. It is unknown if there are client scripts that depend on the old non-atomic behavior so we make it opt-in for now. A later patch will add the possibility to actually use the functionality added by this patch. For now use_atomic is always 0. Inspired-by: Ronnie Sahlberg Helped-by: Eric Sunshine Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) (limited to 'builtin/receive-pack.c') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 96e56a704d..362d33f8e3 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1; static int unpack_limit = 100; static int report_status; static int use_sideband; +static int use_atomic; static int quiet; static int prefer_ofs_delta = 1; static int auto_update_server_info; @@ -1095,7 +1096,48 @@ static void execute_commands_non_atomic(struct command *commands, } ref_transaction_free(transaction); } + strbuf_release(&err); +} + +static void execute_commands_atomic(struct command *commands, + struct shallow_info *si) +{ + struct command *cmd; + struct strbuf err = STRBUF_INIT; + const char *reported_error = "atomic push failure"; + + transaction = ref_transaction_begin(&err); + if (!transaction) { + rp_error("%s", err.buf); + strbuf_reset(&err); + reported_error = "transaction failed to start"; + goto failure; + } + + for (cmd = commands; cmd; cmd = cmd->next) { + if (!should_process_cmd(cmd)) + continue; + + cmd->error_string = update(cmd, si); + + if (cmd->error_string) + goto failure; + } + if (ref_transaction_commit(transaction, &err)) { + rp_error("%s", err.buf); + reported_error = "atomic transaction failed"; + goto failure; + } + goto cleanup; + +failure: + for (cmd = commands; cmd; cmd = cmd->next) + if (!cmd->error_string) + cmd->error_string = reported_error; + +cleanup: + ref_transaction_free(transaction); strbuf_release(&err); } @@ -1133,7 +1175,10 @@ static void execute_commands(struct command *commands, free(head_name_to_free); head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL); - execute_commands_non_atomic(commands, si); + if (use_atomic) + execute_commands_atomic(commands, si); + else + execute_commands_non_atomic(commands, si); if (shallow_update) warn_if_skipped_connectivity_check(commands, si); -- cgit v1.2.1 From 1b70fe5d305462f1dd4b9d6233a2f4cb98e3a581 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Wed, 7 Jan 2015 19:23:20 -0800 Subject: receive-pack.c: negotiate atomic push support This adds the atomic protocol option to allow receive-pack to inform the client that it has atomic push capability. This commit makes the functionality introduced in the previous commits go live for the serving side. The changes in documentation reflect the protocol capabilities of the server. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'builtin/receive-pack.c') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 362d33f8e3..4c069c53e9 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -37,6 +37,7 @@ static int receive_fsck_objects = -1; static int transfer_fsck_objects = -1; static int receive_unpack_limit = -1; static int transfer_unpack_limit = -1; +static int advertise_atomic_push = 1; static int unpack_limit = 100; static int report_status; static int use_sideband; @@ -159,6 +160,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb) return 0; } + if (strcmp(var, "receive.advertiseatomic") == 0) { + advertise_atomic_push = git_config_bool(var, value); + return 0; + } + return git_default_config(var, value, cb); } @@ -174,6 +180,8 @@ static void show_ref(const char *path, const unsigned char *sha1) strbuf_addstr(&cap, "report-status delete-refs side-band-64k quiet"); + if (advertise_atomic_push) + strbuf_addstr(&cap, " atomic"); if (prefer_ofs_delta) strbuf_addstr(&cap, " ofs-delta"); if (push_cert_nonce) @@ -1263,6 +1271,9 @@ static struct command *read_head_info(struct sha1_array *shallow) use_sideband = LARGE_PACKET_MAX; if (parse_feature_request(feature_list, "quiet")) quiet = 1; + if (advertise_atomic_push + && parse_feature_request(feature_list, "atomic")) + use_atomic = 1; } if (!strcmp(line, "push-cert")) { -- cgit v1.2.1