Re: [PATCH v4 2/5] am: improve author-script error reporting

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 4, 2018, 04:12 UTC
Message-ID: <CAPig+cRBwkKGtzY6CwZiKb1mef0a-9F+O24d0R8ZfsRb5ssUbA@mail.gmail.com>
In-Reply-To: <20181031101556.27169-3-phillip.wood@talktalk.net>

On Wed, Oct 31, 2018 at 6:16 AM Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 23 quoted lines

> diff --git a/builtin/am.c b/builtin/am.c
> @@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)
> +       int i, name_i = -2, email_i = -2, date_i = -2, err = 0;
> @@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)
> +       for (i = 0; i < kv.nr; i++) {
> +               if (!strcmp(kv.items[i].string, "GIT_AUTHOR_NAME")) {
> +                       if (name_i >= 0)
> +                               name_i = error(_("'GIT_AUTHOR_NAME' already given"));
> +                       else
> +                               name_i = i;
> +               }
> +               ...
> +       }
> +       if (name_i == -2)
> +               error(_("missing 'GIT_AUTHOR_NAME'"));
> +       ...
> +       if (date_i < 0 || email_i < 0 || date_i < 0 || err)
>                 goto finish;
> +       state->author_name = kv.items[name_i].util;
> +       ...
>         retval = 0;
>  finish:
>         string_list_clear(&kv, !!retval);

Junio labeled the "-2" trick "cute", and while it is optimal in that it only traverses the key/value list once, it also increases cognitive load since the reader has to spend a good deal more brain cycles understanding what is going on than would be the case with simpler (and less noisily repetitive) code.

An alternative would be to make the code trivially easy to understand, though a bit more costly, but that expense should be negligible since this file should always be tiny, containing very few key/value pairs, and it's not timing critical code anyhow. For instance:

static char *find(struct string_list *kv, const char *key)
{
    const char *val = NULL;
    int i;
    for (i = 0; i < kv.nr; i++) {
        if (!strcmp(kv.items[i].string, key)) {
            if (val) {
                error(_("duplicate %s"), key);
                return NULL;
            }
            val = kv.items[i].util;
        }
    }
    if (!val)
        error(_("missing %s"), key);
    return val;
}

static int read_author_script(struct am_state *state)
{
    ...
    char *name, *email, *date;
    ...
    name = find(&kv, "GIT_AUTHOR_NAME");
    email = find(&kv, "GIT_AUTHOR_EMAIL");
    date = find(&kv, "GIT_AUTHOR_DATE");
    if (name && email && date) {
        state->author_name = name;
        state->author_email = email;
        state->author_date = date;
        retval = 0;
    }
    string_list_clear&kv, !!retval);
    ...