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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, steven(dot)winfield(at)cantabcapital(dot)com
Subject: Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Date: 2019-12-13 19:16:41
Message-ID: 18490.1576264601@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

Attachment Content-Type Size
psql-filename-completion-fixes-2.patch text/x-diff 18.8 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Heikki Linnakangas 2019-12-13 22:02:52 Re: BUG #16162: create index using gist_trgm_ops leads to panic
Previous Message Tom Lane 2019-12-13 18:55:07 Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

Browse pgsql-hackers by date

  From Date Subject
Next Message Jeff Janes 2019-12-13 19:43:31 psql's \watch is broken
Previous Message Tom Lane 2019-12-13 18:55:07 Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes