Re: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes

Stefan Beller [off-list ref] writes:

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 ...

... 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,

On Wed, Dec 17, 2014 at 3:26 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

@@ -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?

This would change the current behavior. In the case of !atomic we want
to consider all commands and not stop early.

So maybe more
if (!cmd->error_string) {
        if (!use_atomic
            && ref_transaction_commit(...)) {
            ...
        }
} else {
        if (use_atomic)
             goto check_atomic_commit;
}

and the  check_atomic_commit label is replacing the loop to check:
-        for (cmd = commands; cmd; cmd = cmd->next)
-                if (cmd->error_string)
-                        break;
+ check_atomic_commit: