Re: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:03:18
Stefan Beller [off-list ref] writes:
quoted hunk
@@ -1086,8 +1100,25 @@ 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"; + return; + } + } cmd->error_string = update(cmd, si); + if (!use_atomic) + if (ref_transaction_commit(transaction, &err)) { + ref_transaction_free(transaction); + rp_error("%s", err.buf); + strbuf_release(&err); + cmd->error_string = "failed to update ref"; + return; + }
Hmm, should the code even attempt to commit if update() returned a
non NULL, signaling a failure?
Or would we want to do this instead?
if (cmd->error_string)
goto transaction_abort;
else if (!use_atomic) {
if (ref_transaction_commit(...)) {
...
cmd->error_string = "...";
return;
}
}
and then ...
quoted hunk
if (shallow_update && !cmd->error_string && si->shallow_ref[cmd->index]) { error("BUG: connectivity check has not been run on ref %s",@@ -1096,10 +1127,32 @@ static void execute_commands(struct command *commands, } } + if (use_atomic) { + /* + * update(...) may abort early (i.e. because the hook refused to + * update that ref) which then doesn't even record a transaction + * regarding that ref. Make sure all commands are without error + * and then commit atomically. + */ + for (cmd = commands; cmd; cmd = cmd->next) + if (cmd->error_string) + break; + if (cmd) { + for (cmd = commands; cmd; cmd = cmd->next) + if (!cmd->error_string) + cmd->error_string = "atomic push failure"; + } else if (ref_transaction_commit(transaction, &err)) { + rp_error("%s", err.buf); + for (cmd = commands; cmd; cmd = cmd->next) + cmd->error_string = err.buf; + }
... have the label to jump to here: transaction_abort:
+ ref_transaction_free(transaction);
I was confused by the fact that you did not have any call to transaction-abort, until I realized that there is no such API function and ref_transaction_free() serves that "don't commit, roll it back" purpose.
+ }
if (shallow_update && !checked_connectivity)
error("BUG: run 'git fsck' for safety.\n"
"If there are errors, try to remove "
"the reported refs above");
+ strbuf_release(&err);
}
static struct command **queue_command(struct command **tail,