Re: Making tab-complete.c easier to maintain

On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
>> Thomas Munro wrote:
>>> New version attached, merging recent changes.
>>
>> I wonder about the TailMatches and Matches macros --- wouldn't it be
>> better to have a single one, renaming TailMatches to Matches and
>> replacing the current Matches() with an initial token that corresponds
>> to anchoring to start of command?  Just wondering, not terribly attached
>> to the idea.
>
> +       /* TODO:TM -- begin temporary, not part of the patch! */
> +       Assert(!word_matches(NULL, ""));
> + [...]
> +       Assert(!word_matches("foo", ""));
> +       /* TODO:TM -- end temporary */
>
> Be sure to not forget to remove that later.

Thanks for looking at this Michael.  It's probably not much fun to
review!  Here is a new version with that bit removed.  More responses
inline below.

> -       else if (pg_strcasecmp(prev5_wd, "DEFAULT") == 0 &&
> -                        pg_strcasecmp(prev4_wd, "PRIVILEGES") == 0 &&
> -                        (pg_strcasecmp(prev3_wd, "FOR") == 0 ||
> -                         pg_strcasecmp(prev3_wd, "IN") == 0))
> -       {
> -               static const char *const
> list_ALTER_DEFAULT_PRIVILEGES_REST[] =
> -               {"GRANT", "REVOKE", NULL};
> -
> -               COMPLETE_WITH_LIST(list_ALTER_DEFAULT_PRIVILEGES_REST);
> -       }
> +       else if (TailMatches5("DEFAULT", "PRIVILEGES", "FOR", "ROLE",
> MatchAny) ||
> +                        TailMatches5("DEFAULT", "PRIVILEGES", "IN",
> "SCHEMA", MatchAny))
> +               CompleteWithList2("GRANT", "REVOKE");
> For this chunk I think that you need to check for ROLE|USER and not only
> ROLE.

Right, done.

> +       else if (TailMatches4("ALTER", "DOMAIN", MatchAny, "RENAME"))
>         {
>                 static const char *const list_ALTERDOMAIN[] =
>                 {"CONSTRAINT", "TO", NULL};
> I think you should remove COMPLETE_WITH_LIST here for consistency with the
> rest.

Right, done.

> -       else if (pg_strcasecmp(prev5_wd, "DOMAIN") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "RENAME") == 0 &&
> -                        pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0)
> +       else if (TailMatches3("RENAME", "CONSTRAINT", MatchAny))
>                 COMPLETE_WITH_CONST("TO");
> Perhaps you are missing DOMAIN here?

Right, done.

> -       else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "SEQUENCE") == 0 &&
> -                        pg_strcasecmp(prev_wd, "NO") == 0)
> -       {
> -               static const char *const list_ALTERSEQUENCE2[] =
> -               {"MINVALUE", "MAXVALUE", "CYCLE", NULL};
> -
> -               COMPLETE_WITH_LIST(list_ALTERSEQUENCE2);
> -       }
> +       else if (TailMatches4("ALTER", "SEQUEMCE", MatchAny, "NO"))
> +               CompleteWithList3("MINVALUE", "MAXVALUE", "CYCLE");
> Typo here: s/SEQUEMCE/SEQUENCE.

Oops, fixed.

> -       else if (pg_strcasecmp(prev5_wd, "TABLE") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "RENAME") == 0 &&
> -                        (pg_strcasecmp(prev2_wd, "COLUMN") == 0 ||
> -                         pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0) &&
> -                        pg_strcasecmp(prev_wd, "TO") != 0)
> +       else if (TailMatches6("ALTER", "TABLE", MatchAny, "RENAME",
> "COLUMN|CONSTRAINT", MatchAny) &&
> +                        !TailMatches1("TO"))
> This should use TailMatches5 without ALTER for consistency with the existing
> code?

Ok, done.

> -       else if (pg_strcasecmp(prev_wd, "CLUSTER") == 0 &&
> -                        pg_strcasecmp(prev2_wd, "WITHOUT") != 0)
> +       else if (TailMatches1("CLUSTER") && !TailMatches2("WITHOUT",
> "CLUSTER"))
> Here removing CLUSTER should be fine.

Ok.

