Thread: Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
[ redirecting to -hackers ]
I wrote:
> Yeah, it seems like a bad idea to override the user's choice to write
> a quote, even if one is not formally necessary. I propose the attached.
After further experimentation, I concluded that that patch is a bad idea;
it breaks a lot of cases that used to work before. It turns out that
Readline has a bunch of behaviors for filename completion that occur
outside of the rl_filename_completion_function function proper, and they
all assume that what's passed back from that function is plain unquoted
filename(s). Notably, completion of a path that includes directory names
just doesn't work well at all anymore with that patch ... nor did it
work well before, if the path contained characters that we thought we
should quote.
The right way to do things, seemingly, is to let
rl_filename_completion_function be invoked without any interference,
and instead put our SQL-aware quoting/dequoting logic into the hooks
Readline provides for that purpose, rl_filename_quoting_function and
rl_filename_dequoting_function. (It appears that somebody tried to do
that before, way back around the turn of the century, but gave up on it.
Too bad, because it's the right thing.)
Of course, this only works if we have those hooks :-(. So far as
I can tell, all libreadline variants that might still exist in the wild
do have them; but libedit doesn't, at least not in the version Apple
is shipping. Hence, what the attached patch does is to make configure
probe for the existence of the hook variables; if they're not there,
fall back to what I proposed previously. The behavior on libedit is
a bit less nice than one could wish, but it's better than what we have
now.
I've tested this on the oldest and newest readline versions I have at
hand (4.2a and 8.0), as well as the oldest and newest versions of
Apple's libedit derivative; but I haven't tried it on whatever the
BSDen are shipping as libedit.
There's enough moving parts here that this probably needs to go through
a full review cycle, so I'll add it to the next commitfest. Some notes
for anybody wanting to review:
* The patch now always quotes completed filenames, so quote_if_needed()
is misnamed and overcomplicated for this use-case. I left the extra
generality in place for possible future use. On the other hand, this
is the *only* use-case, so you could also argue that we should just
simplify the function's API. I have no strong opinion about that.
* In addition to the directly-related-to-filenames changes, it turns out
to be necessary to set rl_completer_quote_characters to include at least
single quotes, else Readline doesn't understand that a quoted filename
is quoted. The patch sets it to include double quotes as well. This
is probably the aspect of the patch that most needs testing. The general
effect of this change is that Readline now understands that quoted
strings are single entities, plus it will try to complete the contents
of a string if you ask it. The side-effects I've noticed seem to be
all beneficial -- for example, if you do
select * from "foo<TAB>
it now correctly completes table names starting with "foo", which it
did not before. But there might be some bad effects as well. Also,
although libedit has this variable, setting it doesn't have that effect
there; I've not really found that the variable does anything at all there.
* The behavior of quote_file_name is directly modeled on what Readline's
default implementation rl_quote_filename does, except that it uses
SQL-aware quoting rules. The business of passing back the final quote
mark separately is their idea.
* An example of the kind of case that's interesting is that if you type
\lo_import /usr/i<TAB>
then what you get on readline (with this patch) is
\lo_import '/usr/include/
while libedit produces
\lo_import '/usr/include' (with a space after the trailing quote)
That is, readline knows that the completion-so-far is a directory and
assumes that's not all you want, whereas libedit doesn't know that.
So you typically now have to back up two characters, type slash, and
resume completing. That's kind of a pain, but I'm not sure we can
make it better very easily. Anyway, libedit did something close to
that before, too.
* There are also some interesting cases around special characters in
the filename. It seems to work well for embedded spaces, not so well
for embedded single quotes, though that may well vary across readline
versions. Again, there seems to be a limited amount we can do about
that, given how much of the relevant logic is buried where we can't
modify it. And I'm not sure how much I care about that case, anyway.
regards, tom lane
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..e5bf980 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,11 +209,12 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---------------------------------------
+# PGAC_READLINE_VARIABLES
+# -----------------------
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
#ifdef HAVE_READLINE_READLINE_H
@@ -228,7 +229,38 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#ifdef HAVE_READLINE_READLINE_H
+# include <readline/readline.h>
+#elif defined(HAVE_READLINE_H)
+# include <readline.h>
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#ifdef HAVE_READLINE_READLINE_H
+# include <readline/readline.h>
+#elif defined(HAVE_READLINE_H)
+# include <readline.h>
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 6b1c779..97556b8 100755
--- a/configure
+++ b/configure
@@ -16409,6 +16409,81 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#ifdef HAVE_READLINE_READLINE_H
+# include <readline/readline.h>
+#elif defined(HAVE_READLINE_H)
+# include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; }
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting_function" >&5
+$as_echo_n "checking for rl_filename_quoting_function... " >&6; }
+if ${pgac_cv_var_rl_filename_quoting_function+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#ifdef HAVE_READLINE_READLINE_H
+# include <readline/readline.h>
+#elif defined(HAVE_READLINE_H)
+# include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quoting_function = 0;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quoting_function=yes
+else
+ pgac_cv_var_rl_filename_quoting_function=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quoting_function" >&5
+$as_echo "$pgac_cv_var_rl_filename_quoting_function" >&6; }
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTING_FUNCTION 1" >>confdefs.h
+
+fi
+
for ac_func in rl_completion_matches rl_filename_completion_function rl_reset_screen_size
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
diff --git a/configure.in b/configure.in
index 2b9025c..07c00d9 100644
--- a/configure.in
+++ b/configure.in
@@ -1867,7 +1867,7 @@ fi
LIBS="$LIBS_including_readline"
if test "$with_readline" = yes; then
- PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+ PGAC_READLINE_VARIABLES
AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function rl_reset_screen_size])
AC_CHECK_FUNCS([append_history history_truncate_file])
fi
diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index 8c8b4c2..23a6f9f 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
* entails_quote - any of these present? need outer quotes
* quote - doubled within string, affixed to both ends
* escape - doubled within string
+ * force_quote - if true, quote the output even if it doesn't "need" it
* encoding - the active character-set encoding
*
* Do not use this as a substitute for PQescapeStringConn(). Use it for
@@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, int encoding)
*/
char *
quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding)
+ char quote, char escape, bool force_quote,
+ int encoding)
{
const char *src;
char *ret;
char *dst;
- bool need_quotes = false;
+ bool need_quotes = force_quote;
Assert(source != NULL);
Assert(quote != '\0');
diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h
index 16a7af0..fa00141 100644
--- a/src/bin/psql/stringutils.h
+++ b/src/bin/psql/stringutils.h
@@ -22,6 +22,7 @@ extern char *strtokx(const char *s,
extern void strip_quotes(char *source, char quote, char escape, int encoding);
extern char *quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding);
+ char quote, char escape, bool force_quote,
+ int encoding);
#endif /* STRINGUTILS_H */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 2b1e3cd..a1d64f0 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -53,7 +53,7 @@
#ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
#define filename_completion_function rl_filename_completion_function
#else
-/* missing in some header files */
+/* decl missing in some header files, but function exists anyway */
extern char *filename_completion_function();
#endif
@@ -61,6 +61,15 @@ extern char *filename_completion_function();
#define completion_matches rl_completion_matches
#endif
+/*
+ * Currently we assume that rl_filename_dequoting_function exists if
+ * rl_filename_quoting_function does. If that proves not to be the case,
+ * we'd need to test for the former, or possibly both, in configure.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTING_FUNCTION
+#define USE_FILENAME_QUOTING_FUNCTIONS 1
+#endif
+
/* word break characters */
#define WORD_BREAKS "\t\n@$><=;|&{() "
@@ -1112,9 +1121,9 @@ static char **get_previous_words(int point, char **buffer, int *nwords);
static char *get_guctype(const char *varname);
-#ifdef NOT_USED
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
static char *quote_file_name(char *text, int match_type, char *quote_pointer);
-static char *dequote_file_name(char *text, char quote_char);
+static char *dequote_file_name(char *text, int quote_char);
#endif
@@ -1127,8 +1136,32 @@ initialize_readline(void)
rl_readline_name = (char *) pset.progname;
rl_attempted_completion_function = psql_completion;
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ rl_filename_quoting_function = quote_file_name;
+ rl_filename_dequoting_function = dequote_file_name;
+#endif
+
rl_basic_word_break_characters = WORD_BREAKS;
+ rl_completer_quote_characters = "'\"";
+
+ /*
+ * Set rl_filename_quote_characters to "all possible characters",
+ * otherwise Readline will skip filename quoting if it thinks a filename
+ * doesn't need quoting. Readline actually interprets this as bytes, so
+ * there are no encoding considerations here.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+ {
+ unsigned char *fqc = (unsigned char *) pg_malloc(256);
+
+ for (int i = 0; i < 255; i++)
+ fqc[i] = (unsigned char) (i + 1);
+ fqc[255] = '\0';
+ rl_filename_quote_characters = (const char *) fqc;
+ }
+#endif
+
completion_max_records = 1000;
/*
@@ -4343,12 +4380,31 @@ complete_from_variables(const char *text, const char *prefix, const char *suffix
/*
* 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.
+ * the input before searching for matches and then re-quote the matches.
+ * (We always quote filenames, even though for some psql meta-commands and
+ * some filenames it wouldn't be strictly necessary.)
+ *
+ * Caller must set completion_charp to a zero- or one-character string
+ * containing the escape character. This is necessary since \copy has no
+ * escape character, but every other backslash command recognizes "\" as an
+ * escape character. Since we have only two callers, don't bother providing
+ * a macro to simplify this.
*/
static char *
complete_from_files(const char *text, int state)
{
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ /*
+ * If we're using a version of Readline that exports filename quoting
+ * hooks, use those, and invoke rl_filename_completion_function without
+ * any funny business. Readline does stuff internally that does not work
+ * well at all if we try to handle dequoting here.
+ */
+ return filename_completion_function(text, state);
+#else
+ /*
+ * Otherwise, we have to do the best we can.
+ */
static const char *unquoted_text;
char *unquoted_match;
char *ret = NULL;
@@ -4369,15 +4425,11 @@ complete_from_files(const char *text, int state)
unquoted_match = filename_completion_function(unquoted_text, state);
if (unquoted_match)
{
- /*
- * Caller sets completion_charp to a zero- or one-character string
- * containing the escape character. This is necessary since \copy has
- * no escape character, but every other backslash command recognizes
- * "\" as an escape character. Since we have only two callers, don't
- * bother providing a macro to simplify this.
- */
+ /* Re-quote the result. */
ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
- '\'', *completion_charp, pset.encoding);
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
if (ret)
free(unquoted_match);
else
@@ -4385,6 +4437,7 @@ complete_from_files(const char *text, int state)
}
return ret;
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
}
@@ -4636,46 +4689,60 @@ get_guctype(const char *varname)
return guctype;
}
-#ifdef NOT_USED
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
/*
- * Surround a string with single quotes. This works for both SQL and
- * psql internal. Currently disabled because it is reported not to
- * cooperate with certain versions of readline.
+ * Quote a filename according to SQL rules, returning a malloc'd string.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
*/
static char *
quote_file_name(char *text, int match_type, char *quote_pointer)
{
char *s;
- size_t length;
- (void) quote_pointer; /* not used */
+ /* We always quote (so quote_if_needed's API is overkill) */
+ s = quote_if_needed(text, " \t\r\n\"`",
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
+
+ /*
+ * If quote_pointer is passed, it's better to handle the trailing quote
+ * that way.
+ */
+ if (quote_pointer && s && *s)
+ {
+ char *send = s + strlen(s) - 1;
- length = strlen(text) +(match_type == SINGLE_MATCH ? 3 : 2);
- s = pg_malloc(length);
- s[0] = '\'';
- strcpy(s + 1, text);
- if (match_type == SINGLE_MATCH)
- s[length - 2] = '\'';
- s[length - 1] = '\0';
+ Assert(*send == '\'');
+ *send = '\0';
+ *quote_pointer = '\'';
+ }
return s;
}
+/*
+ * Dequote a filename, if it's quoted.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
+ */
static char *
-dequote_file_name(char *text, char quote_char)
+dequote_file_name(char *text, int quote_char)
{
- char *s;
- size_t length;
-
- if (!quote_char)
- return pg_strdup(text);
+ char *unquoted_text = strtokx(text, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
- length = strlen(text);
- s = pg_malloc(length - 2 + 1);
- strlcpy(s, text +1, length - 2 + 1);
-
- return s;
+ /* expect a NULL return for the empty string only */
+ if (!unquoted_text)
+ {
+ Assert(*text == '\0');
+ unquoted_text = text;
+ }
+ /* readline expects a malloc'd result that it is to free */
+ return pg_strdup(unquoted_text);
}
-#endif /* NOT_USED */
+
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
#endif /* USE_READLINE */
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 939245d..e3baae8 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -492,6 +492,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
#undef HAVE_RL_FILENAME_COMPLETION_FUNCTION
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+#undef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+#undef HAVE_RL_FILENAME_QUOTING_FUNCTION
+
/* Define to 1 if you have the `rl_reset_screen_size' function. */
#undef HAVE_RL_RESET_SCREEN_SIZE
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 36e6bdc..1ca90b1 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -355,6 +355,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
/* #undef HAVE_RL_FILENAME_COMPLETION_FUNCTION */
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+/* #undef HAVE_RL_FILENAME_QUOTE_CHARACTERS */
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+/* #undef HAVE_RL_FILENAME_QUOTING_FUNCTION */
+
/* Define to 1 if you have the <security/pam_appl.h> header file. */
/* #undef HAVE_SECURITY_PAM_APPL_H */
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Alvaro Herrera
Date:
All in all, after testing this for a bit, I think this patch is a clear improvement over the statu quo. Thanks for working on this. I suggest to indicate in complete_from_files where to find the hook functions it refers to (say "see quote_file_name, below", or something.) I tested this on libreadline 7.x (where #define HAVE_RL_FILENAME_COMPLETION_FUNCTION 1). I noticed that if I enter a filename that doesn't exist and then <tab>, it adds a closing quote. Bash manages to do nothing somehow, which is the desired behavior IMO. (I tried to make sense of these hooks, but couldn't readily and I don't have the readline documentation installed, so I have no opinion on whether this problem is fixable. Maybe the trick is to let quote_if_needed know that this is a completion for a filename, and have it test for file existence?) Also, some commands such as \cd want a directory rather than just any file. Not sure rl_filename_completion_function has a way to pass this down. (This point is a bit outside this patch's charter perhaps, but may as well think about it since we're here ...) I don't quite understand why a readline library that doesn't have rl_filename_completion_function is known to have a filename_completion_function, ie. this bit #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION #define filename_completion_function rl_filename_completion_function #else /* decl missing in some header files, but function exists anyway */ extern char *filename_completion_function(); #endif What's going on here? How does this ever work? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Robert Haas
Date:
On Sun, Nov 3, 2019 at 5:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Of course, this only works if we have those hooks :-(. So far as > I can tell, all libreadline variants that might still exist in the wild > do have them; but libedit doesn't, at least not in the version Apple > is shipping. Hence, what the attached patch does is to make configure > probe for the existence of the hook variables; if they're not there, > fall back to what I proposed previously. Are people still compiling against libedit and then redirecting to libreadline at runtime? I seem to recall some discussion about this being a thing, many years ago. If it were being done it would be advantageous to have the checks be runtime rather than compile-time, although I guess that would probably be tough to make work. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Alvaro Herrera
Date:
On 2019-Dec-11, Robert Haas wrote: > On Sun, Nov 3, 2019 at 5:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Of course, this only works if we have those hooks :-(. So far as > > I can tell, all libreadline variants that might still exist in the wild > > do have them; but libedit doesn't, at least not in the version Apple > > is shipping. Hence, what the attached patch does is to make configure > > probe for the existence of the hook variables; if they're not there, > > fall back to what I proposed previously. > > Are people still compiling against libedit and then redirecting to > libreadline at runtime? I seem to recall some discussion about this > being a thing, many years ago. Yeah, Debian did that out of licensing concerns. It seems they still do that, based on https://packages.debian.org/bullseye/postgresql-client-12 > If it were being done it would be > advantageous to have the checks be runtime rather than compile-time, > although I guess that would probably be tough to make work. Yeah. On the other hand, I suppose Debian uses the BSD version of the libraries, not the Apple version, so I think it should be fine? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Alvaro Herrera
Date:
On 2019-Dec-11, Alvaro Herrera wrote: > On 2019-Dec-11, Robert Haas wrote: > > If it were being done it would be > > advantageous to have the checks be runtime rather than compile-time, > > although I guess that would probably be tough to make work. > > Yeah. On the other hand, I suppose Debian uses the BSD version of the > libraries, not the Apple version, so I think it should be fine? ... actually, grepping libedit's source at http://deb.debian.org/debian/pool/main/libe/libedit/libedit_3.1-20191025.orig.tar.gz there's no occurrence of rl_filename_quoting_function. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > On 2019-Dec-11, Robert Haas wrote: >> Are people still compiling against libedit and then redirecting to >> libreadline at runtime? I seem to recall some discussion about this >> being a thing, many years ago. > Yeah, Debian did that out of licensing concerns. It seems they still do > that, based on > https://packages.debian.org/bullseye/postgresql-client-12 I think it's Debian's problem, not ours, if that doesn't work. It is not unreasonable for a package to probe existence of a library function at configure time. It's up to them to make sure that the headers match the actual library. regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Robert Haas
Date:
On Wed, Dec 11, 2019 at 10:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think it's Debian's problem, not ours, if that doesn't work. It is > not unreasonable for a package to probe existence of a library function > at configure time. It's up to them to make sure that the headers match > the actual library. That seems like an unhelpful attitude. Debian is a mainstream platform, and no doubt feels that they have important reasons for what they are doing. That's not to say that I'm against the patch, but I don't believe it's right to treat the concerns of mainstream Linux distributions in anything less than a serious manner. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 11, 2019 at 10:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think it's Debian's problem, not ours, if that doesn't work. It is
>> not unreasonable for a package to probe existence of a library function
>> at configure time. It's up to them to make sure that the headers match
>> the actual library.
> That seems like an unhelpful attitude. Debian is a mainstream
> platform, and no doubt feels that they have important reasons for what
> they are doing.
Nonetheless, if they're doing that, it's *their* bug not ours when
the run-time library fails to match what was supplied to compile
against. I think it would fall to them to patch either libedit
or readline to make those two agree. This is not different in any
way from the expectation that a platform supply a libc whose ABI
is stable.
In any case, this discussion is a bit hypothetical isn't it?
If I understand correctly, your concern is that the proposed
patch might fail to take advantage of functionality that actually
might be present at runtime. So what? It's no worse than before.
More, it's likely that there are other similar losses of functionality
already in our code and/or other peoples'. Debian bought into that
tradeoff, not us.
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I tested this on libreadline 7.x (where #define
> HAVE_RL_FILENAME_COMPLETION_FUNCTION 1). I noticed that if I enter a
> filename that doesn't exist and then <tab>, it adds a closing quote.
> Bash manages to do nothing somehow, which is the desired behavior IMO.
Hmm. I'll take a look, but I'm not terribly hopeful. I have looked
briefly at what Bash does for filename completion, and as I recall
it was massive, spaghetti-ish, and way too much in bed with various
implementation details of libreadline --- they don't pretend to work
with libedit. I'm not prepared to go there. It's reasonable for Bash
to expend huge effort on filename completion, because that's such a core
use-case for them, but I don't think it deserves as much work in psql.
> I don't quite understand why a readline library that doesn't have
> rl_filename_completion_function is known to have a
> filename_completion_function, ie. this bit
> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
> #define filename_completion_function rl_filename_completion_function
> #else
> /* decl missing in some header files, but function exists anyway */
> extern char *filename_completion_function();
> #endif
I think the point is that before rl_filename_completion_function the
function existed but was just called filename_completion_function.
It's possible that that's obsolete --- I've not really checked.
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I don't quite understand why a readline library that doesn't have
>> rl_filename_completion_function is known to have a
>> filename_completion_function, ie. this bit
>> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
>> #define filename_completion_function rl_filename_completion_function
>> #else
>> /* decl missing in some header files, but function exists anyway */
>> extern char *filename_completion_function();
>> #endif
> I think the point is that before rl_filename_completion_function the
> function existed but was just called filename_completion_function.
> It's possible that that's obsolete --- I've not really checked.
I had a look through the buildfarm results, and it seems that the only
(non-Windows) animals that don't HAVE_RL_FILENAME_COMPLETION_FUNCTION
are prairiedog and locust. prairiedog is using the libedit that
Apple supplied in its stone-age version of macOS, and I imagine the
same can be said of locust, though that's one macOS release newer.
prairiedog's version does define filename_completion_function:
$ grep completion_func /usr/include/readline/readline.h
extern CPPFunction *rl_attempted_completion_function;
char *filename_completion_function(const char *, int);
char *username_completion_function(const char *, int);
so the assumption embodied in our code is both correct and necessary
so far as the current universe of buildfarm critters is concerned.
Having said that, prairiedog's version of libedit is buggy as hell;
it generates bogus warnings at every psql exit, for instance.
$ psql postgres
psql (13devel)
Type "help" for help.
postgres=# \q
could not save history to file "/Users/tgl/.psql_history": operating system error 0
$
It wouldn't be an unreasonable idea to desupport this version,
if it allowed additional simplifications in psql beside this
particular #ifdef mess. I'm not sure whether any of the
contortions in e.g. saveHistory could go away if we did so.
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
I wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> I don't quite understand why a readline library that doesn't have
>>> rl_filename_completion_function is known to have a
>>> filename_completion_function, ie. this bit
>>> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
>>> #define filename_completion_function rl_filename_completion_function
>>> #else
>>> /* decl missing in some header files, but function exists anyway */
>>> extern char *filename_completion_function();
>>> #endif
>> I think the point is that before rl_filename_completion_function the
>> function existed but was just called filename_completion_function.
>> It's possible that that's obsolete --- I've not really checked.
Looking closer at this, the "extern" could be got rid of, I think.
prairiedog's readline header does have that extern, so it's hard to
believe anybody is still using libedit versions that don't declare it.
A possible further change is to switch the code over to calling
"rl_filename_completion_function", and then invert the sense of
this logic, like
/*
* Ancient versions of libedit provide filename_completion_function()
* instead of rl_filename_completion_function().
*/
#ifndef HAVE_RL_FILENAME_COMPLETION_FUNCTION
#define rl_filename_completion_function filename_completion_function
#endif
This would make it easier to compare our code to the readline
documentation, so maybe it's helpful ... or maybe it's just
churn. Thoughts?
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Alvaro Herrera
Date:
On 2019-Dec-13, Tom Lane wrote: > I wrote: > >> Alvaro Herrera <alvherre@2ndquadrant.com> writes: > >>> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION > >>> #define filename_completion_function rl_filename_completion_function > >>> #else > >>> /* decl missing in some header files, but function exists anyway */ > >>> extern char *filename_completion_function(); > >>> #endif > Looking closer at this, the "extern" could be got rid of, I think. > prairiedog's readline header does have that extern, so it's hard to > believe anybody is still using libedit versions that don't declare it. Agreed. > A possible further change is to switch the code over to calling > "rl_filename_completion_function", and then invert the sense of > this logic, like > > /* > * Ancient versions of libedit provide filename_completion_function() > * instead of rl_filename_completion_function(). > */ > #ifndef HAVE_RL_FILENAME_COMPLETION_FUNCTION > #define rl_filename_completion_function filename_completion_function > #endif > > This would make it easier to compare our code to the readline > documentation, so maybe it's helpful ... or maybe it's just > churn. Thoughts? +1, I think that's clearer. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Dec-13, Tom Lane wrote:
>> A possible further change is to switch the code over to calling
>> "rl_filename_completion_function", and then invert the sense of
>> this logic, like ...
> +1, I think that's clearer.
OK, I went ahead and pushed that change, since it seems separate
and uncontroversial. I'll send along a new patch for the main
change in a little bit.
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Dec 11, 2019 at 10:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think it's Debian's problem, not ours, if that doesn't work. It is
>>> not unreasonable for a package to probe existence of a library function
>>> at configure time. It's up to them to make sure that the headers match
>>> the actual library.
>> That seems like an unhelpful attitude. Debian is a mainstream
>> platform, and no doubt feels that they have important reasons for what
>> they are doing.
Actually, this argument is based on a false premise anyhow. I took
a look into Debian's source package, and AFAICS they are not doing
anything as weird as a run-time substitution. They are just filling
the build environment with libedit-dev not libreadline-dev. So that
is certainly a supported configuration from our side, and if there
is any header-to-library discrepancy then it's just a simple bug
in the libedit package.
(Maybe at one time they were doing something weird; I didn't look
back further than the current sources for PG 12.)
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I suggest to indicate in complete_from_files where to find the hook
> functions it refers to (say "see quote_file_name, below", or something.)
Done.
> I tested this on libreadline 7.x (where #define
> HAVE_RL_FILENAME_COMPLETION_FUNCTION 1). I noticed that if I enter a
> filename that doesn't exist and then <tab>, it adds a closing quote.
> Bash manages to do nothing somehow, which is the desired behavior IMO.
Fixed --- on looking closer, I'd drawn the wrong conclusions from
looking at readline's default implementation of the quoting function
(which seems to be a tad broken, at least in the version I looked at).
It turns out that there are some special cases we need to handle if
we want it to behave nicely.
> Also, some commands such as \cd want a directory rather than just any
> file. Not sure rl_filename_completion_function has a way to pass this
> down. (This point is a bit outside this patch's charter perhaps, but
> may as well think about it since we're here ...)
I ended up adding an S_ISDIR stat check in the completion function,
because the desired behavior of terminating a directory name with '/'
(and no quote) doesn't seem to be possible to get otherwise. So it would
be possible to do something different for \cd, but I am not clear that
there's any real advantage. You can't really guess if the user wants the
currently completable directory or a subdirectory, so it wouldn't do to
emit a closing quote.
I've now spent some effort on hacking the libedit code path (i.e. the
one where we don't have the hooks) as well as the libreadline path.
This version of the patch seems to behave well on all the following:
* readline 6.0 (RHEL 6)
* readline 8.0 (Fedora 30)
* libedit 3.1 (Debian stretch)
* whatever libedit Apple is shipping in current macOS
I also tried it on ancient libedits from prairiedog and some other
old macOS releases. There are cosmetic issues there (e.g. prairiedog
wants to double the slash after a directory name) but I doubt we care
enough to fix them. It does compile and more-or-less work.
I noticed along the way that configure's probe for
rl_completion_append_character fails if we're using <editline/readline.h>,
because that configure macro was never taught to honor
HAVE_EDITLINE_READLINE_H. This might account for weird behavior on
libedit builds, perhaps. Arguably that could be a back-patchable bug fix,
but I'm disinclined to do so because it might break peoples' muscle memory
about whether a space needs to be typed after a completion; not a great
idea in a minor release.
regards, tom lane
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---------------------------------------
+# PGAC_READLINE_VARIABLES
+# -----------------------
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 3d9bd0b..2635157 100755
--- a/configure
+++ b/configure
@@ -16322,10 +16322,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
int
@@ -16351,6 +16353,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; }
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting_function" >&5
+$as_echo_n "checking for rl_filename_quoting_function... " >&6; }
+if ${pgac_cv_var_rl_filename_quoting_function+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quoting_function = 0;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quoting_function=yes
+else
+ pgac_cv_var_rl_filename_quoting_function=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quoting_function" >&5
+$as_echo "$pgac_cv_var_rl_filename_quoting_function" >&6; }
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTING_FUNCTION 1" >>confdefs.h
+
+fi
+
for ac_func in rl_completion_matches rl_filename_completion_function rl_reset_screen_size
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
diff --git a/configure.in b/configure.in
index 34aea0b..28ca423 100644
--- a/configure.in
+++ b/configure.in
@@ -1878,7 +1878,7 @@ fi
LIBS="$LIBS_including_readline"
if test "$with_readline" = yes; then
- PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+ PGAC_READLINE_VARIABLES
AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function rl_reset_screen_size])
AC_CHECK_FUNCS([append_history history_truncate_file])
fi
diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index 8c8b4c2..23a6f9f 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
* entails_quote - any of these present? need outer quotes
* quote - doubled within string, affixed to both ends
* escape - doubled within string
+ * force_quote - if true, quote the output even if it doesn't "need" it
* encoding - the active character-set encoding
*
* Do not use this as a substitute for PQescapeStringConn(). Use it for
@@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, int encoding)
*/
char *
quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding)
+ char quote, char escape, bool force_quote,
+ int encoding)
{
const char *src;
char *ret;
char *dst;
- bool need_quotes = false;
+ bool need_quotes = force_quote;
Assert(source != NULL);
Assert(quote != '\0');
diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h
index 16a7af0..fa00141 100644
--- a/src/bin/psql/stringutils.h
+++ b/src/bin/psql/stringutils.h
@@ -22,6 +22,7 @@ extern char *strtokx(const char *s,
extern void strip_quotes(char *source, char quote, char escape, int encoding);
extern char *quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding);
+ char quote, char escape, bool force_quote,
+ int encoding);
#endif /* STRINGUTILS_H */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5e0db35..dc867bd 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -41,6 +41,7 @@
#ifdef USE_READLINE
#include <ctype.h>
+#include <sys/stat.h>
#include "catalog/pg_am_d.h"
#include "catalog/pg_class_d.h"
@@ -63,6 +64,15 @@
#define rl_completion_matches completion_matches
#endif
+/*
+ * Currently we assume that rl_filename_dequoting_function exists if
+ * rl_filename_quoting_function does. If that proves not to be the case,
+ * we'd need to test for the former, or possibly both, in configure.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTING_FUNCTION
+#define USE_FILENAME_QUOTING_FUNCTIONS 1
+#endif
+
/* word break characters */
#define WORD_BREAKS "\t\n@$><=;|&{() "
@@ -1114,9 +1124,9 @@ static char **get_previous_words(int point, char **buffer, int *nwords);
static char *get_guctype(const char *varname);
-#ifdef NOT_USED
-static char *quote_file_name(char *text, int match_type, char *quote_pointer);
-static char *dequote_file_name(char *text, char quote_char);
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+static char *quote_file_name(char *fname, int match_type, char *quote_pointer);
+static char *dequote_file_name(char *fname, int quote_char);
#endif
@@ -1129,8 +1139,32 @@ initialize_readline(void)
rl_readline_name = (char *) pset.progname;
rl_attempted_completion_function = psql_completion;
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ rl_filename_quoting_function = quote_file_name;
+ rl_filename_dequoting_function = dequote_file_name;
+#endif
+
rl_basic_word_break_characters = WORD_BREAKS;
+ rl_completer_quote_characters = "'\"";
+
+ /*
+ * Set rl_filename_quote_characters to "all possible characters",
+ * otherwise Readline will skip filename quoting if it thinks a filename
+ * doesn't need quoting. Readline actually interprets this as bytes, so
+ * there are no encoding considerations here.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+ {
+ unsigned char *fqc = (unsigned char *) pg_malloc(256);
+
+ for (int i = 0; i < 255; i++)
+ fqc[i] = (unsigned char) (i + 1);
+ fqc[255] = '\0';
+ rl_filename_quote_characters = (const char *) fqc;
+ }
+#endif
+
completion_max_records = 1000;
/*
@@ -4378,15 +4412,49 @@ complete_from_variables(const char *text, const char *prefix, const char *suffix
/*
* 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.
+ * the input before searching for matches and then re-quote the matches.
+ * (We always quote filenames, even though for some psql meta-commands and
+ * some filenames it wouldn't be strictly necessary.)
+ *
+ * Caller must set completion_charp to a zero- or one-character string
+ * containing the escape character. This is necessary since \copy has no
+ * escape character, but every other backslash command recognizes "\" as an
+ * escape character. Since we have only two callers, don't bother providing
+ * a macro to simplify this.
*/
static char *
complete_from_files(const char *text, int state)
{
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ /*
+ * If we're using a version of Readline that supports filename quoting
+ * hooks, rely on those, and invoke rl_filename_completion_function()
+ * without messing with its arguments. Readline does stuff internally
+ * that does not work well at all if we try to handle dequoting here.
+ * Instead, Readline will call quote_file_name() and dequote_file_name()
+ * (see below) at appropriate times.
+ *
+ * ... or at least, mostly it will. There are some paths involving
+ * unmatched file names in which Readline never calls quote_file_name(),
+ * and if left to its own devices it will incorrectly append a quote
+ * anyway. Set rl_completion_suppress_quote to prevent that. If we do
+ * get to quote_file_name(), we'll clear this again. (Yes, this seems
+ * like it's working around Readline bugs.)
+ *
+ * (For now, we assume that rl_completion_suppress_quote exists if the
+ * filename quoting hooks do.)
+ */
+ rl_completion_suppress_quote = 1;
+
+ return rl_filename_completion_function(text, state);
+#else
+ /*
+ * Otherwise, we have to do the best we can.
+ */
static const char *unquoted_text;
char *unquoted_match;
char *ret = NULL;
+ struct stat statbuf;
if (state == 0)
{
@@ -4404,22 +4472,36 @@ complete_from_files(const char *text, int state)
unquoted_match = rl_filename_completion_function(unquoted_text, state);
if (unquoted_match)
{
+ /* Re-quote the result. */
+ ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
+ Assert(ret);
+
/*
- * Caller sets completion_charp to a zero- or one-character string
- * containing the escape character. This is necessary since \copy has
- * no escape character, but every other backslash command recognizes
- * "\" as an escape character. Since we have only two callers, don't
- * bother providing a macro to simplify this.
+ * If it's a directory, replace trailing quote with a slash; this is
+ * usually more convenient.
*/
- ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
- '\'', *completion_charp, pset.encoding);
- if (ret)
- free(unquoted_match);
- else
- ret = unquoted_match;
+ if (*ret &&
+ stat(unquoted_match, &statbuf) == 0 &&
+ S_ISDIR(statbuf.st_mode))
+ {
+ char *retend = ret + strlen(ret) - 1;
+
+ Assert(*retend == '\'');
+ *retend = '/';
+ /* Try to prevent libedit from adding a space, too */
+#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
+ rl_completion_append_character = '\0';
+#endif
+ }
+
+ free(unquoted_match);
}
return ret;
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
}
@@ -4671,46 +4753,104 @@ get_guctype(const char *varname)
return guctype;
}
-#ifdef NOT_USED
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
/*
- * Surround a string with single quotes. This works for both SQL and
- * psql internal. Currently disabled because it is reported not to
- * cooperate with certain versions of readline.
+ * Quote a filename according to SQL rules, returning a malloc'd string.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
*/
static char *
-quote_file_name(char *text, int match_type, char *quote_pointer)
+quote_file_name(char *fname, int match_type, char *quote_pointer)
{
char *s;
- size_t length;
+ struct stat statbuf;
- (void) quote_pointer; /* not used */
+ /* We always quote (so quote_if_needed's API is overkill) */
+ s = quote_if_needed(fname, " \t\r\n\"`",
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
+
+ /*
+ * However, some of the time we have to strip the trailing quote from what
+ * we send back. The tests on "s" are just paranoia given that we forced
+ * quoting. The meat of this is that we suppress the trailing quote if we
+ * have multiple/no matches (because we don't want to add a quote if the
+ * input is seemingly unfinished), or if the input was already quoted
+ * (because Readline will do arguably-buggy things otherwise), or if the
+ * file does not exist, or if it's a directory.
+ */
+ if (s && *s &&
+ (match_type != SINGLE_MATCH ||
+ (quote_pointer && *quote_pointer == '\'') ||
+ stat(fname, &statbuf) != 0 ||
+ S_ISDIR(statbuf.st_mode)))
+ {
+ char *send = s + strlen(s) - 1;
+
+ Assert(*send == '\'');
+ *send = '\0';
+ }
+
+ /*
+ * And now we can let Readline do its thing with possibly adding a quote
+ * on its own accord. (This covers some additional cases beyond those
+ * dealt with above.)
+ */
+ rl_completion_suppress_quote = 0;
+
+ /*
+ * If user typed a leading quote character other than single quote (i.e.,
+ * double quote), zap it, so that we replace it with the correct single
+ * quote.
+ */
+ if (quote_pointer && *quote_pointer != '\'')
+ *quote_pointer = '\0';
- length = strlen(text) +(match_type == SINGLE_MATCH ? 3 : 2);
- s = pg_malloc(length);
- s[0] = '\'';
- strcpy(s + 1, text);
- if (match_type == SINGLE_MATCH)
- s[length - 2] = '\'';
- s[length - 1] = '\0';
return s;
}
+/*
+ * Dequote a filename, if it's quoted.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
+ */
static char *
-dequote_file_name(char *text, char quote_char)
+dequote_file_name(char *fname, int quote_char)
{
- char *s;
- size_t length;
+ char *unquoted_fname;
+
+ /*
+ * If quote_char is set, it's not included in "fname". We have to add it
+ * or strtokx will not interpret the string correctly (notably, it won't
+ * recognize escapes).
+ */
+ if (quote_char == '\'')
+ {
+ char *workspace = (char *) pg_malloc(strlen(fname) + 2);
- if (!quote_char)
- return pg_strdup(text);
+ workspace[0] = quote_char;
+ strcpy(workspace + 1, fname);
+ unquoted_fname = strtokx(workspace, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
+ free(workspace);
+ }
+ else
+ unquoted_fname = strtokx(fname, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
- length = strlen(text);
- s = pg_malloc(length - 2 + 1);
- strlcpy(s, text +1, length - 2 + 1);
+ /* expect a NULL return for the empty string only */
+ if (!unquoted_fname)
+ {
+ Assert(*fname == '\0');
+ unquoted_fname = fname;
+ }
- return s;
+ /* readline expects a malloc'd result that it is to free */
+ return pg_strdup(unquoted_fname);
}
-#endif /* NOT_USED */
+
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
#endif /* USE_READLINE */
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 050c48b..983d94e 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -488,6 +488,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
#undef HAVE_RL_FILENAME_COMPLETION_FUNCTION
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+#undef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+#undef HAVE_RL_FILENAME_QUOTING_FUNCTION
+
/* Define to 1 if you have the `rl_reset_screen_size' function. */
#undef HAVE_RL_RESET_SCREEN_SIZE
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 808f5ab..51ba760 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -349,6 +349,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
/* #undef HAVE_RL_FILENAME_COMPLETION_FUNCTION */
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+/* #undef HAVE_RL_FILENAME_QUOTE_CHARACTERS */
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+/* #undef HAVE_RL_FILENAME_QUOTING_FUNCTION */
+
/* Define to 1 if you have the <security/pam_appl.h> header file. */
/* #undef HAVE_SECURITY_PAM_APPL_H */
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
I wrote:
> [ psql-filename-completion-fixes-2.patch ]
The cfbot noted this was broken by the removal of pg_config.h.win32,
so here's a new version rebased over that. No changes other than
adjusting the MSVC autoconf-substitute code.
regards, tom lane
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---------------------------------------
+# PGAC_READLINE_VARIABLES
+# -----------------------
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 9de5037..994e736 100755
--- a/configure
+++ b/configure
@@ -16314,10 +16314,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
int
@@ -16343,6 +16345,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; }
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting_function" >&5
+$as_echo_n "checking for rl_filename_quoting_function... " >&6; }
+if ${pgac_cv_var_rl_filename_quoting_function+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quoting_function = 0;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quoting_function=yes
+else
+ pgac_cv_var_rl_filename_quoting_function=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quoting_function" >&5
+$as_echo "$pgac_cv_var_rl_filename_quoting_function" >&6; }
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTING_FUNCTION 1" >>confdefs.h
+
+fi
+
for ac_func in rl_completion_matches rl_filename_completion_function rl_reset_screen_size
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
diff --git a/configure.in b/configure.in
index 9c5e5e7..0921262 100644
--- a/configure.in
+++ b/configure.in
@@ -1873,7 +1873,7 @@ fi
LIBS="$LIBS_including_readline"
if test "$with_readline" = yes; then
- PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+ PGAC_READLINE_VARIABLES
AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function rl_reset_screen_size])
AC_CHECK_FUNCS([append_history history_truncate_file])
fi
diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index 8c8b4c2..23a6f9f 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
* entails_quote - any of these present? need outer quotes
* quote - doubled within string, affixed to both ends
* escape - doubled within string
+ * force_quote - if true, quote the output even if it doesn't "need" it
* encoding - the active character-set encoding
*
* Do not use this as a substitute for PQescapeStringConn(). Use it for
@@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, int encoding)
*/
char *
quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding)
+ char quote, char escape, bool force_quote,
+ int encoding)
{
const char *src;
char *ret;
char *dst;
- bool need_quotes = false;
+ bool need_quotes = force_quote;
Assert(source != NULL);
Assert(quote != '\0');
diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h
index 16a7af0..fa00141 100644
--- a/src/bin/psql/stringutils.h
+++ b/src/bin/psql/stringutils.h
@@ -22,6 +22,7 @@ extern char *strtokx(const char *s,
extern void strip_quotes(char *source, char quote, char escape, int encoding);
extern char *quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding);
+ char quote, char escape, bool force_quote,
+ int encoding);
#endif /* STRINGUTILS_H */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5e0db35..dc867bd 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -41,6 +41,7 @@
#ifdef USE_READLINE
#include <ctype.h>
+#include <sys/stat.h>
#include "catalog/pg_am_d.h"
#include "catalog/pg_class_d.h"
@@ -63,6 +64,15 @@
#define rl_completion_matches completion_matches
#endif
+/*
+ * Currently we assume that rl_filename_dequoting_function exists if
+ * rl_filename_quoting_function does. If that proves not to be the case,
+ * we'd need to test for the former, or possibly both, in configure.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTING_FUNCTION
+#define USE_FILENAME_QUOTING_FUNCTIONS 1
+#endif
+
/* word break characters */
#define WORD_BREAKS "\t\n@$><=;|&{() "
@@ -1114,9 +1124,9 @@ static char **get_previous_words(int point, char **buffer, int *nwords);
static char *get_guctype(const char *varname);
-#ifdef NOT_USED
-static char *quote_file_name(char *text, int match_type, char *quote_pointer);
-static char *dequote_file_name(char *text, char quote_char);
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+static char *quote_file_name(char *fname, int match_type, char *quote_pointer);
+static char *dequote_file_name(char *fname, int quote_char);
#endif
@@ -1129,8 +1139,32 @@ initialize_readline(void)
rl_readline_name = (char *) pset.progname;
rl_attempted_completion_function = psql_completion;
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ rl_filename_quoting_function = quote_file_name;
+ rl_filename_dequoting_function = dequote_file_name;
+#endif
+
rl_basic_word_break_characters = WORD_BREAKS;
+ rl_completer_quote_characters = "'\"";
+
+ /*
+ * Set rl_filename_quote_characters to "all possible characters",
+ * otherwise Readline will skip filename quoting if it thinks a filename
+ * doesn't need quoting. Readline actually interprets this as bytes, so
+ * there are no encoding considerations here.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+ {
+ unsigned char *fqc = (unsigned char *) pg_malloc(256);
+
+ for (int i = 0; i < 255; i++)
+ fqc[i] = (unsigned char) (i + 1);
+ fqc[255] = '\0';
+ rl_filename_quote_characters = (const char *) fqc;
+ }
+#endif
+
completion_max_records = 1000;
/*
@@ -4378,15 +4412,49 @@ complete_from_variables(const char *text, const char *prefix, const char *suffix
/*
* 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.
+ * the input before searching for matches and then re-quote the matches.
+ * (We always quote filenames, even though for some psql meta-commands and
+ * some filenames it wouldn't be strictly necessary.)
+ *
+ * Caller must set completion_charp to a zero- or one-character string
+ * containing the escape character. This is necessary since \copy has no
+ * escape character, but every other backslash command recognizes "\" as an
+ * escape character. Since we have only two callers, don't bother providing
+ * a macro to simplify this.
*/
static char *
complete_from_files(const char *text, int state)
{
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ /*
+ * If we're using a version of Readline that supports filename quoting
+ * hooks, rely on those, and invoke rl_filename_completion_function()
+ * without messing with its arguments. Readline does stuff internally
+ * that does not work well at all if we try to handle dequoting here.
+ * Instead, Readline will call quote_file_name() and dequote_file_name()
+ * (see below) at appropriate times.
+ *
+ * ... or at least, mostly it will. There are some paths involving
+ * unmatched file names in which Readline never calls quote_file_name(),
+ * and if left to its own devices it will incorrectly append a quote
+ * anyway. Set rl_completion_suppress_quote to prevent that. If we do
+ * get to quote_file_name(), we'll clear this again. (Yes, this seems
+ * like it's working around Readline bugs.)
+ *
+ * (For now, we assume that rl_completion_suppress_quote exists if the
+ * filename quoting hooks do.)
+ */
+ rl_completion_suppress_quote = 1;
+
+ return rl_filename_completion_function(text, state);
+#else
+ /*
+ * Otherwise, we have to do the best we can.
+ */
static const char *unquoted_text;
char *unquoted_match;
char *ret = NULL;
+ struct stat statbuf;
if (state == 0)
{
@@ -4404,22 +4472,36 @@ complete_from_files(const char *text, int state)
unquoted_match = rl_filename_completion_function(unquoted_text, state);
if (unquoted_match)
{
+ /* Re-quote the result. */
+ ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
+ Assert(ret);
+
/*
- * Caller sets completion_charp to a zero- or one-character string
- * containing the escape character. This is necessary since \copy has
- * no escape character, but every other backslash command recognizes
- * "\" as an escape character. Since we have only two callers, don't
- * bother providing a macro to simplify this.
+ * If it's a directory, replace trailing quote with a slash; this is
+ * usually more convenient.
*/
- ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
- '\'', *completion_charp, pset.encoding);
- if (ret)
- free(unquoted_match);
- else
- ret = unquoted_match;
+ if (*ret &&
+ stat(unquoted_match, &statbuf) == 0 &&
+ S_ISDIR(statbuf.st_mode))
+ {
+ char *retend = ret + strlen(ret) - 1;
+
+ Assert(*retend == '\'');
+ *retend = '/';
+ /* Try to prevent libedit from adding a space, too */
+#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
+ rl_completion_append_character = '\0';
+#endif
+ }
+
+ free(unquoted_match);
}
return ret;
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
}
@@ -4671,46 +4753,104 @@ get_guctype(const char *varname)
return guctype;
}
-#ifdef NOT_USED
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
/*
- * Surround a string with single quotes. This works for both SQL and
- * psql internal. Currently disabled because it is reported not to
- * cooperate with certain versions of readline.
+ * Quote a filename according to SQL rules, returning a malloc'd string.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
*/
static char *
-quote_file_name(char *text, int match_type, char *quote_pointer)
+quote_file_name(char *fname, int match_type, char *quote_pointer)
{
char *s;
- size_t length;
+ struct stat statbuf;
- (void) quote_pointer; /* not used */
+ /* We always quote (so quote_if_needed's API is overkill) */
+ s = quote_if_needed(fname, " \t\r\n\"`",
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
+
+ /*
+ * However, some of the time we have to strip the trailing quote from what
+ * we send back. The tests on "s" are just paranoia given that we forced
+ * quoting. The meat of this is that we suppress the trailing quote if we
+ * have multiple/no matches (because we don't want to add a quote if the
+ * input is seemingly unfinished), or if the input was already quoted
+ * (because Readline will do arguably-buggy things otherwise), or if the
+ * file does not exist, or if it's a directory.
+ */
+ if (s && *s &&
+ (match_type != SINGLE_MATCH ||
+ (quote_pointer && *quote_pointer == '\'') ||
+ stat(fname, &statbuf) != 0 ||
+ S_ISDIR(statbuf.st_mode)))
+ {
+ char *send = s + strlen(s) - 1;
+
+ Assert(*send == '\'');
+ *send = '\0';
+ }
+
+ /*
+ * And now we can let Readline do its thing with possibly adding a quote
+ * on its own accord. (This covers some additional cases beyond those
+ * dealt with above.)
+ */
+ rl_completion_suppress_quote = 0;
+
+ /*
+ * If user typed a leading quote character other than single quote (i.e.,
+ * double quote), zap it, so that we replace it with the correct single
+ * quote.
+ */
+ if (quote_pointer && *quote_pointer != '\'')
+ *quote_pointer = '\0';
- length = strlen(text) +(match_type == SINGLE_MATCH ? 3 : 2);
- s = pg_malloc(length);
- s[0] = '\'';
- strcpy(s + 1, text);
- if (match_type == SINGLE_MATCH)
- s[length - 2] = '\'';
- s[length - 1] = '\0';
return s;
}
+/*
+ * Dequote a filename, if it's quoted.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
+ */
static char *
-dequote_file_name(char *text, char quote_char)
+dequote_file_name(char *fname, int quote_char)
{
- char *s;
- size_t length;
+ char *unquoted_fname;
+
+ /*
+ * If quote_char is set, it's not included in "fname". We have to add it
+ * or strtokx will not interpret the string correctly (notably, it won't
+ * recognize escapes).
+ */
+ if (quote_char == '\'')
+ {
+ char *workspace = (char *) pg_malloc(strlen(fname) + 2);
- if (!quote_char)
- return pg_strdup(text);
+ workspace[0] = quote_char;
+ strcpy(workspace + 1, fname);
+ unquoted_fname = strtokx(workspace, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
+ free(workspace);
+ }
+ else
+ unquoted_fname = strtokx(fname, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
- length = strlen(text);
- s = pg_malloc(length - 2 + 1);
- strlcpy(s, text +1, length - 2 + 1);
+ /* expect a NULL return for the empty string only */
+ if (!unquoted_fname)
+ {
+ Assert(*fname == '\0');
+ unquoted_fname = fname;
+ }
- return s;
+ /* readline expects a malloc'd result that it is to free */
+ return pg_strdup(unquoted_fname);
}
-#endif /* NOT_USED */
+
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
#endif /* USE_READLINE */
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 050c48b..983d94e 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -488,6 +488,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
#undef HAVE_RL_FILENAME_COMPLETION_FUNCTION
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+#undef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+#undef HAVE_RL_FILENAME_QUOTING_FUNCTION
+
/* Define to 1 if you have the `rl_reset_screen_size' function. */
#undef HAVE_RL_RESET_SCREEN_SIZE
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 909bded..979964c 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -333,6 +333,8 @@ sub GenerateFiles
HAVE_RL_COMPLETION_APPEND_CHARACTER => undef,
HAVE_RL_COMPLETION_MATCHES => undef,
HAVE_RL_FILENAME_COMPLETION_FUNCTION => undef,
+ HAVE_RL_FILENAME_QUOTE_CHARACTERS => undef,
+ HAVE_RL_FILENAME_QUOTING_FUNCTION => undef,
HAVE_RL_RESET_SCREEN_SIZE => undef,
HAVE_SECURITY_PAM_APPL_H => undef,
HAVE_SETPROCTITLE => undef,
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Alvaro Herrera
Date:
On 2019-Dec-27, Tom Lane wrote: > I wrote: > > [ psql-filename-completion-fixes-2.patch ] > > The cfbot noted this was broken by the removal of pg_config.h.win32, > so here's a new version rebased over that. No changes other than > adjusting the MSVC autoconf-substitute code. Works well for me. One minor thing I noticed is that if I enter \copy t from '/tmp/t'<tab> and I have files /tmp/t and /tmp/tst then it removes the ending quote. I can live with that, since the tab there is pointless anyway. (The unpatched version removes *both* quotes. The quotes there are not mandatory, but the new version works better when there are files with whitespace in the name.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> One minor thing I noticed is that if I enter
> \copy t from '/tmp/t'<tab>
> and I have files /tmp/t and /tmp/tst then it removes the ending quote.
Yeah, that bothered me too. [ pokes at it for a bit... ] The
quote_file_name function isn't passed enough info to handle this
in a clean way, but we can make it work if we don't mind introducing
some more coupling with psql_completion, ie having the latter remember
whether the raw input word ended with a quote. As per v4.
> (The unpatched version removes *both* quotes. The quotes there are not
> mandatory, but the new version works better when there are files with
> whitespace in the name.)
Or when you're completing in a COPY rather than \copy --- then, removing
the quotes is broken whether there's whitespace or not.
regards, tom lane
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---------------------------------------
+# PGAC_READLINE_VARIABLES
+# -----------------------
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 9de5037..994e736 100755
--- a/configure
+++ b/configure
@@ -16314,10 +16314,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
int
@@ -16343,6 +16345,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; }
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting_function" >&5
+$as_echo_n "checking for rl_filename_quoting_function... " >&6; }
+if ${pgac_cv_var_rl_filename_quoting_function+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quoting_function = 0;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quoting_function=yes
+else
+ pgac_cv_var_rl_filename_quoting_function=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quoting_function" >&5
+$as_echo "$pgac_cv_var_rl_filename_quoting_function" >&6; }
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTING_FUNCTION 1" >>confdefs.h
+
+fi
+
for ac_func in rl_completion_matches rl_filename_completion_function rl_reset_screen_size
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
diff --git a/configure.in b/configure.in
index 9c5e5e7..0921262 100644
--- a/configure.in
+++ b/configure.in
@@ -1873,7 +1873,7 @@ fi
LIBS="$LIBS_including_readline"
if test "$with_readline" = yes; then
- PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+ PGAC_READLINE_VARIABLES
AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function rl_reset_screen_size])
AC_CHECK_FUNCS([append_history history_truncate_file])
fi
diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index 8c8b4c2..23a6f9f 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
* entails_quote - any of these present? need outer quotes
* quote - doubled within string, affixed to both ends
* escape - doubled within string
+ * force_quote - if true, quote the output even if it doesn't "need" it
* encoding - the active character-set encoding
*
* Do not use this as a substitute for PQescapeStringConn(). Use it for
@@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, int encoding)
*/
char *
quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding)
+ char quote, char escape, bool force_quote,
+ int encoding)
{
const char *src;
char *ret;
char *dst;
- bool need_quotes = false;
+ bool need_quotes = force_quote;
Assert(source != NULL);
Assert(quote != '\0');
diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h
index 16a7af0..fa00141 100644
--- a/src/bin/psql/stringutils.h
+++ b/src/bin/psql/stringutils.h
@@ -22,6 +22,7 @@ extern char *strtokx(const char *s,
extern void strip_quotes(char *source, char quote, char escape, int encoding);
extern char *quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding);
+ char quote, char escape, bool force_quote,
+ int encoding);
#endif /* STRINGUTILS_H */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5e0db35..3071f41 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -41,6 +41,7 @@
#ifdef USE_READLINE
#include <ctype.h>
+#include <sys/stat.h>
#include "catalog/pg_am_d.h"
#include "catalog/pg_class_d.h"
@@ -63,6 +64,15 @@
#define rl_completion_matches completion_matches
#endif
+/*
+ * Currently we assume that rl_filename_dequoting_function exists if
+ * rl_filename_quoting_function does. If that proves not to be the case,
+ * we'd need to test for the former, or possibly both, in configure.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTING_FUNCTION
+#define USE_FILENAME_QUOTING_FUNCTIONS 1
+#endif
+
/* word break characters */
#define WORD_BREAKS "\t\n@$><=;|&{() "
@@ -157,9 +167,11 @@ typedef struct SchemaQuery
static int completion_max_records;
/*
- * Communication variables set by COMPLETE_WITH_FOO macros and then used by
- * the completion callback functions. Ugly but there is no better way.
+ * Communication variables set by psql_completion (mostly in COMPLETE_WITH_FOO
+ * macros) and then used by the completion callback functions. Ugly but there
+ * is no better way.
*/
+static char completion_last_char; /* last char of input word */
static const char *completion_charp; /* to pass a string */
static const char *const *completion_charpp; /* to pass a list of strings */
static const char *completion_info_charp; /* to pass a second string */
@@ -1114,9 +1126,9 @@ static char **get_previous_words(int point, char **buffer, int *nwords);
static char *get_guctype(const char *varname);
-#ifdef NOT_USED
-static char *quote_file_name(char *text, int match_type, char *quote_pointer);
-static char *dequote_file_name(char *text, char quote_char);
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+static char *quote_file_name(char *fname, int match_type, char *quote_pointer);
+static char *dequote_file_name(char *fname, int quote_char);
#endif
@@ -1129,8 +1141,32 @@ initialize_readline(void)
rl_readline_name = (char *) pset.progname;
rl_attempted_completion_function = psql_completion;
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ rl_filename_quoting_function = quote_file_name;
+ rl_filename_dequoting_function = dequote_file_name;
+#endif
+
rl_basic_word_break_characters = WORD_BREAKS;
+ rl_completer_quote_characters = "'\"";
+
+ /*
+ * Set rl_filename_quote_characters to "all possible characters",
+ * otherwise Readline will skip filename quoting if it thinks a filename
+ * doesn't need quoting. Readline actually interprets this as bytes, so
+ * there are no encoding considerations here.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+ {
+ unsigned char *fqc = (unsigned char *) pg_malloc(256);
+
+ for (int i = 0; i < 255; i++)
+ fqc[i] = (unsigned char) (i + 1);
+ fqc[255] = '\0';
+ rl_filename_quote_characters = (const char *) fqc;
+ }
+#endif
+
completion_max_records = 1000;
/*
@@ -1447,8 +1483,10 @@ psql_completion(const char *text, int start, int end)
NULL
};
- (void) end; /* "end" is not used */
+ /* Remember last char of the given input word. */
+ completion_last_char = (end > start) ? text[end - start - 1] : '\0';
+ /* We usually want the append character to be a space. */
#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
rl_completion_append_character = ' ';
#endif
@@ -4378,15 +4416,49 @@ complete_from_variables(const char *text, const char *prefix, const char *suffix
/*
* 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.
+ * the input before searching for matches and then re-quote the matches.
+ * (We always quote filenames, even though for some psql meta-commands and
+ * some filenames it wouldn't be strictly necessary.)
+ *
+ * Caller must set completion_charp to a zero- or one-character string
+ * containing the escape character. This is necessary since \copy has no
+ * escape character, but every other backslash command recognizes "\" as an
+ * escape character. Since we have only two callers, don't bother providing
+ * a macro to simplify this.
*/
static char *
complete_from_files(const char *text, int state)
{
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ /*
+ * If we're using a version of Readline that supports filename quoting
+ * hooks, rely on those, and invoke rl_filename_completion_function()
+ * without messing with its arguments. Readline does stuff internally
+ * that does not work well at all if we try to handle dequoting here.
+ * Instead, Readline will call quote_file_name() and dequote_file_name()
+ * (see below) at appropriate times.
+ *
+ * ... or at least, mostly it will. There are some paths involving
+ * unmatched file names in which Readline never calls quote_file_name(),
+ * and if left to its own devices it will incorrectly append a quote
+ * anyway. Set rl_completion_suppress_quote to prevent that. If we do
+ * get to quote_file_name(), we'll clear this again. (Yes, this seems
+ * like it's working around Readline bugs.)
+ *
+ * (For now, we assume that rl_completion_suppress_quote exists if the
+ * filename quoting hooks do.)
+ */
+ rl_completion_suppress_quote = 1;
+
+ return rl_filename_completion_function(text, state);
+#else
+ /*
+ * Otherwise, we have to do the best we can.
+ */
static const char *unquoted_text;
char *unquoted_match;
char *ret = NULL;
+ struct stat statbuf;
if (state == 0)
{
@@ -4404,22 +4476,36 @@ complete_from_files(const char *text, int state)
unquoted_match = rl_filename_completion_function(unquoted_text, state);
if (unquoted_match)
{
+ /* Re-quote the result. */
+ ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
+ Assert(ret);
+
/*
- * Caller sets completion_charp to a zero- or one-character string
- * containing the escape character. This is necessary since \copy has
- * no escape character, but every other backslash command recognizes
- * "\" as an escape character. Since we have only two callers, don't
- * bother providing a macro to simplify this.
+ * If it's a directory, replace trailing quote with a slash; this is
+ * usually more convenient.
*/
- ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
- '\'', *completion_charp, pset.encoding);
- if (ret)
- free(unquoted_match);
- else
- ret = unquoted_match;
+ if (*ret &&
+ stat(unquoted_match, &statbuf) == 0 &&
+ S_ISDIR(statbuf.st_mode))
+ {
+ char *retend = ret + strlen(ret) - 1;
+
+ Assert(*retend == '\'');
+ *retend = '/';
+ /* Try to prevent libedit from adding a space, too */
+#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
+ rl_completion_append_character = '\0';
+#endif
+ }
+
+ free(unquoted_match);
}
return ret;
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
}
@@ -4671,46 +4757,106 @@ get_guctype(const char *varname)
return guctype;
}
-#ifdef NOT_USED
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
/*
- * Surround a string with single quotes. This works for both SQL and
- * psql internal. Currently disabled because it is reported not to
- * cooperate with certain versions of readline.
+ * Quote a filename according to SQL rules, returning a malloc'd string.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
*/
static char *
-quote_file_name(char *text, int match_type, char *quote_pointer)
+quote_file_name(char *fname, int match_type, char *quote_pointer)
{
char *s;
- size_t length;
+ struct stat statbuf;
- (void) quote_pointer; /* not used */
+ /* We always quote (so quote_if_needed's API is overkill) */
+ s = quote_if_needed(fname, " \t\r\n\"`",
+ '\'', *completion_charp,
+ true,
+ pset.encoding);
+
+ /*
+ * However, some of the time we have to strip the trailing quote from what
+ * we send back. The tests on "s" are just paranoia given that we forced
+ * quoting. Never strip the trailing quote if the user already typed one;
+ * otherwise, suppress the trailing quote if we have multiple/no matches
+ * (because we don't want to add a quote if the input is seemingly
+ * unfinished), or if the input was already quoted (because Readline will
+ * do arguably-buggy things otherwise), or if the file does not exist, or
+ * if it's a directory.
+ */
+ if (s && *s &&
+ completion_last_char != '\'' &&
+ (match_type != SINGLE_MATCH ||
+ (quote_pointer && *quote_pointer == '\'') ||
+ stat(fname, &statbuf) != 0 ||
+ S_ISDIR(statbuf.st_mode)))
+ {
+ char *send = s + strlen(s) - 1;
+
+ Assert(*send == '\'');
+ *send = '\0';
+ }
+
+ /*
+ * And now we can let Readline do its thing with possibly adding a quote
+ * on its own accord. (This covers some additional cases beyond those
+ * dealt with above.)
+ */
+ rl_completion_suppress_quote = 0;
+
+ /*
+ * If user typed a leading quote character other than single quote (i.e.,
+ * double quote), zap it, so that we replace it with the correct single
+ * quote.
+ */
+ if (quote_pointer && *quote_pointer != '\'')
+ *quote_pointer = '\0';
- length = strlen(text) +(match_type == SINGLE_MATCH ? 3 : 2);
- s = pg_malloc(length);
- s[0] = '\'';
- strcpy(s + 1, text);
- if (match_type == SINGLE_MATCH)
- s[length - 2] = '\'';
- s[length - 1] = '\0';
return s;
}
+/*
+ * Dequote a filename, if it's quoted.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
+ */
static char *
-dequote_file_name(char *text, char quote_char)
+dequote_file_name(char *fname, int quote_char)
{
- char *s;
- size_t length;
+ char *unquoted_fname;
+
+ /*
+ * If quote_char is set, it's not included in "fname". We have to add it
+ * or strtokx will not interpret the string correctly (notably, it won't
+ * recognize escapes).
+ */
+ if (quote_char == '\'')
+ {
+ char *workspace = (char *) pg_malloc(strlen(fname) + 2);
- if (!quote_char)
- return pg_strdup(text);
+ workspace[0] = quote_char;
+ strcpy(workspace + 1, fname);
+ unquoted_fname = strtokx(workspace, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
+ free(workspace);
+ }
+ else
+ unquoted_fname = strtokx(fname, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
- length = strlen(text);
- s = pg_malloc(length - 2 + 1);
- strlcpy(s, text +1, length - 2 + 1);
+ /* expect a NULL return for the empty string only */
+ if (!unquoted_fname)
+ {
+ Assert(*fname == '\0');
+ unquoted_fname = fname;
+ }
- return s;
+ /* readline expects a malloc'd result that it is to free */
+ return pg_strdup(unquoted_fname);
}
-#endif /* NOT_USED */
+
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
#endif /* USE_READLINE */
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 050c48b..983d94e 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -488,6 +488,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
#undef HAVE_RL_FILENAME_COMPLETION_FUNCTION
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+#undef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+#undef HAVE_RL_FILENAME_QUOTING_FUNCTION
+
/* Define to 1 if you have the `rl_reset_screen_size' function. */
#undef HAVE_RL_RESET_SCREEN_SIZE
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 909bded..979964c 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -333,6 +333,8 @@ sub GenerateFiles
HAVE_RL_COMPLETION_APPEND_CHARACTER => undef,
HAVE_RL_COMPLETION_MATCHES => undef,
HAVE_RL_FILENAME_COMPLETION_FUNCTION => undef,
+ HAVE_RL_FILENAME_QUOTE_CHARACTERS => undef,
+ HAVE_RL_FILENAME_QUOTING_FUNCTION => undef,
HAVE_RL_RESET_SCREEN_SIZE => undef,
HAVE_SECURITY_PAM_APPL_H => undef,
HAVE_SETPROCTITLE => undef,
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Peter Eisentraut
Date:
On 2019-11-03 23:40, Tom Lane wrote: > * The patch now always quotes completed filenames, so quote_if_needed() > is misnamed and overcomplicated for this use-case. I left the extra > generality in place for possible future use. On the other hand, this > is the*only* use-case, so you could also argue that we should just > simplify the function's API. I have no strong opinion about that. I haven't found an explanation in this thread why it does always quote now. That seems a bit unusual. Is there a reason for this? Can we do without it? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-11-03 23:40, Tom Lane wrote:
>> * The patch now always quotes completed filenames, so quote_if_needed()
>> is misnamed and overcomplicated for this use-case. I left the extra
>> generality in place for possible future use. On the other hand, this
>> is the*only* use-case, so you could also argue that we should just
>> simplify the function's API. I have no strong opinion about that.
> I haven't found an explanation in this thread why it does always quote
> now. That seems a bit unusual. Is there a reason for this? Can we do
> without it?
The core problem we're trying to solve is stated in the thread title:
if you do
prompt# copy mytab from 'myfil<TAB>
then (assuming some completion is available) the current code actually
*removes* the quote, which is completely wrong. Even if the user
didn't type a leading quote, it's better to add one, because COPY
won't work otherwise.
It'd be possible, perhaps, to distinguish between this case and the
cases in backslash commands, which are okay with omitted quotes
(for some filenames). I'm not sure that that would be an improvement
though.
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
I wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> I haven't found an explanation in this thread why it does always quote
>> now. That seems a bit unusual. Is there a reason for this? Can we do
>> without it?
> The core problem we're trying to solve is stated in the thread title:
> ...
> It'd be possible, perhaps, to distinguish between this case and the
> cases in backslash commands, which are okay with omitted quotes
> (for some filenames). I'm not sure that that would be an improvement
> though.
I realized that there *is* a good reason to worry about this. Fairly
recent versions of libedit will try to backslash-escape the output
of psql_completion(), as I described over at [1]. This is absolutely
disastrous if we've emitted a quoted filename: we end up going from,
say,
\lo_import myf<TAB>
to
\lo_import \'myfile\'
which of course doesn't work at all (psql thinks \' is in itself a
backslash command).
There isn't much we can do about this in contexts where we must
quote the filename: not doing so produces an equally broken command.
However, this problem does suggest that we shouldn't force quoting
if we don't have to, as that just breaks cases we needn't break.
Hence, the attached revision only forces quoting in a SQL COPY
command, or if the user already typed a quote.
I also added some regression tests (whee!). They pass for me with a
couple different readline and libedit versions, but I have only minimal
hopes for them passing everywhere without further hacking ... the
results of the other thread suggest that pain is to be expected here.
regards, tom lane
[1] https://www.postgresql.org/message-id/13708.1578059577%40sss.pgh.pa.us
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---------------------------------------
+# PGAC_READLINE_VARIABLES
+# -----------------------
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index d2d63f2..40fe9c1 100755
--- a/configure
+++ b/configure
@@ -16316,10 +16316,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
int
@@ -16345,6 +16347,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; }
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting_function" >&5
+$as_echo_n "checking for rl_filename_quoting_function... " >&6; }
+if ${pgac_cv_var_rl_filename_quoting_function+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quoting_function = 0;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quoting_function=yes
+else
+ pgac_cv_var_rl_filename_quoting_function=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quoting_function" >&5
+$as_echo "$pgac_cv_var_rl_filename_quoting_function" >&6; }
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTING_FUNCTION 1" >>confdefs.h
+
+fi
+
for ac_func in rl_completion_matches rl_filename_completion_function rl_reset_screen_size
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
diff --git a/configure.in b/configure.in
index 1599fc5..8165f70 100644
--- a/configure.in
+++ b/configure.in
@@ -1874,7 +1874,7 @@ fi
LIBS="$LIBS_including_readline"
if test "$with_readline" = yes; then
- PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+ PGAC_READLINE_VARIABLES
AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function rl_reset_screen_size])
AC_CHECK_FUNCS([append_history history_truncate_file])
fi
diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index c74e58d..c521749 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
* entails_quote - any of these present? need outer quotes
* quote - doubled within string, affixed to both ends
* escape - doubled within string
+ * force_quote - if true, quote the output even if it doesn't "need" it
* encoding - the active character-set encoding
*
* Do not use this as a substitute for PQescapeStringConn(). Use it for
@@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, int encoding)
*/
char *
quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding)
+ char quote, char escape, bool force_quote,
+ int encoding)
{
const char *src;
char *ret;
char *dst;
- bool need_quotes = false;
+ bool need_quotes = force_quote;
Assert(source != NULL);
Assert(quote != '\0');
diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h
index 1fb1cfc..4be172e 100644
--- a/src/bin/psql/stringutils.h
+++ b/src/bin/psql/stringutils.h
@@ -22,6 +22,7 @@ extern char *strtokx(const char *s,
extern void strip_quotes(char *source, char quote, char escape, int encoding);
extern char *quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding);
+ char quote, char escape, bool force_quote,
+ int encoding);
#endif /* STRINGUTILS_H */
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index ef6c28f..49f6d95 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -50,6 +50,30 @@ delete $ENV{TERM};
# Some versions of readline inspect LS_COLORS, so for luck unset that too.
delete $ENV{LS_COLORS};
+# In a VPATH build, we'll be started in the source directory, but we want
+# to run in the build directory so that we can use relative paths to
+# access the tmp_check subdirectory; otherwise the output from filename
+# completion tests is too variable.
+if ($ENV{TESTDIR})
+{
+ chdir $ENV{TESTDIR} or die "could not chdir to \"$ENV{TESTDIR}\": $!";
+}
+
+# Create some junk files for filename completion testing.
+my $FH;
+open $FH, ">", "tmp_check/somefile"
+ or die("could not create file \"tmp_check/somefile\": $!");
+print $FH "some stuff\n";
+close $FH;
+open $FH, ">", "tmp_check/afile123"
+ or die("could not create file \"tmp_check/afile123\": $!");
+print $FH "more stuff\n";
+close $FH;
+open $FH, ">", "tmp_check/afile456"
+ or die("could not create file \"tmp_check/afile456\": $!");
+print $FH "other stuff\n";
+close $FH;
+
# fire up an interactive psql session
my $in = '';
my $out = '';
@@ -94,6 +118,15 @@ sub clear_query
return;
}
+# Clear current line to start over
+# (this will work in an incomplete string literal, but it's less desirable
+# than clear_query because we lose evidence in the history file)
+sub clear_line
+{
+ check_completion("\025\n", qr/postgres=# /, "control-U works");
+ return;
+}
+
# check basic command completion: SEL<tab> produces SELECT<space>
check_completion("SEL\t", qr/SELECT /, "complete SEL<tab> to SELECT");
@@ -132,6 +165,47 @@ check_completion("\\DRD\t", qr/drds /, "complete \\DRD<tab> to \\drds");
clear_query();
+# check filename completion
+check_completion(
+ "\\lo_import tmp_check/some\t",
+ qr|tmp_check/somefile |,
+ "filename completion with one possibility");
+
+clear_query();
+
+# note: readline might print a bell before the completion
+check_completion(
+ "\\lo_import tmp_check/af\t",
+ qr|tmp_check/af\a?ile|,
+ "filename completion with multiple possibilities");
+
+clear_query();
+
+# COPY requires quoting
+# note: broken versions of libedit want to backslash the closing quote;
+# not much we can do about that
+check_completion(
+ "COPY foo FROM tmp_check/some\t",
+ qr|'tmp_check/somefile\\?' |,
+ "quoted filename completion with one possibility");
+
+clear_line();
+
+check_completion(
+ "COPY foo FROM tmp_check/af\t",
+ qr|'tmp_check/afile|,
+ "quoted filename completion with multiple possibilities");
+
+# some versions of readline/libedit require two tabs here, some only need one
+# also, some will offer the whole path name and some just the file name
+# the quotes might appear, too
+check_completion(
+ "\t\t",
+ qr|afile123'? +'?(tmp_check/)?afile456|,
+ "offer multiple file choices");
+
+clear_line();
+
# send psql an explicit \q to shut it down, else pty won't close properly
$timer->start(5);
$in .= "\\q\n";
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 2fd8886..4c80b9a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -41,6 +41,7 @@
#ifdef USE_READLINE
#include <ctype.h>
+#include <sys/stat.h>
#include "catalog/pg_am_d.h"
#include "catalog/pg_class_d.h"
@@ -63,6 +64,15 @@
#define rl_completion_matches completion_matches
#endif
+/*
+ * Currently we assume that rl_filename_dequoting_function exists if
+ * rl_filename_quoting_function does. If that proves not to be the case,
+ * we'd need to test for the former, or possibly both, in configure.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTING_FUNCTION
+#define USE_FILENAME_QUOTING_FUNCTIONS 1
+#endif
+
/* word break characters */
#define WORD_BREAKS "\t\n@$><=;|&{() "
@@ -157,9 +167,11 @@ typedef struct SchemaQuery
static int completion_max_records;
/*
- * Communication variables set by COMPLETE_WITH_FOO macros and then used by
- * the completion callback functions. Ugly but there is no better way.
+ * Communication variables set by psql_completion (mostly in COMPLETE_WITH_FOO
+ * macros) and then used by the completion callback functions. Ugly but there
+ * is no better way.
*/
+static char completion_last_char; /* last char of input word */
static const char *completion_charp; /* to pass a string */
static const char *const *completion_charpp; /* to pass a list of strings */
static const char *completion_info_charp; /* to pass a second string */
@@ -167,6 +179,7 @@ static const char *completion_info_charp2; /* to pass a third string */
static const VersionedQuery *completion_vquery; /* to pass a VersionedQuery */
static const SchemaQuery *completion_squery; /* to pass a SchemaQuery */
static bool completion_case_sensitive; /* completion is case sensitive */
+static bool completion_force_quote; /* true to force-quote filenames */
/*
* A few macros to ease typing. You can use these to complete the given
@@ -1114,9 +1127,9 @@ static char **get_previous_words(int point, char **buffer, int *nwords);
static char *get_guctype(const char *varname);
-#ifdef NOT_USED
-static char *quote_file_name(char *text, int match_type, char *quote_pointer);
-static char *dequote_file_name(char *text, char quote_char);
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+static char *quote_file_name(char *fname, int match_type, char *quote_pointer);
+static char *dequote_file_name(char *fname, int quote_char);
#endif
@@ -1129,8 +1142,32 @@ initialize_readline(void)
rl_readline_name = (char *) pset.progname;
rl_attempted_completion_function = psql_completion;
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ rl_filename_quoting_function = quote_file_name;
+ rl_filename_dequoting_function = dequote_file_name;
+#endif
+
rl_basic_word_break_characters = WORD_BREAKS;
+ rl_completer_quote_characters = "'\"";
+
+ /*
+ * Set rl_filename_quote_characters to "all possible characters",
+ * otherwise Readline will skip filename quoting if it thinks a filename
+ * doesn't need quoting. Readline actually interprets this as bytes, so
+ * there are no encoding considerations here.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+ {
+ unsigned char *fqc = (unsigned char *) pg_malloc(256);
+
+ for (int i = 0; i < 255; i++)
+ fqc[i] = (unsigned char) (i + 1);
+ fqc[255] = '\0';
+ rl_filename_quote_characters = (const char *) fqc;
+ }
+#endif
+
completion_max_records = 1000;
/*
@@ -1455,6 +1492,10 @@ psql_completion(const char *text, int start, int end)
char *text_copy = pnstrdup(rl_line_buffer + start, end - start);
text = text_copy;
+ /* Remember last char of the given input word. */
+ completion_last_char = (end > start) ? text[end - start - 1] : '\0';
+
+ /* We usually want the append character to be a space. */
#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
rl_completion_append_character = ' ';
#endif
@@ -2265,13 +2306,19 @@ psql_completion(const char *text, int start, int end)
Matches("COPY", "BINARY", MatchAny))
COMPLETE_WITH("FROM", "TO");
/* If we have COPY [BINARY] <sth> FROM|TO, complete with filename */
- else if (Matches("COPY|\\copy", MatchAny, "FROM|TO") ||
+ else if (Matches("COPY", MatchAny, "FROM|TO") ||
Matches("COPY", "BINARY", MatchAny, "FROM|TO"))
{
completion_charp = "";
+ completion_force_quote = true; /* COPY requires quoted filename */
+ matches = rl_completion_matches(text, complete_from_files);
+ }
+ else if (Matches("\\copy", MatchAny, "FROM|TO"))
+ {
+ completion_charp = "";
+ completion_force_quote = false;
matches = rl_completion_matches(text, complete_from_files);
}
-
/* Handle COPY [BINARY] <sth> FROM|TO filename */
else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny) ||
Matches("COPY", "BINARY", MatchAny, "FROM|TO", MatchAny))
@@ -3821,6 +3868,7 @@ psql_completion(const char *text, int start, int end)
"\\s|\\w|\\write|\\lo_import"))
{
completion_charp = "\\";
+ completion_force_quote = false;
matches = rl_completion_matches(text, complete_from_files);
}
@@ -4387,14 +4435,58 @@ complete_from_variables(const char *text, const char *prefix, const char *suffix
* 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.
+ *
+ * Caller must set completion_charp to a zero- or one-character string
+ * containing the escape character. This is necessary since \copy has no
+ * escape character, but every other backslash command recognizes "\" as an
+ * escape character.
+ *
+ * Caller must also set completion_force_quote to indicate whether to force
+ * quotes around the result. (The SQL COPY command requires that.)
*/
static char *
complete_from_files(const char *text, int state)
{
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+
+ /*
+ * If we're using a version of Readline that supports filename quoting
+ * hooks, rely on those, and invoke rl_filename_completion_function()
+ * without messing with its arguments. Readline does stuff internally
+ * that does not work well at all if we try to handle dequoting here.
+ * Instead, Readline will call quote_file_name() and dequote_file_name()
+ * (see below) at appropriate times.
+ *
+ * ... or at least, mostly it will. There are some paths involving
+ * unmatched file names in which Readline never calls quote_file_name(),
+ * and if left to its own devices it will incorrectly append a quote
+ * anyway. Set rl_completion_suppress_quote to prevent that. If we do
+ * get to quote_file_name(), we'll clear this again. (Yes, this seems
+ * like it's working around Readline bugs.)
+ *
+ * (For now, we assume that rl_completion_suppress_quote exists if the
+ * filename quoting hooks do.)
+ */
+ rl_completion_suppress_quote = 1;
+
+ /* If user typed a quote, force quoting (never remove user's quote) */
+ if (*text == '\'')
+ completion_force_quote = true;
+
+ return rl_filename_completion_function(text, state);
+#else
+
+ /*
+ * Otherwise, we have to do the best we can.
+ */
static const char *unquoted_text;
char *unquoted_match;
char *ret = NULL;
+ /* If user typed a quote, force quoting (never remove user's quote) */
+ if (*text == '\'')
+ completion_force_quote = true;
+
if (state == 0)
{
/* Initialization: stash the unquoted input. */
@@ -4411,22 +4503,40 @@ complete_from_files(const char *text, int state)
unquoted_match = rl_filename_completion_function(unquoted_text, state);
if (unquoted_match)
{
- /*
- * Caller sets completion_charp to a zero- or one-character string
- * containing the escape character. This is necessary since \copy has
- * no escape character, but every other backslash command recognizes
- * "\" as an escape character. Since we have only two callers, don't
- * bother providing a macro to simplify this.
- */
+ struct stat statbuf;
+ bool is_dir = (stat(unquoted_match, &statbuf) == 0 &&
+ S_ISDIR(statbuf.st_mode) != 0);
+
+ /* Re-quote the result, if needed. */
ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
- '\'', *completion_charp, pset.encoding);
+ '\'', *completion_charp,
+ completion_force_quote,
+ pset.encoding);
if (ret)
free(unquoted_match);
else
ret = unquoted_match;
+
+ /*
+ * If it's a directory, replace trailing quote with a slash; this is
+ * usually more convenient. (If we didn't quote, leave this to
+ * libedit.)
+ */
+ if (*ret == '\'' && is_dir)
+ {
+ char *retend = ret + strlen(ret) - 1;
+
+ Assert(*retend == '\'');
+ *retend = '/';
+ /* Try to prevent libedit from adding a space, too */
+#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
+ rl_completion_append_character = '\0';
+#endif
+ }
}
return ret;
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
}
@@ -4678,46 +4788,108 @@ get_guctype(const char *varname)
return guctype;
}
-#ifdef NOT_USED
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
/*
- * Surround a string with single quotes. This works for both SQL and
- * psql internal. Currently disabled because it is reported not to
- * cooperate with certain versions of readline.
+ * Quote a filename according to SQL rules, returning a malloc'd string.
+ * completion_charp must point to escape character or '\0', and
+ * completion_force_quote must be set correctly, as per comments for
+ * complete_from_files().
*/
static char *
-quote_file_name(char *text, int match_type, char *quote_pointer)
+quote_file_name(char *fname, int match_type, char *quote_pointer)
{
char *s;
- size_t length;
+ struct stat statbuf;
+
+ /* Quote if needed. */
+ s = quote_if_needed(fname, " \t\r\n\"`",
+ '\'', *completion_charp,
+ completion_force_quote,
+ pset.encoding);
+ if (!s)
+ s = pg_strdup(fname);
- (void) quote_pointer; /* not used */
+ /*
+ * However, some of the time we have to strip the trailing quote from what
+ * we send back. Never strip the trailing quote if the user already typed
+ * one; otherwise, suppress the trailing quote if we have multiple/no
+ * matches (because we don't want to add a quote if the input is seemingly
+ * unfinished), or if the input was already quoted (because Readline will
+ * do arguably-buggy things otherwise), or if the file does not exist, or
+ * if it's a directory.
+ */
+ if (*s == '\'' &&
+ completion_last_char != '\'' &&
+ (match_type != SINGLE_MATCH ||
+ (quote_pointer && *quote_pointer == '\'') ||
+ stat(fname, &statbuf) != 0 ||
+ S_ISDIR(statbuf.st_mode)))
+ {
+ char *send = s + strlen(s) - 1;
+
+ Assert(*send == '\'');
+ *send = '\0';
+ }
+
+ /*
+ * And now we can let Readline do its thing with possibly adding a quote
+ * on its own accord. (This covers some additional cases beyond those
+ * dealt with above.)
+ */
+ rl_completion_suppress_quote = 0;
+
+ /*
+ * If user typed a leading quote character other than single quote (i.e.,
+ * double quote), zap it, so that we replace it with the correct single
+ * quote.
+ */
+ if (quote_pointer && *quote_pointer != '\'')
+ *quote_pointer = '\0';
- length = strlen(text) +(match_type == SINGLE_MATCH ? 3 : 2);
- s = pg_malloc(length);
- s[0] = '\'';
- strcpy(s + 1, text);
- if (match_type == SINGLE_MATCH)
- s[length - 2] = '\'';
- s[length - 1] = '\0';
return s;
}
+/*
+ * Dequote a filename, if it's quoted.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
+ */
static char *
-dequote_file_name(char *text, char quote_char)
+dequote_file_name(char *fname, int quote_char)
{
- char *s;
- size_t length;
+ char *unquoted_fname;
- if (!quote_char)
- return pg_strdup(text);
+ /*
+ * If quote_char is set, it's not included in "fname". We have to add it
+ * or strtokx will not interpret the string correctly (notably, it won't
+ * recognize escapes).
+ */
+ if (quote_char == '\'')
+ {
+ char *workspace = (char *) pg_malloc(strlen(fname) + 2);
- length = strlen(text);
- s = pg_malloc(length - 2 + 1);
- strlcpy(s, text +1, length - 2 + 1);
+ workspace[0] = quote_char;
+ strcpy(workspace + 1, fname);
+ unquoted_fname = strtokx(workspace, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
+ free(workspace);
+ }
+ else
+ unquoted_fname = strtokx(fname, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
- return s;
+ /* expect a NULL return for the empty string only */
+ if (!unquoted_fname)
+ {
+ Assert(*fname == '\0');
+ unquoted_fname = fname;
+ }
+
+ /* readline expects a malloc'd result that it is to free */
+ return pg_strdup(unquoted_fname);
}
-#endif /* NOT_USED */
+
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
#endif /* USE_READLINE */
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 050c48b..983d94e 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -488,6 +488,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
#undef HAVE_RL_FILENAME_COMPLETION_FUNCTION
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+#undef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+#undef HAVE_RL_FILENAME_QUOTING_FUNCTION
+
/* Define to 1 if you have the `rl_reset_screen_size' function. */
#undef HAVE_RL_RESET_SCREEN_SIZE
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 909bded..979964c 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -333,6 +333,8 @@ sub GenerateFiles
HAVE_RL_COMPLETION_APPEND_CHARACTER => undef,
HAVE_RL_COMPLETION_MATCHES => undef,
HAVE_RL_FILENAME_COMPLETION_FUNCTION => undef,
+ HAVE_RL_FILENAME_QUOTE_CHARACTERS => undef,
+ HAVE_RL_FILENAME_QUOTING_FUNCTION => undef,
HAVE_RL_RESET_SCREEN_SIZE => undef,
HAVE_SECURITY_PAM_APPL_H => undef,
HAVE_SETPROCTITLE => undef,
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Peter Eisentraut
Date:
On 2020-01-06 07:06, Tom Lane wrote: > Hence, the attached revision only forces quoting in a SQL COPY > command, or if the user already typed a quote. Yes, that seems better. Users tend to not like if tab completion messes with what they have already typed unless strictly necessary. The file name completion portion of this patch seems to work quite well now. I have found a weird behavior with identifier quoting, which is not the subject of this patch, but it appears to be affected by it. The good thing is that the new code will behave sensibly with select * from "pg_cl<TAB> which the old code didn't offer anything on. The problem is that if you have create table "test""1" (a int); then the new code responds to select * from "te<TAB> by making that select * from "te" whereas the old code curiously handled that perfectly. Neither the old nor the new code will produce anything from select * from te<TAB> -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> The file name completion portion of this patch seems to work quite well now.
Thanks for testing!
> I have found a weird behavior with identifier quoting, which is not the
> subject of this patch, but it appears to be affected by it.
> The good thing is that the new code will behave sensibly with
> select * from "pg_cl<TAB>
> which the old code didn't offer anything on.
Check.
> The problem is that if you have
> create table "test""1" (a int);
> then the new code responds to
> select * from "te<TAB>
> by making that
> select * from "te"
> whereas the old code curiously handled that perfectly.
Right. The underlying cause of both these changes seems to be that
what readline passes to psql_completion is just the contents of the
string, now that we've told it that double-quote is a string quoting
character. So that works fine with 'pg_cl' which didn't need quoting
anyway. In your second example, because we generate possible matches
that are already quoted-if-necessary, we fail to find anything that
bare 'te' is a prefix of, where before we were considering '"te' and
it worked.
I'll think about how to improve this. Given that we're dequoting
filenames explicitly anyway, maybe we don't need to tell readline that
double-quote is a quoting character. Another idea is that maybe
we ought to refactor things so that identifier dequoting and requoting
is handled explicitly, similarly to the way filenames work now.
That would make the patch a lot bigger though.
regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
I wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> I have found a weird behavior with identifier quoting, which is not the
>> subject of this patch, but it appears to be affected by it.
> I'll think about how to improve this. Given that we're dequoting
> filenames explicitly anyway, maybe we don't need to tell readline that
> double-quote is a quoting character. Another idea is that maybe
> we ought to refactor things so that identifier dequoting and requoting
> is handled explicitly, similarly to the way filenames work now.
> That would make the patch a lot bigger though.
On reflection, it seems like the best bet for the moment is to
remove double-quote from the rl_completer_quote_characters string,
which should restore all behavior around double-quoted strings to
what it was before. (We have to keep single-quote in that string,
though, or quoted file names fail entirely.)
The only impact this has on filename completion is that we're no
longer bright enough to convert a double-quoted filename to
single-quoted for you, which would be a nice-to-have but it's
hardly core functionality.
At some point it'd be good to undo that and upgrade quoted-identifier
processing, but I don't really want to include such changes in this
patch. I'm about burned out on tab completion for right now.
regards, tom lane
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---------------------------------------
+# PGAC_READLINE_VARIABLES
+# -----------------------
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 25cfbcb..a46ba40 100755
--- a/configure
+++ b/configure
@@ -16316,10 +16316,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include <stdio.h>
-#ifdef HAVE_READLINE_READLINE_H
-# include <readline/readline.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
#elif defined(HAVE_READLINE_H)
-# include <readline.h>
+#include <readline.h>
#endif
int
@@ -16345,6 +16347,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; }
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting_function" >&5
+$as_echo_n "checking for rl_filename_quoting_function... " >&6; }
+if ${pgac_cv_var_rl_filename_quoting_function+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <stdio.h>
+#if defined(HAVE_READLINE_READLINE_H)
+#include <readline/readline.h>
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include <editline/readline.h>
+#elif defined(HAVE_READLINE_H)
+#include <readline.h>
+#endif
+
+int
+main ()
+{
+rl_filename_quoting_function = 0;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quoting_function=yes
+else
+ pgac_cv_var_rl_filename_quoting_function=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quoting_function" >&5
+$as_echo "$pgac_cv_var_rl_filename_quoting_function" >&6; }
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTING_FUNCTION 1" >>confdefs.h
+
+fi
+
for ac_func in rl_completion_matches rl_filename_completion_function rl_reset_screen_size
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
diff --git a/configure.in b/configure.in
index 1599fc5..8165f70 100644
--- a/configure.in
+++ b/configure.in
@@ -1874,7 +1874,7 @@ fi
LIBS="$LIBS_including_readline"
if test "$with_readline" = yes; then
- PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+ PGAC_READLINE_VARIABLES
AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function rl_reset_screen_size])
AC_CHECK_FUNCS([append_history history_truncate_file])
fi
diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index c74e58d..c521749 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
* entails_quote - any of these present? need outer quotes
* quote - doubled within string, affixed to both ends
* escape - doubled within string
+ * force_quote - if true, quote the output even if it doesn't "need" it
* encoding - the active character-set encoding
*
* Do not use this as a substitute for PQescapeStringConn(). Use it for
@@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, int encoding)
*/
char *
quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding)
+ char quote, char escape, bool force_quote,
+ int encoding)
{
const char *src;
char *ret;
char *dst;
- bool need_quotes = false;
+ bool need_quotes = force_quote;
Assert(source != NULL);
Assert(quote != '\0');
diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h
index 1fb1cfc..4be172e 100644
--- a/src/bin/psql/stringutils.h
+++ b/src/bin/psql/stringutils.h
@@ -22,6 +22,7 @@ extern char *strtokx(const char *s,
extern void strip_quotes(char *source, char quote, char escape, int encoding);
extern char *quote_if_needed(const char *source, const char *entails_quote,
- char quote, char escape, int encoding);
+ char quote, char escape, bool force_quote,
+ int encoding);
#endif /* STRINGUTILS_H */
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index b35c4f8..c27f216 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -60,6 +60,30 @@ delete $ENV{TERM};
# Some versions of readline inspect LS_COLORS, so for luck unset that too.
delete $ENV{LS_COLORS};
+# In a VPATH build, we'll be started in the source directory, but we want
+# to run in the build directory so that we can use relative paths to
+# access the tmp_check subdirectory; otherwise the output from filename
+# completion tests is too variable.
+if ($ENV{TESTDIR})
+{
+ chdir $ENV{TESTDIR} or die "could not chdir to \"$ENV{TESTDIR}\": $!";
+}
+
+# Create some junk files for filename completion testing.
+my $FH;
+open $FH, ">", "tmp_check/somefile"
+ or die("could not create file \"tmp_check/somefile\": $!");
+print $FH "some stuff\n";
+close $FH;
+open $FH, ">", "tmp_check/afile123"
+ or die("could not create file \"tmp_check/afile123\": $!");
+print $FH "more stuff\n";
+close $FH;
+open $FH, ">", "tmp_check/afile456"
+ or die("could not create file \"tmp_check/afile456\": $!");
+print $FH "other stuff\n";
+close $FH;
+
# fire up an interactive psql session
my $in = '';
my $out = '';
@@ -104,6 +128,15 @@ sub clear_query
return;
}
+# Clear current line to start over
+# (this will work in an incomplete string literal, but it's less desirable
+# than clear_query because we lose evidence in the history file)
+sub clear_line
+{
+ check_completion("\025\n", qr/postgres=# /, "control-U works");
+ return;
+}
+
# check basic command completion: SEL<tab> produces SELECT<space>
check_completion("SEL\t", qr/SELECT /, "complete SEL<tab> to SELECT");
@@ -142,6 +175,47 @@ check_completion("\\DRD\t", qr/drds /, "complete \\DRD<tab> to \\drds");
clear_query();
+# check filename completion
+check_completion(
+ "\\lo_import tmp_check/some\t",
+ qr|tmp_check/somefile |,
+ "filename completion with one possibility");
+
+clear_query();
+
+# note: readline might print a bell before the completion
+check_completion(
+ "\\lo_import tmp_check/af\t",
+ qr|tmp_check/af\a?ile|,
+ "filename completion with multiple possibilities");
+
+clear_query();
+
+# COPY requires quoting
+# note: broken versions of libedit want to backslash the closing quote;
+# not much we can do about that
+check_completion(
+ "COPY foo FROM tmp_check/some\t",
+ qr|'tmp_check/somefile\\?' |,
+ "quoted filename completion with one possibility");
+
+clear_line();
+
+check_completion(
+ "COPY foo FROM tmp_check/af\t",
+ qr|'tmp_check/afile|,
+ "quoted filename completion with multiple possibilities");
+
+# some versions of readline/libedit require two tabs here, some only need one
+# also, some will offer the whole path name and some just the file name
+# the quotes might appear, too
+check_completion(
+ "\t\t",
+ qr|afile123'? +'?(tmp_check/)?afile456|,
+ "offer multiple file choices");
+
+clear_line();
+
# send psql an explicit \q to shut it down, else pty won't close properly
$timer->start(5);
$in .= "\\q\n";
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 2fd8886..2871159 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -41,6 +41,7 @@
#ifdef USE_READLINE
#include <ctype.h>
+#include <sys/stat.h>
#include "catalog/pg_am_d.h"
#include "catalog/pg_class_d.h"
@@ -63,6 +64,15 @@
#define rl_completion_matches completion_matches
#endif
+/*
+ * Currently we assume that rl_filename_dequoting_function exists if
+ * rl_filename_quoting_function does. If that proves not to be the case,
+ * we'd need to test for the former, or possibly both, in configure.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTING_FUNCTION
+#define USE_FILENAME_QUOTING_FUNCTIONS 1
+#endif
+
/* word break characters */
#define WORD_BREAKS "\t\n@$><=;|&{() "
@@ -157,9 +167,11 @@ typedef struct SchemaQuery
static int completion_max_records;
/*
- * Communication variables set by COMPLETE_WITH_FOO macros and then used by
- * the completion callback functions. Ugly but there is no better way.
+ * Communication variables set by psql_completion (mostly in COMPLETE_WITH_FOO
+ * macros) and then used by the completion callback functions. Ugly but there
+ * is no better way.
*/
+static char completion_last_char; /* last char of input word */
static const char *completion_charp; /* to pass a string */
static const char *const *completion_charpp; /* to pass a list of strings */
static const char *completion_info_charp; /* to pass a second string */
@@ -167,6 +179,7 @@ static const char *completion_info_charp2; /* to pass a third string */
static const VersionedQuery *completion_vquery; /* to pass a VersionedQuery */
static const SchemaQuery *completion_squery; /* to pass a SchemaQuery */
static bool completion_case_sensitive; /* completion is case sensitive */
+static bool completion_force_quote; /* true to force-quote filenames */
/*
* A few macros to ease typing. You can use these to complete the given
@@ -1114,9 +1127,9 @@ static char **get_previous_words(int point, char **buffer, int *nwords);
static char *get_guctype(const char *varname);
-#ifdef NOT_USED
-static char *quote_file_name(char *text, int match_type, char *quote_pointer);
-static char *dequote_file_name(char *text, char quote_char);
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+static char *quote_file_name(char *fname, int match_type, char *quote_pointer);
+static char *dequote_file_name(char *fname, int quote_char);
#endif
@@ -1129,8 +1142,37 @@ initialize_readline(void)
rl_readline_name = (char *) pset.progname;
rl_attempted_completion_function = psql_completion;
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+ rl_filename_quoting_function = quote_file_name;
+ rl_filename_dequoting_function = dequote_file_name;
+#endif
+
rl_basic_word_break_characters = WORD_BREAKS;
+ /*
+ * We should include '"' in rl_completer_quote_characters too, but that
+ * will require some upgrades to how we handle quoted identifiers, so
+ * that's for another day.
+ */
+ rl_completer_quote_characters = "'";
+
+ /*
+ * Set rl_filename_quote_characters to "all possible characters",
+ * otherwise Readline will skip filename quoting if it thinks a filename
+ * doesn't need quoting. Readline actually interprets this as bytes, so
+ * there are no encoding considerations here.
+ */
+#ifdef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+ {
+ unsigned char *fqc = (unsigned char *) pg_malloc(256);
+
+ for (int i = 0; i < 255; i++)
+ fqc[i] = (unsigned char) (i + 1);
+ fqc[255] = '\0';
+ rl_filename_quote_characters = (const char *) fqc;
+ }
+#endif
+
completion_max_records = 1000;
/*
@@ -1455,6 +1497,10 @@ psql_completion(const char *text, int start, int end)
char *text_copy = pnstrdup(rl_line_buffer + start, end - start);
text = text_copy;
+ /* Remember last char of the given input word. */
+ completion_last_char = (end > start) ? text[end - start - 1] : '\0';
+
+ /* We usually want the append character to be a space. */
#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
rl_completion_append_character = ' ';
#endif
@@ -2265,13 +2311,19 @@ psql_completion(const char *text, int start, int end)
Matches("COPY", "BINARY", MatchAny))
COMPLETE_WITH("FROM", "TO");
/* If we have COPY [BINARY] <sth> FROM|TO, complete with filename */
- else if (Matches("COPY|\\copy", MatchAny, "FROM|TO") ||
+ else if (Matches("COPY", MatchAny, "FROM|TO") ||
Matches("COPY", "BINARY", MatchAny, "FROM|TO"))
{
completion_charp = "";
+ completion_force_quote = true; /* COPY requires quoted filename */
+ matches = rl_completion_matches(text, complete_from_files);
+ }
+ else if (Matches("\\copy", MatchAny, "FROM|TO"))
+ {
+ completion_charp = "";
+ completion_force_quote = false;
matches = rl_completion_matches(text, complete_from_files);
}
-
/* Handle COPY [BINARY] <sth> FROM|TO filename */
else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny) ||
Matches("COPY", "BINARY", MatchAny, "FROM|TO", MatchAny))
@@ -3821,6 +3873,7 @@ psql_completion(const char *text, int start, int end)
"\\s|\\w|\\write|\\lo_import"))
{
completion_charp = "\\";
+ completion_force_quote = false;
matches = rl_completion_matches(text, complete_from_files);
}
@@ -4387,14 +4440,58 @@ complete_from_variables(const char *text, const char *prefix, const char *suffix
* 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.
+ *
+ * Caller must set completion_charp to a zero- or one-character string
+ * containing the escape character. This is necessary since \copy has no
+ * escape character, but every other backslash command recognizes "\" as an
+ * escape character.
+ *
+ * Caller must also set completion_force_quote to indicate whether to force
+ * quotes around the result. (The SQL COPY command requires that.)
*/
static char *
complete_from_files(const char *text, int state)
{
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
+
+ /*
+ * If we're using a version of Readline that supports filename quoting
+ * hooks, rely on those, and invoke rl_filename_completion_function()
+ * without messing with its arguments. Readline does stuff internally
+ * that does not work well at all if we try to handle dequoting here.
+ * Instead, Readline will call quote_file_name() and dequote_file_name()
+ * (see below) at appropriate times.
+ *
+ * ... or at least, mostly it will. There are some paths involving
+ * unmatched file names in which Readline never calls quote_file_name(),
+ * and if left to its own devices it will incorrectly append a quote
+ * anyway. Set rl_completion_suppress_quote to prevent that. If we do
+ * get to quote_file_name(), we'll clear this again. (Yes, this seems
+ * like it's working around Readline bugs.)
+ *
+ * (For now, we assume that rl_completion_suppress_quote exists if the
+ * filename quoting hooks do.)
+ */
+ rl_completion_suppress_quote = 1;
+
+ /* If user typed a quote, force quoting (never remove user's quote) */
+ if (*text == '\'')
+ completion_force_quote = true;
+
+ return rl_filename_completion_function(text, state);
+#else
+
+ /*
+ * Otherwise, we have to do the best we can.
+ */
static const char *unquoted_text;
char *unquoted_match;
char *ret = NULL;
+ /* If user typed a quote, force quoting (never remove user's quote) */
+ if (*text == '\'')
+ completion_force_quote = true;
+
if (state == 0)
{
/* Initialization: stash the unquoted input. */
@@ -4411,22 +4508,40 @@ complete_from_files(const char *text, int state)
unquoted_match = rl_filename_completion_function(unquoted_text, state);
if (unquoted_match)
{
- /*
- * Caller sets completion_charp to a zero- or one-character string
- * containing the escape character. This is necessary since \copy has
- * no escape character, but every other backslash command recognizes
- * "\" as an escape character. Since we have only two callers, don't
- * bother providing a macro to simplify this.
- */
+ struct stat statbuf;
+ bool is_dir = (stat(unquoted_match, &statbuf) == 0 &&
+ S_ISDIR(statbuf.st_mode) != 0);
+
+ /* Re-quote the result, if needed. */
ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
- '\'', *completion_charp, pset.encoding);
+ '\'', *completion_charp,
+ completion_force_quote,
+ pset.encoding);
if (ret)
free(unquoted_match);
else
ret = unquoted_match;
+
+ /*
+ * If it's a directory, replace trailing quote with a slash; this is
+ * usually more convenient. (If we didn't quote, leave this to
+ * libedit.)
+ */
+ if (*ret == '\'' && is_dir)
+ {
+ char *retend = ret + strlen(ret) - 1;
+
+ Assert(*retend == '\'');
+ *retend = '/';
+ /* Try to prevent libedit from adding a space, too */
+#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
+ rl_completion_append_character = '\0';
+#endif
+ }
}
return ret;
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
}
@@ -4678,46 +4793,108 @@ get_guctype(const char *varname)
return guctype;
}
-#ifdef NOT_USED
+#ifdef USE_FILENAME_QUOTING_FUNCTIONS
/*
- * Surround a string with single quotes. This works for both SQL and
- * psql internal. Currently disabled because it is reported not to
- * cooperate with certain versions of readline.
+ * Quote a filename according to SQL rules, returning a malloc'd string.
+ * completion_charp must point to escape character or '\0', and
+ * completion_force_quote must be set correctly, as per comments for
+ * complete_from_files().
*/
static char *
-quote_file_name(char *text, int match_type, char *quote_pointer)
+quote_file_name(char *fname, int match_type, char *quote_pointer)
{
char *s;
- size_t length;
+ struct stat statbuf;
+
+ /* Quote if needed. */
+ s = quote_if_needed(fname, " \t\r\n\"`",
+ '\'', *completion_charp,
+ completion_force_quote,
+ pset.encoding);
+ if (!s)
+ s = pg_strdup(fname);
+
+ /*
+ * However, some of the time we have to strip the trailing quote from what
+ * we send back. Never strip the trailing quote if the user already typed
+ * one; otherwise, suppress the trailing quote if we have multiple/no
+ * matches (because we don't want to add a quote if the input is seemingly
+ * unfinished), or if the input was already quoted (because Readline will
+ * do arguably-buggy things otherwise), or if the file does not exist, or
+ * if it's a directory.
+ */
+ if (*s == '\'' &&
+ completion_last_char != '\'' &&
+ (match_type != SINGLE_MATCH ||
+ (quote_pointer && *quote_pointer == '\'') ||
+ stat(fname, &statbuf) != 0 ||
+ S_ISDIR(statbuf.st_mode)))
+ {
+ char *send = s + strlen(s) - 1;
+
+ Assert(*send == '\'');
+ *send = '\0';
+ }
- (void) quote_pointer; /* not used */
+ /*
+ * And now we can let Readline do its thing with possibly adding a quote
+ * on its own accord. (This covers some additional cases beyond those
+ * dealt with above.)
+ */
+ rl_completion_suppress_quote = 0;
+
+ /*
+ * If user typed a leading quote character other than single quote (i.e.,
+ * double quote), zap it, so that we replace it with the correct single
+ * quote.
+ */
+ if (quote_pointer && *quote_pointer != '\'')
+ *quote_pointer = '\0';
- length = strlen(text) +(match_type == SINGLE_MATCH ? 3 : 2);
- s = pg_malloc(length);
- s[0] = '\'';
- strcpy(s + 1, text);
- if (match_type == SINGLE_MATCH)
- s[length - 2] = '\'';
- s[length - 1] = '\0';
return s;
}
+/*
+ * Dequote a filename, if it's quoted.
+ * completion_charp must point to escape character or '\0', as per
+ * comments for complete_from_files().
+ */
static char *
-dequote_file_name(char *text, char quote_char)
+dequote_file_name(char *fname, int quote_char)
{
- char *s;
- size_t length;
+ char *unquoted_fname;
+
+ /*
+ * If quote_char is set, it's not included in "fname". We have to add it
+ * or strtokx will not interpret the string correctly (notably, it won't
+ * recognize escapes).
+ */
+ if (quote_char == '\'')
+ {
+ char *workspace = (char *) pg_malloc(strlen(fname) + 2);
- if (!quote_char)
- return pg_strdup(text);
+ workspace[0] = quote_char;
+ strcpy(workspace + 1, fname);
+ unquoted_fname = strtokx(workspace, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
+ free(workspace);
+ }
+ else
+ unquoted_fname = strtokx(fname, "", NULL, "'", *completion_charp,
+ false, true, pset.encoding);
- length = strlen(text);
- s = pg_malloc(length - 2 + 1);
- strlcpy(s, text +1, length - 2 + 1);
+ /* expect a NULL return for the empty string only */
+ if (!unquoted_fname)
+ {
+ Assert(*fname == '\0');
+ unquoted_fname = fname;
+ }
- return s;
+ /* readline expects a malloc'd result that it is to free */
+ return pg_strdup(unquoted_fname);
}
-#endif /* NOT_USED */
+
+#endif /* USE_FILENAME_QUOTING_FUNCTIONS */
#endif /* USE_READLINE */
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 050c48b..983d94e 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -488,6 +488,14 @@
/* Define to 1 if you have the `rl_filename_completion_function' function. */
#undef HAVE_RL_FILENAME_COMPLETION_FUNCTION
+/* Define to 1 if you have the global variable 'rl_filename_quote_characters'.
+ */
+#undef HAVE_RL_FILENAME_QUOTE_CHARACTERS
+
+/* Define to 1 if you have the global variable 'rl_filename_quoting_function'.
+ */
+#undef HAVE_RL_FILENAME_QUOTING_FUNCTION
+
/* Define to 1 if you have the `rl_reset_screen_size' function. */
#undef HAVE_RL_RESET_SCREEN_SIZE
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 909bded..979964c 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -333,6 +333,8 @@ sub GenerateFiles
HAVE_RL_COMPLETION_APPEND_CHARACTER => undef,
HAVE_RL_COMPLETION_MATCHES => undef,
HAVE_RL_FILENAME_COMPLETION_FUNCTION => undef,
+ HAVE_RL_FILENAME_QUOTE_CHARACTERS => undef,
+ HAVE_RL_FILENAME_QUOTING_FUNCTION => undef,
HAVE_RL_RESET_SCREEN_SIZE => undef,
HAVE_SECURITY_PAM_APPL_H => undef,
HAVE_SETPROCTITLE => undef,
Re: BUG #16059: Tab-completion of filenames in COPY commands removesrequired quotes
From
Peter Eisentraut
Date:
On 2020-01-14 02:38, Tom Lane wrote: > I wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> I have found a weird behavior with identifier quoting, which is not the >>> subject of this patch, but it appears to be affected by it. > >> I'll think about how to improve this. Given that we're dequoting >> filenames explicitly anyway, maybe we don't need to tell readline that >> double-quote is a quoting character. Another idea is that maybe >> we ought to refactor things so that identifier dequoting and requoting >> is handled explicitly, similarly to the way filenames work now. >> That would make the patch a lot bigger though. > > On reflection, it seems like the best bet for the moment is to > remove double-quote from the rl_completer_quote_characters string, > which should restore all behavior around double-quoted strings to > what it was before. (We have to keep single-quote in that string, > though, or quoted file names fail entirely.) This patch (version 6) looks good to me. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
From
Tom Lane
Date:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-01-14 02:38, Tom Lane wrote:
>> On reflection, it seems like the best bet for the moment is to
>> remove double-quote from the rl_completer_quote_characters string,
>> which should restore all behavior around double-quoted strings to
>> what it was before. (We have to keep single-quote in that string,
>> though, or quoted file names fail entirely.)
> This patch (version 6) looks good to me.
Thanks for reviewing! Pushed, now we'll see what the buildfarm thinks...
regards, tom lane