Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[ 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