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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Dec 18, 2014 at 12:45 PM, Stefan Beller <sbeller@google.com> wrote:
> From: Ronnie Sahlberg <sahlberg@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 <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index e76e5d5..4952d91 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1059,6 +1063,16 @@ static void execute_commands(struct command *commands,
>                 return;
>         }
>
> +       if (use_atomic) {
> +               transaction = ref_transaction_begin(&err);

Repeating from my earlier review[1]: If the 'pre-receive' hook
"declines", then this transaction is left dangling (and its resources
leaked).

> +               if (!transaction) {
> +                       error("%s", err.buf);
> +                       strbuf_release(&err);
> +                       for (cmd = commands; cmd; cmd = cmd->next)
> +                               cmd->error_string = "transaction error";

The !use_atomic case (below), calls this error "failed to start
transaction", not merely "transaction error".

Furthermore, in the use_atomic case (also below), when a commit fails,
you assign err.buf to cmd->error_string rather than a generic
"transaction error" message. What differs between these cases which
makes the generic message preferable here over the more specific
err.buf message?

> +                       return;
> +               }
> +       }
>         data.cmds = commands;
>         data.si = si;
>         if (check_everything_connected(iterate_receive_command_list, 0, &data))
> @@ -1087,7 +1101,30 @@ 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";
> +                               continue;
> +                       }
> +               }
>                 cmd->error_string = update(cmd, si);
> +               if (!cmd->error_string) {
> +                       if (!use_atomic
> +                           && ref_transaction_commit(transaction, &err)) {
> +                               ref_transaction_free(transaction);

Repeating from my earlier review[1]: This is leaking 'transaction' for
each successful commit (and only freeing it upon commit error).

> +                               rp_error("%s", err.buf);
> +                               strbuf_release(&err);
> +                               cmd->error_string = "failed to update ref";
> +                       }
> +               } else if (use_atomic) {
> +                       goto atomic_failure;

See commentary below.

> +               } else {
> +                       ref_transaction_free(transaction);
> +               }
> +
>                 if (shallow_update && !cmd->error_string &&
>                     si->shallow_ref[cmd->index]) {
>                         error("BUG: connectivity check has not been run on ref %s",
> @@ -1096,10 +1133,28 @@ 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;

At the end of this function, strbuf_release(&err) is invoked, which
leaves all these cmd->error_strings dangling.

> +               }
> +               ref_transaction_free(transaction);
> +       }
>         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);
> +
> +       return;
> +
> +atomic_failure:
> +       for (cmd = commands; cmd; cmd = cmd->next)
> +               if (!cmd->error_string)
> +                       cmd->error_string = "atomic push failure";
> +
> +       ref_transaction_free(transaction);

goto's can help simplify error-handling when multiple conditional
branches need to perform common cleanup, however, this label
corresponds to only a single goto statement. Placing the cleanup code
for that one case so far away from the point where the condition is
detected actually obfuscates the code slightly rather than clarifying
it. It would be a bit easier to understand the logic if this cleanup
(plus a 'return') was inlined directly at the location of the 'goto'.

By the way, you employ the idiom of iterating over 'commands' and
assigning error_string often enough, that it might be worthwhile to
wrap it in a function. In that case, inlining the above cleanup code
in place of the 'goto' would make it even easier to read since it
would be a mere two-liner.

>  }

[1]: http://article.gmane.org/gmane.comp.version-control.git/261455