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

attached updated patch

* don't use a default option for navigation in editor - user have to
set this option explicitly
* name for this psql variable is EDITOR_LINENUMBER_SWITCH -
* updated comments, doc and some issues described by Robert

Regards

Pavel Stehule

2010/8/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> 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
>