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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 19, 2014, 10:14 UTC
Message-ID: <CAPig+cSqHW9MGsPW9DCFZ8_Xz+WUc1TBpih2xvPq1NSkJQMM5Q@mail.gmail.com>
In-Reply-To: <1418948521-30787-1-git-send-email-sbeller@google.com>

On Thu, Dec 18, 2014 at 7:22 PM, Stefan Beller <sbeller@google.com> wrote:

Show 30 quoted lines

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

Show 9 quoted lines

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

Show 15 quoted lines

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

Show 24 quoted lines

>                     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