diff mbox series

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

Message ID 20181031101556.27169-3-phillip.wood@talktalk.net (mailing list archive)
State New, archived
Headers show
Series [v4,1/5] am: don't die in read_author_script() | expand

Commit Message

Phillip Wood Oct. 31, 2018, 10:15 a.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are errors in a user edited author-script there was no
indication of what was wrong. This commit adds some specific error messages
depending on the problem. It also relaxes the requirement that the
variables appear in a specific order in the file to match the behavior
of 'rebase --interactive'.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/am.c | 49 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 10 deletions(-)

Comments

Eric Sunshine Nov. 4, 2018, 4:12 a.m. UTC | #1 
On Wed, Oct 31, 2018 at 6:16 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> 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);
    ...
Junio C Hamano Nov. 5, 2018, 1:53 a.m. UTC | #2 
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);
>     ...
diff mbox series

Patch

diff --git a/builtin/am.c b/builtin/am.c
index b68578bc3f..d42b725273 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -270,8 +270,11 @@  static int parse_key_value_squoted(char *buf, struct string_list *list)
 		struct string_list_item *item;
 		char *np;
 		char *cp = strchr(buf, '=');
-		if (!cp)
-			return -1;
+		if (!cp) {
+			np = strchrnul(buf, '\n');
+			return error(_("unable to parse '%.*s'"),
+				     (int) (np - buf), buf);
+		}
 		np = strchrnul(cp, '\n');
 		*cp++ = '\0';
 		item = string_list_append(list, buf);
@@ -280,7 +283,8 @@  static int parse_key_value_squoted(char *buf, struct string_list *list)
 		*np = '\0';
 		cp = sq_dequote(cp);
 		if (!cp)
-			return -1;
+			return error(_("unable to dequote value of '%s'"),
+				     item->string);
 		item->util = xstrdup(cp);
 	}
 	return 0;
@@ -308,6 +312,7 @@  static int read_author_script(struct am_state *state)
 	struct strbuf buf = STRBUF_INIT;
 	struct string_list kv = STRING_LIST_INIT_DUP;
 	int retval = -1; /* assume failure */
+	int i, name_i = -2, email_i = -2, date_i = -2, err = 0;
 	int fd;
 
 	assert(!state->author_name);
@@ -326,14 +331,38 @@  static int read_author_script(struct am_state *state)
 	if (parse_key_value_squoted(buf.buf, &kv))
 		goto finish;
 
-	if (kv.nr != 3 ||
-	    strcmp(kv.items[0].string, "GIT_AUTHOR_NAME") ||
-	    strcmp(kv.items[1].string, "GIT_AUTHOR_EMAIL") ||
-	    strcmp(kv.items[2].string, "GIT_AUTHOR_DATE"))
+	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;
+		} else if (!strcmp(kv.items[i].string, "GIT_AUTHOR_EMAIL")) {
+			if (email_i >= 0)
+				email_i = error(_("'GIT_AUTHOR_EMAIL' already given"));
+			else
+				email_i = i;
+		} else if (!strcmp(kv.items[i].string, "GIT_AUTHOR_DATE")) {
+			if (date_i >= 0)
+				date_i = error(_("'GIT_AUTHOR_DATE' already given"));
+			else
+				date_i = i;
+		} else {
+			err = error(_("unknown variable '%s'"),
+				    kv.items[i].string);
+		}
+	}
+	if (name_i == -2)
+		error(_("missing 'GIT_AUTHOR_NAME'"));
+	if (email_i == -2)
+		error(_("missing 'GIT_AUTHOR_EMAIL'"));
+	if (date_i == -2)
+		error(_("missing 'GIT_AUTHOR_DATE'"));
+	if (date_i < 0 || email_i < 0 || date_i < 0 || err)
 		goto finish;
-	state->author_name = kv.items[0].util;
-	state->author_email = kv.items[1].util;
-	state->author_date = kv.items[2].util;
+	state->author_name = kv.items[name_i].util;
+	state->author_email = kv.items[email_i].util;
+	state->author_date = kv.items[date_i].util;
 	retval = 0;
 finish:
 	string_list_clear(&kv, !!retval);