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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Dec 18, 2014 at 7:22 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..ebce2fa 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1087,7 +1102,36 @@ 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 = "transaction failed to start";
> +                               continue;

For this failure, you 'continue' the loop.

> +                       }
> +               }
>                 cmd->error_string = update(cmd, si);
> +               if (!cmd->error_string) {
> +                       if (!use_atomic) {
> +                               if (ref_transaction_commit(transaction, &err)) {
> +                                       rp_error("%s", err.buf);
> +                                       strbuf_release(&err);
> +                                       cmd->error_string = "failed to update ref";

However, for this failure, you fall through...

> +                               }
> +                               ref_transaction_free(transaction);
> +                       }
> +               } else {
> +                       ref_transaction_free(transaction);
> +                       if (use_atomic) {
> +                               for (cmd = commands; cmd; cmd = cmd->next)
> +                                       if (!cmd->error_string)
> +                                               cmd->error_string = "atomic push failure";
> +                               strbuf_release(&err);
> +                               return;
> +                       }
> +               }
> +
>                 if (shallow_update && !cmd->error_string &&

And here must check if an error occurred in some code above.

This is ugly and inconsistent, and feels as if the new code was
plopped into the middle of this function without concern for overall
flow, thus negatively impacting maintainability and readability. It
could be made a bit cleaner by either (1) consistently using
'continue' for the non-atomic error cases, or (2) moving the "shallow"
handling up into the conditional where you _know_ that no error has
occurred (error_string is NULL).

However, this issue is symptomatic of a larger problem. Prior to this
patch, execute_commands() was relatively straight-forward and easy to
read and understand. With the patch, and its interleaved atomic and
non-atomic cases, the logic flow is a mess: it places a high cognitive
load on the reader and is difficult to maintain and to do correctly in
the first place (as already evidenced).

Have you considered refactoring the code to implement the atomic and
non-atomic cases as distinct single-purpose helper functions of
execute_commands()? It should be possible to do so with very little
duplicated code between the two functions, and the result would likely
be much more readable and maintainable.

>                     si->shallow_ref[cmd->index]) {
>                         error("BUG: connectivity check has not been run on ref %s",
> @@ -1096,10 +1140,19 @@ 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 = "atomic transaction failed";
> +               }
> +               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);
>  }
>
>  static struct command **queue_command(struct command **tail,
> --
> 2.2.1.62.g3f15098