Re: [PATCH v4 2/5] am: improve author-script error reporting
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 5, 2018, 01:53 UTC
- Message-ID
- <xmqqy3a8jnmw.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <CAPig+cRBwkKGtzY6CwZiKb1mef0a-9F+O24d0R8ZfsRb5ssUbA@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 5 quoted lines
> 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.
Show 39 quoted lines
> 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);
> ...