Re: Making tab-complete.c easier to maintain

On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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