Re: [PATCHv3 3/6] send-pack.c: add --atomic command line argument

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 19, 2014, 01:22 UTC
Message-ID: <CAPig+cSHp+4Q3yM8EeG-b3VqofuuhAjaKRMrjJ=5jGutY2eKhw@mail.gmail.com>
In-Reply-To: <1418841177-12152-4-git-send-email-sbeller@google.com>

On Wed, Dec 17, 2014 at 1:32 PM, Stefan Beller <sbeller@google.com> wrote:

Show 39 quoted lines

> From: Ronnie Sahlberg <sahlberg@google.com>
>
> This adds support to send-pack to negotiate and use atomic pushes
> iff the server supports it. Atomic pushes are activated by a new command
> line flag --atomic.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/send-pack.c b/send-pack.c
> index 77e4201..a696d5c 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -282,6 +282,30 @@ free_return:
>         return update_seen;
>  }
>
> +
> +static int atomic_push_failure(struct send_pack_args *args,
> +                              struct ref *remote_refs,
> +                              struct ref *failing_ref)
> +{
> +       struct ref *ref;
> +       /* Mark other refs as failed */
> +       for (ref = remote_refs; ref; ref = ref->next) {
> +               if (!ref->peer_ref && !args->send_mirror)
> +                       continue;
> +
> +               switch (ref->status) {
> +               case REF_STATUS_EXPECTING_REPORT:
> +                       ref->status = REF_STATUS_ATOMIC_PUSH_FAILED;
> +                       continue;
> +               default:
> +                       ; /* do nothing */
> +               }
> +       }
> +       error("atomic push failed for ref %s. "
> +             "status: %d\n", failing_ref->name, failing_ref->status);
> +       return -1;

error() returns -1, so:

    return error(...);

Also, why split the string literal like that? It's not warranted by the indentation level. Instead:

    return error("atomic push failed for ref %s. status: %d\n",
        failing_ref->name, failing_ref->status);

Show 14 quoted lines

> +}
> +
>  int send_pack(struct send_pack_args *args,
>               int fd[], struct child_process *conn,
>               struct ref *remote_refs,
> @@ -373,9 +397,21 @@ int send_pack(struct send_pack_args *args,
>          * the pack data.
>          */
>         for (ref = remote_refs; ref; ref = ref->next) {
> -               if (check_to_send_update(ref, args) < 0)
> +               switch (check_to_send_update(ref, args)) {
> +               case 0: /* no error */
> +                       break;
> +               case -CHECK_REF_STATUS_REJECTED:

I realize that Junio said that this could wait for cleanup patch by someone later, but I'd argue that defining these constants now with their proper negative values would be beneficial:

First, it is a potential maintenance burden that each person working on the code later must remember to negate the constants each time they are used. Forgetting to do so will lead to incorrect behavior.

Second, negating these constants at point-of-use implies incorrectly that there is some valid use-case for the non-negated forms. It's misleading and confusing.

Show 14 quoted lines

> +                       /*
> +                        * When we know the server would reject a ref update if
> +                        * we were to send it and we're trying to send the refs
> +                        * atomically, abort the whole operation.
> +                        */
> +                       if (use_atomic)
> +                               return atomic_push_failure(args, remote_refs, ref);
> +                       /* Fallthrough for non atomic case. */
> +               default:
>                         continue;
> -
> +               }
>                 if (!ref->deletion)
>                         need_pack_data = 1;