> -       else if (pg_strcasecmp(prev2_wd, "CLUSTER") == 0 &&
> -                        pg_strcasecmp(prev_wd, "ON") != 0 &&
> -                        pg_strcasecmp(prev_wd, "VERBOSE") != 0)
> -       {
> +       else if (TailMatches2("CLUSTER", MatchAny) &&
> !TailMatches1("VERBOSE"))
> Handling of ON has been forgotten.

Right, fixed.

> -       else if (pg_strcasecmp(prev3_wd, "CREATE") == 0 &&
> -                        !(pg_strcasecmp(prev2_wd, "USER") == 0 &&
> pg_strcasecmp(prev_wd, "MAPPING") == 0) &&
> -                        (pg_strcasecmp(prev2_wd, "ROLE") == 0 ||
> -                         pg_strcasecmp(prev2_wd, "GROUP") == 0 ||
> pg_strcasecmp(prev2_wd, "USER") == 0))
> +       else if (TailMatches3("CREATE", "ROLE|GROUP|USER", MatchAny) &&
> +                        !TailMatches3("CREATE", "USER", "MAPPING"))
> !TailMatches2("USER", "MAPPING")?

Ok.

> -       else if (pg_strcasecmp(prev4_wd, "CREATE") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "MATERIALIZED") == 0 &&
> -                        pg_strcasecmp(prev2_wd, "VIEW") == 0)
> +       else if (TailMatches3("CREATE", "MATERIALIZED", "VIEW"))
> Forgot a MatchAny here?

Right, fixed.

> -       else if (pg_strcasecmp(prev_wd, "DELETE") == 0 &&
> -                        !(pg_strcasecmp(prev2_wd, "ON") == 0 ||
> -                          pg_strcasecmp(prev2_wd, "GRANT") == 0 ||
> -                          pg_strcasecmp(prev2_wd, "BEFORE") == 0 ||
> -                          pg_strcasecmp(prev2_wd, "AFTER") == 0))
> +       else if (TailMatches1("DELETE") &&
> !TailMatches2("ON|GRANT|BEFORE|AFTER", "DELETE"))
>                 COMPLETE_WITH_CONST("FROM");
> In the second clause checking for DELETE is not necessary.

Ok.

> -       else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
> -                        prev2_wd[0] == '\0')
> +       else if (Matches1("EXECUTE"))
> This looks not complete.

I think it's correct.  The existing code has prev2_wd[0] == '\0' as a
way of checking that EXECUTE is the first word.  Matches1("EXECUTE")
does the same.

> +       else if (TailMatches1("EXPLAIN"))
> +               CompleteWithList7("SELECT", "INSERT", "DELETE", "UPDATE",
> "DECLARE",
> +                                                       "ANALYZE",
> "VERBOSE");
> +       else if (TailMatches2("EXPLAIN", "ANALYZE|ANALYSE"))
> ANALYSE should be removed, former code only checked for ANALYZE => I heard
> about the grammar issues here :)

Right.  Removed.

> -               else if (pg_strcasecmp(prev4_wd, "GRANT") == 0)
> +               else if (TailMatches4("GRANT", MatchAny, MatchAny,
> MatchAny))
> +                       COMPLETE_WITH_CONST("TO");
> +               else
> +                       COMPLETE_WITH_CONST("FROM");
> +       }
> +
> +       /* Complete "GRANT/REVOKE * ON * *" with TO/FROM */
> +       else if (TailMatches5("GRANT|REVOKE", MatchAny, "ON", MatchAny,
> MatchAny))
> +       {
> +               if (TailMatches5("GRANT", MatchAny, MatchAny, MatchAny,
> MatchAny))
> Isn't the first check with TailMatches4 enough here?

I don't think so: the first handles GRANT * ON * where the final word
isn't one of the known keywords that will be followed by an
appropriate list of objects (in other words when it was a table name).
The TailMatches5 case is for supplying TO or FROM after you write eg
GRANT * ON TABLE *.

> -               if (pg_strcasecmp(prev6_wd, "GRANT") == 0)
> +               if (TailMatches6("GRANT", MatchAny, MatchAny, MatchAny,
> MatchAny, MatchAny))
> HeadMatches1 perhaps?

I agree that would probably be better but Alvaro suggested following
the existing logic in the first pass, which was mostly based on tails,
and then considering simpler head-based patterns in a future pass.

Thanks!

--
Thomas Munro
http://www.enterprisedb.com