Re: [PATCH 02/26] transport-helper.c: refactor set_helper_option()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 25, 2016, 06:47 UTC
Message-ID: <CAPig+cR--ZK1eGfOKfE-PhKha80EqwVi-AH90GxOQt4vka3=tg@mail.gmail.com>
In-Reply-To: <1460552110-5554-3-git-send-email-pclouds@gmail.com>

On Wed, Apr 13, 2016 at 8:54 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 23 quoted lines

> For now we can handle two types, string and boolean, in
> set_helper_option(). Later on we'll add string_list support, which does
> not fit well. The new function strbuf_set_helper_option() can be reused
> for a separate function that handles string-list.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/transport-helper.c b/transport-helper.c
> @@ -260,6 +260,28 @@ static const char *boolean_options[] = {
> +static int strbuf_set_helper_option(struct helper_data *data,
> +                                   struct strbuf *buf)
> +{
> +       int ret;
> +
> +       sendline(data, buf);
> +       if (recvline(data, buf))
> +               exit(128);
> +
> +       if (!strcmp(buf->buf, "ok"))
> +               ret = 0;
> +       else if (starts_with(buf->buf, "error")) {
> +               ret = -1;
> +       } else if (!strcmp(buf->buf, "unsupported"))

You could use this opportunity to drop the unnecessary braces. (True, doing so makes it something other than pure code movement, but it's minor and probably a worthy cleanup.)

Show 33 quoted lines

> +               ret = 1;
> +       else {
> +               warning("%s unexpectedly said: '%s'", data->name, buf->buf);
> +               ret = 1;
> +       }
> +       return ret;
> +}
> +
>  static int set_helper_option(struct transport *transport,
>                           const char *name, const char *value)
>  {
> @@ -291,20 +313,7 @@ static int set_helper_option(struct transport *transport,
>                 quote_c_style(value, &buf, NULL, 0);
>         strbuf_addch(&buf, '\n');
>
> -       sendline(data, &buf);
> -       if (recvline(data, &buf))
> -               exit(128);
> -
> -       if (!strcmp(buf.buf, "ok"))
> -               ret = 0;
> -       else if (starts_with(buf.buf, "error")) {
> -               ret = -1;
> -       } else if (!strcmp(buf.buf, "unsupported"))
> -               ret = 1;
> -       else {
> -               warning("%s unexpectedly said: '%s'", data->name, buf.buf);
> -               ret = 1;
> -       }
> +       ret = strbuf_set_helper_option(data, &buf);
>         strbuf_release(&buf);
>         return ret;
>  }