Hi,
here's a review of the \sf and \ef [num] patch from
http://archives.postgresql.org/message-id/162867791003290927y3ca44051p80e697bc6b19d...@mail.gmail.com
== Formatting ==
The patch has some small tabs/spaces and whitespace issues and it
applies with some offsets, I ran pgindent and rebased against HEAD,
attaching the resulting patch for your convenience.
== Functionality ==
The patch adds the following features:
* \e file.txt num -> starts a editor for the current query buffer
and puts the cursor on the [num] line
* \ef func num -> starts a editor for a function and puts the cursor
on the [num] line
* \sf func -> shows a full CREATE FUNCTION statement for the function
* \sf+ func -> the same, but with line numbers
* \sf[+] func num -> the same, but only from line num onward
It only touches psql, so no performance or backend stability worries.
In my humble opinion, only the \sf[+] is interesting, because it gives
you a copy/pasteable version of the function definition without opening
up an editor, and I can find that useful (OTOH: you can set PSQL_EDITOR
to cat and get the same effect with \ef... ok, just joking). Line
numbers are an extra touch, personally it does not thrill me too much,
but I've nothing against it.
The number variants of \e and \ef work by simply executing $EDITOR +num
file. I tried with some editors that came to my mind, and not all of
them support it (most do, though):
* emacs and emacsclient work
* vi works
* nano works
* pico works
* mcedit works
* kwrite does not work
* kedit does not work
not sure what other people (or for instance Windows people) use. Apart
from no universal support from editors, it does not save that many
keystrokes - at most a couple. In the end you can usually easily jump to
the line you want once you are inside your dream editor.
My recommendation would be to only integrate the \sf[+] part of the
patch, which will have the additional benefit of making it much smaller
and cleaner (will avoid the grotty splitting of the number from the
function name, for instance). But I'm just another user out there, maybe
others will find uses for the other cases.
I would personally not add the leading and trailing newlines to \sf
output, but that's a question of taste.
Docs could use some small grammar fixes, but other than that they're fine.
== Code ==
In \sf code there just a strncmp, so this works:
\sfblablabla funcname
The error for an empty \sf is not great, it should probably look more like
\sf: missing required argument
following the examples of \pset, \copy or \prompt.
Why is lnptr always being passed as a pointer? Looks like a unnecessary
complication and one more variable to care about. Can't we just pass lineno?
== End ==
Cheers,
Jan
*** doc/src/sgml/ref/psql-ref.sgml
--- /tmp/CUVdHd_psql-ref.sgml 2010-07-16 13:31:53.362662393 +0200
***************
*** 1329,1335 ****
<varlistentry>
! <term><literal>\edit</literal> (or <literal>\e</literal>) <literal><optional> <replaceable class="parameter">filename</replaceable> </optional></literal></term>
<listitem>
<para>
--- 1329,1335 ----
<varlistentry>
! <term><literal>\edit</literal> (or <literal>\e</literal>) <literal><optional> <replaceable class="parameter">filename</replaceable> </optional> <optional> linenumber </optional></literal></term>
<listitem>
<para>
***************
*** 1359,1370 ****
systems, <filename>notepad.exe</filename> on Windows systems.
</para>
</tip>
</listitem>
</varlistentry>
<varlistentry>
! <term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> </optional></literal></term>
<listitem>
<para>
--- 1359,1376 ----
systems, <filename>notepad.exe</filename> on Windows systems.
</para>
</tip>
+
+ <para>
+ If <replaceable class="parameter">linenumber</replaceable> is
+ specified, then cursor is moved on this line after start of
+ editor.
+ </para>
</listitem>
</varlistentry>
<varlistentry>
! <term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> </optional> <optional> linenumber </optional> </literal></term>
<listitem>
<para>
***************
*** 1387,1392 ****
--- 1393,1405 ----
If no function is specified, a blank <command>CREATE FUNCTION</>
template is presented for editing.
</para>
+
+ <para>
+ If <replaceable class="parameter">linenumber</replaceable> is
+ specified, then cursor is moved on this line after start of
+ editor. It count lines from start of function body, not from
+ start of text.
+ </para>
</listitem>
</varlistentry>
***************
*** 2106,2111 ****
--- 2119,2136 ----
<varlistentry>
+ <term><literal>\sf[+] <replaceable class="parameter">function_description</replaceable> <optional> linenumber </optional> </literal></term>
+
+ <listitem>
+ <para>
+ This command fetches and shows the definition of the named function,
+ in the form of a <command>CREATE OR REPLACE FUNCTION</> command.
+ If the form <literal>\sf+</literal> is used, then lines are numbered.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>\t</literal></term>
<listitem>
<para>
***************
*** 2113,2118 ****
--- 2138,2149 ----
footer. This command is equivalent to <literal>\pset
tuples_only</literal> and is provided for convenience.
</para>
+
+ <para>
+ If <replaceable class="parameter">linenumber</replaceable> is
+ specified, then cursor is moved on this line after start of
+ editor.
+ </para>
</listitem>
</varlistentry>
*** src/bin/psql/command.c
--- /tmp/uM1Twe_command.c 2010-07-16 13:31:53.366666075 +0200
***************
*** 57,63 ****
PsqlScanState scan_state,
PQExpBuffer query_buf);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
! bool *edited);
static bool do_connect(char *dbname, char *user, char *host, char *port);
static bool do_shell(const char *command);
static bool lookup_function_oid(PGconn *conn, const char *desc, Oid *foid);
--- 57,63 ----
PsqlScanState scan_state,
PQExpBuffer query_buf);
static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
! bool *edited, int *lineno);
static bool do_connect(char *dbname, char *user, char *host, char *port);
static bool do_shell(const char *command);
static bool lookup_function_oid(PGconn *conn, const char *desc, Oid *foid);
***************
*** 488,500 ****
else
{
char *fname;
fname = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
! if (do_edit(fname, query_buf, NULL))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
--- 488,517 ----
else
{
char *fname;
+ char *lineno;
+ int ln,
+ *lnptr = NULL;
+
fname = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
+
+ /* try to get lineno */
+ if (fname)
+ {
+ lineno = psql_scan_slash_option(scan_state,
+ OT_NORMAL, NULL, true);
+ if (lineno)
+ {
+ ln = atoi(lineno);
+ lnptr = &ln;
+ }
+ }
+
expand_tilde(&fname);
if (fname)
canonicalize_path(fname);
! if (do_edit(fname, query_buf, NULL, lnptr))
status = PSQL_CMD_NEWEDIT;
else
status = PSQL_CMD_ERROR;
***************
*** 508,513 ****
--- 525,533 ----
*/
else if (strcmp(cmd, "ef") == 0)
{
+ int lineno;
+ int *lnptr = NULL;
+
if (!query_buf)
{
psql_error("no query buffer\n");
***************
*** 520,525 ****
--- 540,574 ----
func = psql_scan_slash_option(scan_state,
OT_WHOLE_LINE, NULL, true);
+
+ /*
+ * parse linenumber from the right
+ */
+ if (func && *func)
+ {
+ char *endfunc = func + strlen(func) - 1;
+ char *c;
+
+ c = endfunc;
+ while (c >= func)
+ if (!isdigit(*c))
+ break;
+ else
+ c--;
+
+ if (c < endfunc && c > func)
+ {
+ if (*c == ' ' || *c == ')')
+ {
+ c++;
+ /* append rows for SQL decoration */
+ lineno = atoi(c) + 3;
+ *c = '\0';
+ lnptr = &lineno;
+ }
+ }
+ }
+
if (!func)
{
/* set up an empty command to fill in */
***************
*** 549,555 ****
{
bool edited = false;
! if (!do_edit(0, query_buf, &edited))
status = PSQL_CMD_ERROR;
else if (!edited)
puts(_("No changes"));
--- 598,604 ----
{
bool edited = false;
! if (!do_edit(0, query_buf, &edited, lnptr))
status = PSQL_CMD_ERROR;
else if (!edited)
puts(_("No changes"));
***************
*** 944,949 ****
--- 993,1103 ----
free(fname);
}
+ /*
+ * \sf -- show the named function
+ */
+ else if (strncmp(cmd, "sf", 2) == 0)
+ {
+ int lineno;
+ int *lnptr = NULL;
+ bool with_lno = false;
+
+ if (strcmp(cmd, "sf+") == 0)
+ with_lno = true;
+
+ if (!query_buf)
+ {
+ psql_error("no query buffer\n");
+ status = PSQL_CMD_ERROR;
+ }
+ else
+ {
+ char *func;
+ Oid foid = InvalidOid;
+
+ func = psql_scan_slash_option(scan_state,
+ OT_WHOLE_LINE, NULL, true);
+
+ /*
+ * parse linenumber from the right
+ */
+ if (func && *func)
+ {
+ char *endfunc = func + strlen(func) - 1;
+ char *c;
+
+ c = endfunc;
+ while (c >= func)
+ if (!isdigit(*c))
+ break;
+ else
+ c--;
+
+ if (c < endfunc && c > func)
+ {
+ if (*c == ' ' || *c == ')')
+ {
+ c++;
+ /* append rows for SQL decoration */
+ lineno = atoi(c) + 3;
+ *c = '\0';
+ lnptr = &lineno;
+ }
+ }
+ }
+
+ if (!func)
+ {
+ /* show error for empty command */
+ psql_error("missing a function name\n");
+ status = PSQL_CMD_ERROR;
+ }
+ else if (!lookup_function_oid(pset.db, func, &foid))
+ {
+ /* error already reported */
+ status = PSQL_CMD_ERROR;
+ }
+ else if (!get_create_function_cmd(pset.db, foid, query_buf))
+ {
+ /* error already reported */
+ status = PSQL_CMD_ERROR;
+ }
+ if (func)
+ free(func);
+ }
+
+ if (status != PSQL_CMD_ERROR)
+ {
+ int lineno = 0;
+ char *c = query_buf->data;
+ char *ptr;
+
+ printf("\n");
+ while (*c)
+ {
+ for (ptr = c; *c != '\n'; c++);
+ *c++ = '\0';
+ lineno++;
+
+ if (lnptr && (*lnptr > lineno))
+ continue;
+
+ if (!with_lno)
+ printf("%s\n", ptr);
+ else
+ {
+ /* don't show lineno for first three rows and last row */
+ if ((*c == '\0' && lineno != 3) || lineno < 4)
+ printf("**** %s\n", ptr);
+ else
+ printf("%4d %s\n", lineno - 3, ptr);
+ }
+ }
+ printf("\n");
+ fflush(stdout);
+ }
+ }
+
/* \set -- generalized set variable/option command */
else if (strcmp(cmd, "set") == 0)
{
***************
*** 1517,1523 ****
*/
static bool
! editFile(const char *fname)
{
const char *editorName;
char *sys;
--- 1671,1677 ----
*/
static bool
! editFile(const char *fname, int *lineno)
{
const char *editorName;
char *sys;
***************
*** 1541,1551 ****
* severe brain damage in their command shell plus the fact that standard
* program paths include spaces.
*/
! sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
#ifndef WIN32
! sprintf(sys, "exec %s '%s'", editorName, fname);
#else
! sprintf(sys, SYSTEMQUOTE "\"%s\" \"%s\"" SYSTEMQUOTE, editorName, fname);
#endif
result = system(sys);
if (result == -1)
--- 1695,1714 ----
* severe brain damage in their command shell plus the fact that standard
* program paths include spaces.
*/
! sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);
#ifndef WIN32
! if (!lineno)
! sprintf(sys, "exec %s '%s'", editorName, fname);
! else
! sprintf(sys, "exec %s +%d '%s'", editorName, *lineno, fname);
#else
! if (!lineno)
! sprintf(sys, SYSTEMQUOTE "\"%s\" \"%s\"" SYSTEMQUOTE, editorName, fname);
! else
! sprintf(sys, SYSTEMQUOTE "\"%s\" +%d \"%s\"" SYSTEMQUOTE,
! editorName,
! *lineno,
! fname);
#endif
result = system(sys);
if (result == -1)
***************
*** 1560,1566 ****
/* call this one */
static bool
! do_edit(const char *filename_arg, PQExpBuffer query_buf, bool *edited)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
--- 1723,1729 ----
/* call this one */
static bool
! do_edit(const char *filename_arg, PQExpBuffer query_buf, bool *edited, int *lineno)
{
char fnametmp[MAXPGPATH];
FILE *stream = NULL;
***************
*** 1652,1658 ****
/* call editor */
if (!error)
! error = !editFile(fname);
if (!error && stat(fname, &after) != 0)
{
--- 1815,1821 ----
/* call editor */
if (!error)
! error = !editFile(fname, lineno);
if (!error && stat(fname, &after) != 0)
{
*** src/bin/psql/help.c
--- /tmp/g3JMge_help.c 2010-07-16 13:31:53.370666106 +0200
***************
*** 174,186 ****
fprintf(output, "\n");
fprintf(output, _("Query Buffer\n"));
! fprintf(output, _(" \\e [FILE] edit the query buffer (or file) with external editor\n"));
! fprintf(output, _(" \\ef [FUNCNAME] edit function definition with external editor\n"));
fprintf(output, _(" \\p show the contents of the query buffer\n"));
fprintf(output, _(" \\r reset (clear) the query buffer\n"));
#ifdef USE_READLINE
fprintf(output, _(" \\s [FILE] display history or save it to file\n"));
#endif
fprintf(output, _(" \\w FILE write query buffer to file\n"));
fprintf(output, "\n");
--- 174,187 ----
fprintf(output, "\n");
fprintf(output, _("Query Buffer\n"));
! fprintf(output, _(" \\e [FILE] [lno] edit the query buffer (or file) with external editor\n"));
! fprintf(output, _(" \\ef [FUNCNAME] [lno] edit function definition with external editor\n"));
fprintf(output, _(" \\p show the contents of the query buffer\n"));
fprintf(output, _(" \\r reset (clear) the query buffer\n"));
#ifdef USE_READLINE
fprintf(output, _(" \\s [FILE] display history or save it to file\n"));
#endif
+ fprintf(output, _(" \\sf[+] FUNCNAME [lno] show finction definition\n"));
fprintf(output, _(" \\w FILE write query buffer to file\n"));
fprintf(output, "\n");
*** src/bin/psql/tab-complete.c
--- /tmp/ERVkIe_tab-complete.c 2010-07-16 13:31:53.370666106 +0200
***************
*** 644,650 ****
"\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\l",
"\\lo_import", "\\lo_export", "\\lo_list", "\\lo_unlink",
"\\o", "\\p", "\\password", "\\prompt", "\\pset", "\\q", "\\qecho", "\\r",
! "\\set", "\\t", "\\T",
"\\timing", "\\unset", "\\x", "\\w", "\\z", "\\!", NULL
};
--- 644,650 ----
"\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\l",
"\\lo_import", "\\lo_export", "\\lo_list", "\\lo_unlink",
"\\o", "\\p", "\\password", "\\prompt", "\\pset", "\\q", "\\qecho", "\\r",
! "\\set", "\\sf", "\\t", "\\T",
"\\timing", "\\unset", "\\x", "\\w", "\\z", "\\!", NULL
};
***************
*** 2502,2507 ****
--- 2502,2510 ----
else if (strcmp(prev_wd, "\\ef") == 0)
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_functions, NULL);
+ else if (strncmp(prev_wd, "\\sf", 2) == 0)
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_functions, NULL);
+
else if (strcmp(prev_wd, "\\encoding") == 0)
COMPLETE_WITH_QUERY(Query_for_list_of_encodings);
else if (strcmp(prev_wd, "\\h") == 0 || strcmp(prev_wd, "\\help") == 0)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
