Re: review: psql: edit function, show function commands patch

On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> b) more robust algorithm for header rows identification
>
> Have not gotten to this one yet.

I notIce that on WIN32 the default editor is notepad.exe and the
default editor navigation option is "/". Does "notepad.exe /lineno
filename" actually work on Windows? A quick Google search suggests
that the answer is "no", which seems somewhat unfortunate: it means
we'd be shipping "broken on Win32 out of the box".

http://www.robvanderwoude.com/commandlineswitches.php#Notepad

This is actually my biggest concern about this patch - that it may be
just too much of a hassle to actually make it work for people. I just
tried setting $EDITOR to MacOS's TextEdit program, and it turns out
that TextEdit doesn't understand +. I'm afraid that we're going to
end up with a situation where it only works for people using emacs or
vi, and everyone else ends up with a broken install (and, possibly, no
clear idea how to fix it). Perhaps it would be better to accept \sf
and reject \sf+ and \ef <func> <lineno>.

Assuming we get past that threshold issue, I'm also wondering whether
the "navigation option" terminology is best; e.g. set
PSQL_EDITOR_NAVIGATION_OPTION to configure it. It doesn't seem
terrible, but it doesn't seem great, either. Anyone have a better
idea?

The docs are a little rough; they could benefit from some editing by a
fluent English speaker. If anyone has time to work on this, it would
be much appreciated.

Instead of "line number is unacceptable", I think you should write
"invalid line number".

"dollar" should not be spelled "dolar". "function" should not be
spelled "finction".

This change looks suspiciously like magic. If it's actually safe, it
needs a comment explaining why:

- sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
+ sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);

Attempting to compile with this patch applied emits a warning on my
machine; whether or not the warning is valid, you need to make it go
away:

command.c: In function ‘HandleSlashCmds’:
command.c:1055: warning: ‘bsep’ may be used uninitialized in this function
command.c:1055: note: ‘bsep’ was declared here

Why does the \sf output have a trailing blank line? This seems weird,
especially because \ef puts no such trailing blank line in the editor.

Instead of:

+ /* skip useles whitespaces */
+ while (c >= func)
+ if (isblank(*c))
+ c--;
+ else
+ break;

...wouldn't it be just as good to write:

while (c >= func && isblank(*c))
c--;

(and similarly elsewhere)

In extract_separator, if you invert the sense of the first if-test,
then you can avoid having to indent the entire function contents.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company