On Thu, Sep 25, 2025 at 6:16 PM Yugo Nagata <[email protected]> wrote:
>
> On Thu, 25 Sep 2025 14:31:18 -0700
> Masahiko Sawada <[email protected]> wrote:
>
> > > > I fixed it to use 'generator'.
> >
> > LGTM. I've pushed the 0001 patch.
>
> Thank you!
>
> > > > > ---
> > > > > - /* Complete COPY <sth> FROM <sth> */
> > > > > - else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny))
> > > > > + /* Complete COPY <sth> FROM [PROGRAM] <sth> */
> > > > > + else if (Matches("COPY|\\copy", MatchAny, "FROM",
> > > > > MatchAnyExcept("PROGRAM")) ||
> > > > > + Matches("COPY|\\copy", MatchAny, "FROM", "PROGRAM",
> > > > > MatchAny))
> > > > >
> > > > > I see this kind of conversion many places in the patch; convert one
> > > > > condition with MatchAny into two conditions with
> > > > > MatchAnyExcept("PROGRAM") and '"PROGRAM", MatchAny'. How about
> > > > > simplifying it using MatchAnyN. For example,
> > > > >
> > > > > else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny,
> > > > > MatchAnyN))
> > > >
> > > > We could simplify this by using MatchAnyN, but doing so would cause
> > > > "WITH ("
> > > > or "WHERE" to be suggested after "WITH (...)", even though that is not
> > > > allowed
> > > > by the syntax. This could be misleading for users, so I wonder whether
> > > > it is
> > > > worth adding a bit of complexity to prevent possible confusion.
> > >
> > > There was a mistake in the previous statement: "WHERE" appearing after
> > > "WITH (...)"
> > > is actually correct. However, this also results in "WITH" being suggested
> > > after
> > > "WHERE", which is not permitted by the syntax.
> >
> > True. How about other places? That is, where we check the completion
> > after "WITH (". For example:
> >
> > - /* Complete COPY <sth> TO filename WITH ( */
> > - else if (Matches("COPY|\\copy", MatchAny, "TO", MatchAny, "WITH", "("))
> > + /* Complete COPY <sth> TO [PROGRAM] filename WITH ( */
> > + else if (Matches("COPY|\\copy", MatchAny, "TO",
> > MatchAnyExcept("PROGRAM"), "WITH", "(") ||
> > + Matches("COPY|\\copy", MatchAny, "TO", "PROGRAM",
> > MatchAny, "WITH", "("))
> >
> > Does it make sense to replace these two lines with the following one line?
> >
> > else if (Matches("COPY|\\copy", MatchAny, "TO", MatchAny, MatchAnyN,
> > "WITH", "("))
>
> That works for other places where options are suggested after "WITH (" and
> "WHERE" is suggested after "WITH (*)".
>
> I've attached updated patches using MatchAnyN following your suggestion.
Thank you for updating the patch!
After reviewing both the v6 and v7 patches, I realize that your
original approach in v6 is actually better than what I suggested.
While it requires more lines of code, it provides more precise
completions. Additionally, since most of these extra lines are removed
by the next patch (v6-0003), the code size isn't really an issue.
Would it be possible to withdraw my previous comments and proceed with
the v6 approach? I apologize for the back-and-forth on this.
I have two review comments about the complete_from_files() function:
+ * This function wraps _complete_from_files() so that both literal keywords
+ * and filenames can be included in the completion results.
+ *
+ * If completion_charpp is set to a null-terminated array of literal keywords,
+ * those keywords are added to the completion results alongside filenames,
+ * as long as they case-insensitively match the current input.
How about rephrasing the comments to the following?
/*
* This function returns in order one of a fixed, NULL pointer terminated list
* of string that matches file names or optionally specified list of keywords.
*
* If completion_charpp is set to a null-terminated array of literal keywords,
* those keywords are added to the completion results alongside filenames if
* they case-insensitively match the current input.
*/
---
+ /* Return a filename that matches */
+ if (!files_done && (result = _complete_from_files(text, state)))
+ return result;
+ else if (!completion_charpp)
+ return NULL;
+ else
+ files_done = true;
It works but it's odd a bit that we don't set files_done to true if
completion_charpp is not NULL. I think it becomes more readable if we
could set files_done to true if _complete_from_files() doesn't return
a string and proceed with the hard-wired keywords.
The attached patch that can be applied on top of v6-0002 patch,
implements my suggestions and includes pgindent fixes. Please review
these changes.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index a8041401b8b..c4e40042048 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -3339,7 +3339,7 @@ match_previous_words(int pattern_id,
else if (Matches("COPY|\\copy", MatchAny, "FROM|TO"))
{
/* COPY requires quoted filename */
- bool force_quote = HeadMatches("COPY");
+ bool force_quote = HeadMatches("COPY");
if (TailMatches("FROM"))
COMPLETE_WITH_FILES_PLUS("", force_quote, "STDIN",
"PROGRAM");
@@ -3349,7 +3349,8 @@ match_previous_words(int pattern_id,
/* Complete COPY|\copy <sth> FROM|TO PROGRAM command */
else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM"))
- COMPLETE_WITH_FILES("", HeadMatches("COPY")); /* COPY
requires quoted filename */
+ COMPLETE_WITH_FILES("", HeadMatches("COPY")); /* COPY
requires quoted
+
* filename */
/* Complete COPY <sth> TO [PROGRAM] <sth> */
else if (Matches("COPY|\\copy", MatchAny, "TO",
MatchAnyExcept("PROGRAM")) ||
@@ -6271,19 +6272,18 @@ complete_from_variables(const char *text, const char
*prefix, const char *suffix
/*
- * This function wraps _complete_from_files() so that both literal keywords
- * and filenames can be included in the completion results.
+ * This function returns in order one of a fixed, NULL pointer terminated list
+ * of string that matches file names or optionally specified list of keywords.
*
* If completion_charpp is set to a null-terminated array of literal keywords,
- * those keywords are added to the completion results alongside filenames,
- * as long as they case-insensitively match the current input.
+ * those keywords are added to the completion results alongside filenames if
+ * they case-insensitively match the current input.
*/
static char *
complete_from_files(const char *text, int state)
{
- char *result;
static int list_index;
- static bool files_done;
+ static bool files_done;
const char *item;
/* Initialization */
@@ -6293,18 +6293,21 @@ complete_from_files(const char *text, int state)
files_done = false;
}
- /* Return a filename that matches */
- if (!files_done && (result = _complete_from_files(text, state)))
- return result;
- else if (!completion_charpp)
- return NULL;
- else
+ if (!files_done)
+ {
+ char *result = _complete_from_files(text, state);
+
+ /* Return a filename that matches */
+ if (result)
+ return result;
+
+ /* There are no more matching files */
files_done = true;
+ }
/*
- * If there are no more matching files, check for hard-wired keywords.
- * These will only be returned if they match the input-so-far,
- * ignoring case.
+ * Check for hard-wired keywords. These will only be returned if they
+ * match the input-so-far, ignoring case.
*/
while ((item = completion_charpp[list_index++]))
{
