On Tue, Dec 16, 2014 at 1:49 PM, Stefan Beller <sbel...@google.com> wrote:
> From: Ronnie Sahlberg <sahlb...@google.com>
>
> Update receive-pack to use an atomic transaction iff the client negotiated
> that it wanted atomic-push. This leaves the default behavior to be the old
> non-atomic one ref at a time update. 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.
>
> If it turns out over time that there are no client scripts that depend on the
> old behavior we can change git to default to use atomic pushes and instead
> offer an opt-out argument for people that do not want atomic pushes.
>
> Signed-off-by: Ronnie Sahlberg <sahlb...@google.com>
> Signed-off-by: Stefan Beller <sbel...@google.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index e76e5d5..0803fd2 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -67,6 +67,8 @@ static const char *NONCE_SLOP = "SLOP";
> static const char *nonce_status;
> static long nonce_stamp_slop;
> static unsigned long nonce_stamp_slop_limit;
> +struct strbuf err = STRBUF_INIT;
> +struct ref_transaction *transaction;
Should these be static?
> static enum deny_action parse_deny_action(const char *var, const char *value)
> {
> @@ -1059,6 +1059,16 @@ static void execute_commands(struct command *commands,
> return;
> }
>
> + if (use_atomic) {
> + transaction = ref_transaction_begin(&err);
> + if (!transaction) {
> + error("%s", err.buf);
> + strbuf_release(&err);
> + for (cmd = commands; cmd; cmd = cmd->next)
> + cmd->error_string = "transaction error";
> + return;
> + }
> + }
Not seen in this diff, but just below this point, the pre-receive hook
is invoked. If it "declines", then execute_commands() returns without
releasing the transaction which was begun here. Is that correct
behavior?
For robustness, it might also be sane to release the 'err' strbuf at
this early return (though the current code does not strictly leak it).
> data.cmds = commands;
> data.si = si;
> if (check_everything_connected(iterate_receive_command_list, 0,
> &data))
> @@ -1086,8 +1096,23 @@ static void execute_commands(struct command *commands,
>
> if (cmd->skip_update)
> continue;
> -
> + if (!use_atomic) {
> + transaction = ref_transaction_begin(&err);
> + if (!transaction) {
> + rp_error("%s", err.buf);
> + strbuf_release(&err);
> + cmd->error_string = "failed to start
> transaction";
Here, you assign cmd->error_string...
> + }
> + }
> cmd->error_string = update(cmd, si);
and then immediately overwrite it here. Is it correct to invoke
update() when ref_transaction_begin() has failed? Seems fishy.
> + if (!use_atomic)
> + if (ref_transaction_commit(transaction, &err)) {
> + ref_transaction_free(transaction);
Shouldn't you be freeing the transaction outside of this conditional
rather than only in case of failure?
> + rp_error("%s", err.buf);
> + strbuf_release(&err);
> + cmd->error_string = "failed to update ref";
> + }
> +
> if (shallow_update && !cmd->error_string &&
> si->shallow_ref[cmd->index]) {
> error("BUG: connectivity check has not been run on
> ref %s",
> @@ -1096,6 +1121,14 @@ static void execute_commands(struct command *commands,
> }
> }
>
> + if (use_atomic) {
> + if (ref_transaction_commit(transaction, &err)) {
> + rp_error("%s", err.buf);
> + for (cmd = commands; cmd; cmd = cmd->next)
> + cmd->error_string = err.buf;
> + }
> + ref_transaction_free(transaction);
> + }
> if (shallow_update && !checked_connectivity)
> error("BUG: run 'git fsck' for safety.\n"
> "If there are errors, try to remove "
> @@ -1543,5 +1576,6 @@ int cmd_receive_pack(int argc, const char **argv, const
> char *prefix)
> sha1_array_clear(&shallow);
> sha1_array_clear(&ref);
> free((void *)push_cert_nonce);
> + strbuf_release(&err);
> return 0;
> }
> --
> 2.2.0.31.gad78000.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
