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

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

... and update three copies of very similar looking code that have
to stay similar.  If this were parsing unbounded and customizable
set of variables, I think the approach you suggest to use a helper
that iterates over the whole array for each key makes sense, but for
now I think what was queued would be OK.

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