On Wed, Sep 17, 2025 at 8:27 PM Yugo Nagata <[email protected]> wrote:
>
> On Thu, 18 Sep 2025 12:05:06 +0900
> Yugo Nagata <[email protected]> wrote:
>
> > On Wed, 17 Sep 2025 12:53:10 -0700
> > Masahiko Sawada <[email protected]> wrote:
> >
> > > On Fri, Jul 18, 2025 at 12:49 AM Yugo Nagata <[email protected]> wrote:
> > > >
> > > > On Thu, 17 Jul 2025 10:57:36 +0900
> > > > Yugo Nagata <[email protected]> wrote:
> > > >
> > > > > On Tue, 17 Jun 2025 00:08:32 +0900
> > > > > Yugo Nagata <[email protected]> wrote:
> > > > >
> > > > > > On Thu, 5 Jun 2025 16:52:00 +0900
> > > > > > Yugo Nagata <[email protected]> wrote:
> > > > > >
> > > > > > > On Thu, 5 Jun 2025 10:08:35 +0900
> > > > > > > Yugo Nagata <[email protected]> wrote:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Currently, tab completion for COPY only suggests filenames
> > > > > > > > after TO or
> > > > > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid
> > > > > > > > syntax options.
> > > > > > > >
> > > > > > > > I'd like to propose improving the tab completion behavior as
> > > > > > > > described in
> > > > > > > > the subject, so that these keywords are suggested
> > > > > > > > appropriately, and filenames
> > > > > > > > are offered as potential command names after the PROGRAM
> > > > > > > > keyword.
> > > > > > > >
> > > > > > > > I've attached this proposal as a patch series with the
> > > > > > > > following three parts:
> > > > > > >
> > > > > > > I'm sorry but the previous patches were accidentally broken and
> > > > > > > didn't work.
> > > > > > > I've attached fixed patches.
> > > > > > >
> > > > > > > >
> > > > > > > > 0001: Refactor match_previous_words() to remove direct use of
> > > > > > > > rl_completion_matches()
> > > > > > > >
> > > > > > > > This is a preparatory cleanup. Most completions in
> > > > > > > > match_previous_words() already use
> > > > > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches().
> > > > > > > > However, some direct calls
> > > > > > > > still remain.
> > > > > > > >
> > > > > > > > This patch replaces the remaining direct calls with
> > > > > > > > COMPLETE_WITH_FILES or
> > > > > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability.
> > > > > > > >
> > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN,
> > > > > > > > STDOUT, and PROGRAM
> > > > > > > >
> > > > > > > > This is the main patch. It extends tab completion to suggest
> > > > > > > > STDIN, STDOUT, and PROGRAM
> > > > > > > > after TO or FROM. After PROGRAM, filenames are suggested as
> > > > > > > > possible command names.
> > > > > > > >
> > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is
> > > > > > > > introduced. This allows
> > > > > > > > combining literal keywords with filename suggestions in the
> > > > > > > > completion list.
> > > > > > > >
> > > > > > > > 0003: Improve tab completion for COPY option lists
> > > > > > > >
> > > > > > > > Currently, only the first option in a parenthesized list is
> > > > > > > > suggested during completion,
> > > > > > > > and nothing is suggested after a comma.
> > > > > > > >
> > > > > > > > This patch enables suggestions after each comma, improving
> > > > > > > > usability when specifying
> > > > > > > > multiple options.
> > > > > > > >
> > > > > > > > Although not directly related to the main proposal, I believe
> > > > > > > > this is a helpful enhancement
> > > > > > > > to COPY tab completion and included it here for completeness.
> > > > > > > >
> > > > > > > > I’d appreciate your review and feedback on this series.
> > > >
> > > > The previous patch was broken failed to complie since I missed following
> > > > the required format of if-conditions in match_previous_words().
> > > > I've attached update patches.
> > > >
> > >
> > > I agree with the basic direction of the patches. Here are some
> > > comments on the first two patches:
> >
> > Thank you for reviewing it.
> > I've attached an updated patch.
Thank you for updating the patches!
> >
> > >
> > > v5-0001-Refactor-match_previous_words-to-remove-direct-us.patch:
> > >
> > > ---
> > > +#define COMPLETE_WITH_GENERATOR(function) \
> > > + matches = rl_completion_matches(text, function)
> > >
> > > I think it would be clearer if we use 'generator' or 'genfunc' instead
> > > of 'function' as a macro argument.
> >
> > I fixed it to use 'generator'.
LGTM. I've pushed the 0001 patch.
> >
> > >
> > > v5-0002-Add-tab-completion-support-for-COPY-.-TO-FROM-STD.patch:
> > >
> > > ---
> > > + /* 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 */
> > >
> > > Why does it complete the query with files names even after 'PROGRAM'?
> >
> > Users can specify the command by giving a filename with an absolute or
> > relative path, so I think it makes sense to allow filename completion
> > after PROGRAM.
Agreed.
> >
> > > ---
> > > +static char *
> > > +_complete_from_files(const char *text, int state)
> > > {
> > >
> > > I think the comments of complete_from_files() should be moved to this
> > > new function. For instance, the comments starts with:
> > >
> > > * This function wraps rl_filename_completion_function() to strip quotes
> > > from
> > > * the input before searching for matches and to quote any matches for
> > > which
> > > * the consuming command will require it.
> > >
> > > But complete_from_files() function no longer calls
> > > rl_filename_completion_function().
> >
> > I moved the comments to the top of _complete_from_files() and added a new
> > comment for complete_from_files() to describe that it is a wrapper of the
> > former.
> >
> > > ---
> > > - /* 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", "("))
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
