On Tue, 3 Jan 2023 at 12:30, vignesh C <vignes...@gmail.com> wrote:
>
> The patch does not apply on top of HEAD as in [1], please post a rebased
> patch:
>
This is because 0001 has been committed.
Re-uploading 0002, to keep the CF-bot happy.
Reviewing 0002...
I'm not entirely convinced that the PartialMatches() changes are
really necessary. As far as I can see USING followed by ON doesn't
appear anywhere else in the grammar, and the later completions
involving WHEN [NOT] MATCHED are definitely unique to MERGE.
Nonetheless, I guess it's not a bad thing to check that it really is a
MERGE. Also the new matching function might prove useful for other
cases.
Some more detailed code comments:
I find the name PartialMatches() a little off, since "partial" doesn't
really accurately describe what it's doing. HeadMatches() and
TailMatches() are also partial matches (matching the head and tail
parts). So perhaps MidMatches() would be a better name.
I also found the comment description of PartialMatchesImpl() misleading:
/*
* Implementation of PartialMatches and PartialMatchesCS macros: do parts of
* the words in previous_words match the variadic arguments?
*/
I think a more accurate description would be:
/*
* Implementation of MidMatches and MidMatchesCS macros: do any N consecutive
* words in previous_words match the variadic arguments?
*/
Similarly, instead of:
/* Match N words on the line partially, case-insensitively. */
how about:
/* Match N consecutive words anywhere on the line, case-insensitively. */
In PartialMatchesImpl()'s main loop:
if (previous_words_count - startpos < narg)
{
va_end(args);
return false;
}
couldn't that just be built into the loop's termination clause (i.e.,
loop while startpos <= previous_words_count - narg)?
For the first block of changes using the new function:
else if (PartialMatches("MERGE", "INTO", MatchAny, "USING") ||
PartialMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING") ||
PartialMatches("MERGE", "INTO", MatchAny, MatchAny, "USING"))
{
/* Complete MERGE INTO ... ON with target table attributes */
if (TailMatches("INTO", MatchAny, "USING", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev4_wd);
else if (TailMatches("INTO", MatchAny, "AS", MatchAny,
"USING", MatchAny, "AS", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev8_wd);
else if (TailMatches("INTO", MatchAny, MatchAny, "USING",
MatchAny, MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev6_wd);
wouldn't it be simpler to just include "MERGE" in the TailMatches()
arguments, and leave these 3 cases outside the new code block. I.e.:
/* Complete MERGE INTO ... ON with target table attributes */
else if (TailMatches("MERGE", "INTO", MatchAny, "USING", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev4_wd);
else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING", MatchAny, "AS", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev8_wd);
else if (TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING",
MatchAny, MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev6_wd);
Regards,
Dean
From 8c7e25e4a2d939a32751bd9a0d487c510ec66191 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fu...@postgresql.org>
Date: Wed, 21 Sep 2022 13:58:50 +0900
Subject: [PATCH 2/2] psql: Add PartialMatches() macro for better
tab-completion.
---
src/bin/psql/tab-complete.c | 152 +++++++++++++++++++++++++++---------
1 file changed, 113 insertions(+), 39 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 820f47d23a..0b8c252615 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1582,6 +1582,65 @@ HeadMatchesImpl(bool case_sensitive,
return true;
}
+/*
+ * Implementation of PartialMatches and PartialMatchesCS macros: do parts of
+ * the words in previous_words match the variadic arguments?
+ */
+static bool
+PartialMatchesImpl(bool case_sensitive,
+ int previous_words_count, char **previous_words,
+ int narg,...)
+{
+ va_list args;
+ char *firstarg = NULL;
+
+ if (previous_words_count < narg)
+ return false;
+
+ for (int startpos = 0; startpos < previous_words_count; startpos++)
+ {
+ int argno;
+
+ if (firstarg == NULL)
+ {
+ va_start(args, narg);
+ firstarg = va_arg(args, char *);
+ }
+
+ if (!word_matches(firstarg,
+ previous_words[previous_words_count - startpos - 1],
+ case_sensitive))
+ continue;
+
+ if (previous_words_count - startpos < narg)
+ {
+ va_end(args);
+ return false;
+ }
+
+ for (argno = 1; argno < narg; argno++)
+ {
+ const char *arg = va_arg(args, const char *);
+
+ if (!word_matches(arg,
+ previous_words[previous_words_count - argno - startpos - 1],
+ case_sensitive))
+ break;
+ }
+
+ va_end(args);
+ firstarg = NULL;
+
+ if (argno == narg)
+ return true;
+ }
+
+ if (firstarg != NULL)
+ va_end(args);
+
+ return false;
+}
+
/*
* Check if the final character of 's' is 'c'.
*/
@@ -1663,6 +1722,16 @@ psql_completion(const char *text, int start, int end)
HeadMatchesImpl(true, previous_words_count, previous_words, \
VA_ARGS_NARGS(__VA_ARGS__), __VA_ARGS__)
+ /* Match N words on the line partially, case-insensitively. */
+#define PartialMatches(...) \
+ PartialMatchesImpl(false, previous_words_count, previous_words, \
+ VA_ARGS_NARGS(__VA_ARGS__), __VA_ARGS__)
+
+ /* Match N words on the line partially, case-sensitively. */
+#define PartialMatchesCS(...) \
+ PartialMatchesImpl(true, previous_words_count, previous_words, \
+ VA_ARGS_NARGS(__VA_ARGS__), __VA_ARGS__)
+
/* Known command-starting keywords. */
static const char *const sql_commands[] = {
"ABORT", "ALTER", "ANALYZE", "BEGIN", "CALL", "CHECKPOINT", "CLOSE", "CLUSTER",
@@ -4108,46 +4177,51 @@ psql_completion(const char *text, int start, int end)
TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING", MatchAny, MatchAnyExcept("ON|AS")))
COMPLETE_WITH("ON");
- /* Complete MERGE INTO ... ON with target table attributes */
- else if (TailMatches("INTO", MatchAny, "USING", MatchAny, "ON"))
- COMPLETE_WITH_ATTR(prev4_wd);
- else if (TailMatches("INTO", MatchAny, "AS", MatchAny, "USING", MatchAny, "AS", MatchAny, "ON"))
- COMPLETE_WITH_ATTR(prev8_wd);
- else if (TailMatches("INTO", MatchAny, MatchAny, "USING", MatchAny, MatchAny, "ON"))
- COMPLETE_WITH_ATTR(prev6_wd);
+ else if (PartialMatches("MERGE", "INTO", MatchAny, "USING") ||
+ PartialMatches("MERGE", "INTO", MatchAny, "AS", MatchAny, "USING") ||
+ PartialMatches("MERGE", "INTO", MatchAny, MatchAny, "USING"))
+ {
+ /* Complete MERGE INTO ... ON with target table attributes */
+ if (TailMatches("INTO", MatchAny, "USING", MatchAny, "ON"))
+ COMPLETE_WITH_ATTR(prev4_wd);
+ else if (TailMatches("INTO", MatchAny, "AS", MatchAny, "USING", MatchAny, "AS", MatchAny, "ON"))
+ COMPLETE_WITH_ATTR(prev8_wd);
+ else if (TailMatches("INTO", MatchAny, MatchAny, "USING", MatchAny, MatchAny, "ON"))
+ COMPLETE_WITH_ATTR(prev6_wd);
- /*
- * Complete ... USING <relation> [[AS] alias] ON join condition
- * (consisting of one or three words typically used) with WHEN [NOT]
- * MATCHED
- */
- else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
- TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny) ||
- TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny) ||
- TailMatches("USING", MatchAny, "ON", MatchAny, MatchAnyExcept("WHEN"), MatchAnyExcept("WHEN")) ||
- TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny, MatchAnyExcept("WHEN"), MatchAnyExcept("WHEN")) ||
- TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny, MatchAnyExcept("WHEN"), MatchAnyExcept("WHEN")))
- COMPLETE_WITH("WHEN MATCHED", "WHEN NOT MATCHED");
- else if (TailMatches("USING", MatchAny, "ON", MatchAny, "WHEN") ||
- TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny, "WHEN") ||
- TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny, "WHEN") ||
- TailMatches("USING", MatchAny, "ON", MatchAny, MatchAny, MatchAny, "WHEN") ||
- TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny, MatchAny, MatchAny, "WHEN") ||
- TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny, MatchAny, MatchAny, "WHEN"))
- COMPLETE_WITH("MATCHED", "NOT MATCHED");
-
- /* Complete ... WHEN [NOT] MATCHED with THEN/AND */
- else if (TailMatches("WHEN", "MATCHED") ||
- TailMatches("WHEN", "NOT", "MATCHED"))
- COMPLETE_WITH("THEN", "AND");
-
- /* Complete ... WHEN MATCHED THEN with UPDATE SET/DELETE/DO NOTHING */
- else if (TailMatches("WHEN", "MATCHED", "THEN"))
- COMPLETE_WITH("UPDATE SET", "DELETE", "DO NOTHING");
-
- /* Complete ... WHEN NOT MATCHED THEN with INSERT/DO NOTHING */
- else if (TailMatches("WHEN", "NOT", "MATCHED", "THEN"))
- COMPLETE_WITH("INSERT", "DO NOTHING");
+ /*
+ * Complete ... USING <relation> [[AS] alias] ON join condition
+ * (consisting of one or three words typically used) with WHEN [NOT]
+ * MATCHED
+ */
+ else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
+ TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny) ||
+ TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny) ||
+ TailMatches("USING", MatchAny, "ON", MatchAny, MatchAnyExcept("WHEN"), MatchAnyExcept("WHEN")) ||
+ TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny, MatchAnyExcept("WHEN"), MatchAnyExcept("WHEN")) ||
+ TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny, MatchAnyExcept("WHEN"), MatchAnyExcept("WHEN")))
+ COMPLETE_WITH("WHEN MATCHED", "WHEN NOT MATCHED");
+ else if (TailMatches("USING", MatchAny, "ON", MatchAny, "WHEN") ||
+ TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny, "WHEN") ||
+ TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny, "WHEN") ||
+ TailMatches("USING", MatchAny, "ON", MatchAny, MatchAny, MatchAny, "WHEN") ||
+ TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny, MatchAny, MatchAny, "WHEN") ||
+ TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny, MatchAny, MatchAny, "WHEN"))
+ COMPLETE_WITH("MATCHED", "NOT MATCHED");
+
+ /* Complete ... WHEN [NOT] MATCHED with THEN/AND */
+ else if (TailMatches("WHEN", "MATCHED") ||
+ TailMatches("WHEN", "NOT", "MATCHED"))
+ COMPLETE_WITH("THEN", "AND");
+
+ /* Complete ... WHEN MATCHED THEN with UPDATE SET/DELETE/DO NOTHING */
+ else if (TailMatches("WHEN", "MATCHED", "THEN"))
+ COMPLETE_WITH("UPDATE SET", "DELETE", "DO NOTHING");
+
+ /* Complete ... WHEN NOT MATCHED THEN with INSERT/DO NOTHING */
+ else if (TailMatches("WHEN", "NOT", "MATCHED", "THEN"))
+ COMPLETE_WITH("INSERT", "DO NOTHING");
+ }
/* NOTIFY --- can be inside EXPLAIN, RULE, etc */
else if (TailMatches("NOTIFY"))
--
2.37.1
