Hello 2010/7/23 Jan Urbański <wulc...@wulczer.org>: > On 21/07/10 14:43, Pavel Stehule wrote: >> Hello >> >> I am sending a actualised patch. > > Hi, thanks! > >> I understand to your criticism about line numbering. I have to >> agree. With line numbering the patch is longer. I have a one >> significant reason for it. > >> **** CREATE OR REPLACE FUNCTION public.foo() **** RETURNS integer >> **** LANGUAGE plpgsql **** AS $function$ 1 begin 2 return >> 10/0; 3 end; **** $function$ >> >> postgres=# select foo(); ERROR: division by zero CONTEXT: SQL >> statement "SELECT 10/0" PL/pgSQL function "foo" line 2 at RETURN >> postgres=# > > OK, that convinced me, and also others seem to like the feature. So I'll > just make code remarks this time. > > > In the \e handling code, if the file name was given and there is no line > number, an uninitialised variable will be passed to do_edit. I see that > you want to avoid passing a magic number to do_edit, but I think you > should just treat anything <0 as that magic value, initialise lineno > with -1 and simplify all these nested ifs. >
fixed uninitialised variable. Second is little bit different - there is three states, not only two - lineno is undefined, lineno is wrong and lineno is correct. I would not to ignore a wrong lineno. > Typo in a comment: > when wirst row of function -> when first row of function fixed > > I think the #define for DEFAULT_BODY_SEPARATOR should either be at the > beginning of the file (and then also used in \ef handling) or just > hardcoded in both places. this means some like only local constant - see PARAMS_ARRAY_SIZE in same file. It is used only inside a first_row_is_empty function. It's not used outside this function. > > Any reason why DEFAULT_NAVIGATION_COMMAND has a space in front of it? > no it is useless - fixed > Some lines have >80 characters. there are lot of longer lines - but I can't to modify other lines only for formating. My code has max line about 90 characters (when it doesn't respect more common form for some parts). > > If you agree that a -1 parameter do do_edit will mean "no lineno", then > I think you can change get_lineno_for_navigation to not take a > backslashResult argument and signal errors by returning -1. there are a too much magic constants, so I prefer a cleaner form with backslashResult. The code isn't longer and it reacts better on wrong entered value - negative value is nonsense for this purpose. > > In get_lineno_for_navigation you will have to protect the large comment > block with /*------ otherwise pgindent will reflow it. done > > PSQL_NAVIGATION_COMMAND needs to be documented in the SGML docs. > I changed PSQL_NAVIGATION_COMMAND to PSQL_EDITOR_NAVIGATION_OPTION and append a few lines - as I can - some one who can speak English has to correct it. > > Uff, that's all from me, sorry for bringing up all these small issues, I > hope this will go in soon! > It is your job :) > I'm going to change it to waiting on author again, mainly because of the > uninitialised variable in \d handling, the rest are just stylistic nitpicks. > > Cheers, > Jan > attached updated patch Thank you very much Pavel Stehule
*** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-07-20 05:54:19.000000000 +0200 --- ./doc/src/sgml/ref/psql-ref.sgml 2010-07-23 20:46:49.746690828 +0200 *************** *** 1339,1345 **** <varlistentry> ! <term><literal>\edit</literal> (or <literal>\e</literal>) <literal><optional> <replaceable class="parameter">filename</replaceable> </optional></literal></term> <listitem> <para> --- 1339,1345 ---- <varlistentry> ! <term><literal>\edit</literal> (or <literal>\e</literal>) <literal><optional> <replaceable class="parameter">filename</replaceable> </optional> <optional> linenumber </optional></literal></term> <listitem> <para> *************** *** 1369,1380 **** 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> --- 1369,1386 ---- 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> *************** *** 1397,1402 **** --- 1403,1415 ---- 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> *************** *** 2116,2121 **** --- 2129,2146 ---- <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> *************** *** 2123,2128 **** --- 2148,2159 ---- 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> *************** *** 3066,3071 **** --- 3097,3117 ---- </varlistentry> <varlistentry> + <term><envar>PSQL_EDITOR_NAVIGATION_OPTION</envar></term> + + <listitem> + <para> + Option used for navigation (go to line command) in external + editor. When it isn't defined, then it uses <option>+</option> + on Unix systems and <option>/</option> on Windows systems. For + wide used KDE editors is necessary to set this option to + <option>--line </option>. The space after <literal>line</literal> + is required. + </para> + </listitem> + </varlistentry> + + <varlistentry> <term><envar>SHELL</envar></term> <listitem> *** ./src/bin/psql/command.c.orig 2010-07-23 16:56:54.000000000 +0200 --- ./src/bin/psql/command.c 2010-07-23 20:50:29.189688832 +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); *************** *** 65,70 **** --- 65,72 ---- static void minimal_error_message(PGresult *res); static void printSSLInfo(void); + static bool first_row_is_empty(char *src); + static int get_lineno_for_navigation(char *func, backslashResult *status); #ifdef WIN32 static void checkWin32Codepage(void); *************** *** 513,528 **** 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; free(fname); } } --- 515,552 ---- else { char *fname; ! char *ln; ! int lineno = -1; ! fname = psql_scan_slash_option(scan_state, ! OT_NORMAL, NULL, true); ! ! /* try to get lineno */ if (fname) ! { ! ln = psql_scan_slash_option(scan_state, ! OT_NORMAL, NULL, true); ! if (ln) ! { ! if (atoi(ln) < 1) ! { ! psql_error("line number is unacceptable\n"); ! status = PSQL_CMD_ERROR; ! } ! else ! lineno = atoi(ln); ! } ! } ! if (status != PSQL_CMD_ERROR) ! { ! expand_tilde(&fname); ! if (fname) ! canonicalize_path(fname); ! if (do_edit(fname, query_buf, NULL, lineno)) ! status = PSQL_CMD_NEWEDIT; ! else ! status = PSQL_CMD_ERROR; ! } free(fname); } } *************** *** 533,538 **** --- 557,564 ---- */ else if (strcmp(cmd, "ef") == 0) { + int lineno; + if (!query_buf) { psql_error("no query buffer\n"); *************** *** 545,580 **** func = psql_scan_slash_option(scan_state, OT_WHOLE_LINE, NULL, true); ! if (!func) { ! /* set up an empty command to fill in */ ! printfPQExpBuffer(query_buf, ! "CREATE FUNCTION ( )\n" ! " RETURNS \n" ! " LANGUAGE \n" ! " -- common options: IMMUTABLE STABLE STRICT SECURITY DEFINER\n" ! "AS $function$\n" ! "\n$function$\n"); ! } ! 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) { bool edited = false; ! if (!do_edit(0, query_buf, &edited)) status = PSQL_CMD_ERROR; else if (!edited) puts(_("No changes")); --- 571,615 ---- func = psql_scan_slash_option(scan_state, OT_WHOLE_LINE, NULL, true); ! lineno = get_lineno_for_navigation(func, &status); ! ! if (status != PSQL_CMD_ERROR) { ! if (!func) ! { ! /* set up an empty command to fill in */ ! printfPQExpBuffer(query_buf, ! "CREATE FUNCTION ( )\n" ! " RETURNS \n" ! " LANGUAGE \n" ! " -- common options: IMMUTABLE STABLE STRICT SECURITY DEFINER\n" ! "AS $function$\n" ! "\n$function$\n"); ! } ! 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) { bool edited = false; ! /* correct lineno when first row of function's body is empty */ ! if (!first_row_is_empty(query_buf->data)) ! lineno--; ! ! if (!do_edit(0, query_buf, &edited, lineno)) status = PSQL_CMD_ERROR; else if (!edited) puts(_("No changes")); *************** *** 969,974 **** --- 1004,1097 ---- free(fname); } + /* + * \sf -- show the named function + */ + else if (strcmp(cmd, "sf") == 0 || strcmp(cmd, "sf+") == 0) + { + int skip_lines = -1; + bool with_lno; + + with_lno = (strcmp(cmd, "sf+") == 0); + + 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); + skip_lines = get_lineno_for_navigation(func, &status) - 1; + + if (status != PSQL_CMD_ERROR) + { + 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; + + /* + * PL doesn't calculate first row of function's body + * when first row is empty. So checks first row, and + * correct lineno when it is necessary. + */ + if (first_row_is_empty(query_buf->data)) + lineno--; + + while (*c) + { + /* find next end of line */ + for (ptr = c; *c != '\n'; c++); + *c++ = '\0'; + lineno++; + + /* skip first n lines */ + if (skip_lines > 0 && (skip_lines > 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) { *************** *** 1550,1558 **** */ static bool ! editFile(const char *fname) { const char *editorName; char *sys; int result; --- 1673,1682 ---- */ static bool ! editFile(const char *fname, int lineno) { const char *editorName; + const char *navigation_cmd; char *sys; int result; *************** *** 1566,1571 **** --- 1690,1699 ---- editorName = getenv("VISUAL"); if (!editorName) editorName = DEFAULT_EDITOR; + + navigation_cmd = getenv("PSQL_EDITOR_NAVIGATION_OPTION"); + if (!navigation_cmd) + navigation_cmd = DEFAULT_EDITOR_NAVIGATION_OPTION; /* * On Unix the EDITOR value should *not* be quoted, since it might include *************** *** 1574,1584 **** * 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) --- 1702,1722 ---- * 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 > 0) ! sprintf(sys, "exec %s %s%d '%s'", editorName, navigation_cmd, lineno, fname); ! else ! sprintf(sys, "exec %s '%s'", editorName, fname); #else ! if (lineno > 0) ! sprintf(sys, SYSTEMQUOTE "\"%s\" %s%d \"%s\"" SYSTEMQUOTE, ! editorName, ! navigation_cmd, ! lineno, ! fname); ! else ! sprintf(sys, SYSTEMQUOTE "\"%s\" \"%s\"" SYSTEMQUOTE, editorName, fname); #endif result = system(sys); if (result == -1) *************** *** 1593,1599 **** /* call this one */ static bool ! do_edit(const char *filename_arg, PQExpBuffer query_buf, bool *edited) { char fnametmp[MAXPGPATH]; FILE *stream = NULL; --- 1731,1737 ---- /* call this one */ static bool ! do_edit(const char *filename_arg, PQExpBuffer query_buf, bool *edited, int lineno) { char fnametmp[MAXPGPATH]; FILE *stream = NULL; *************** *** 1685,1691 **** /* call editor */ if (!error) ! error = !editFile(fname); if (!error && stat(fname, &after) != 0) { --- 1823,1829 ---- /* call editor */ if (!error) ! error = !editFile(fname, lineno); if (!error && stat(fname, &after) != 0) { *************** *** 2181,2186 **** --- 2319,2347 ---- return result; } + + /* + * When first row of prosrc has only new-line char, then is ignored. + * This functions returns true, when first row is empty, else returns + * false. + */ + static bool + first_row_is_empty(char *src) + { + char *ptr; + + #define DEFAULT_BODY_SEPARATOR "$function$" + + ptr = strstr(src, DEFAULT_BODY_SEPARATOR); + + psql_assert(ptr != NULL); + + ptr += strlen(DEFAULT_BODY_SEPARATOR); + + return *ptr == '\n'; + } + + /* * Fetches the "CREATE OR REPLACE FUNCTION ..." command that describes the * function with the given OID. If successful, the result is stored in buf. *************** *** 2213,2218 **** --- 2374,2380 ---- return result; } + /* * Report just the primary error; this is to avoid cluttering the output * with, for instance, a redisplay of the internally generated query *************** *** 2241,2243 **** --- 2403,2480 ---- destroyPQExpBuffer(msg); } + + + /* + * Returns lineno used in \sf and \ef commands. + * + * These commands can be completed with number used as line + * number for navigation in showed lines / open file. The most + * simple method for parsing is reading isolated digits from + * right - \ef foo nn, \ef foo(..)nn. Returns -1 when + * lineno isn't defined. + */ + static int + get_lineno_for_navigation(char *func, backslashResult *status) + { + char *endfunc; + char *c; + int lineno = -1; + + if (!func) + return lineno; + + endfunc = func + strlen(func) - 1; + c = endfunc; + + /* skip useles whitespaces */ + while (c >= func) + if (isblank(*c)) + c--; + else + break; + + /* search the most left digit of continuously number */ + while (c >= func) + if (!isdigit(*c)) + break; + else + c--; + + /* + * when left char isn't blank and isn't a right parenthesis + * then command hasn't a lineno. + */ + if (c < endfunc && c > func) + { + if (isblank(*c) || *c == ')') + { + c++; + + if (atoi(c) < 1) + { + psql_error("line number is unacceptable\n"); + *status = PSQL_CMD_ERROR; + } + else + { + /*---------- + * Function get_create_function_cmd appends a few lines + * to function's body. But we would to like use a line + * numbers use a PL parsers - so add three lines to + * lineno: + * CREATE OR REPLACE FUNCTION .. + * RETURNS ... + * LANGUAGE ... + * AS $finction$ + */ + lineno = atoi(c) + 4; + + /* remove lineno from function descriptor */ + *c = '\0'; + } + } + } + + return lineno; + } *** ./src/bin/psql/help.c.orig 2010-07-23 19:51:20.039690385 +0200 --- ./src/bin/psql/help.c 2010-07-23 19:51:39.905689608 +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/settings.h.orig 2010-07-23 19:51:20.043690094 +0200 --- ./src/bin/psql/settings.h 2010-07-23 20:51:12.235691465 +0200 *************** *** 18,25 **** --- 18,27 ---- #if defined(WIN32) || defined(__CYGWIN__) #define DEFAULT_EDITOR "notepad.exe" + #define DEFAULT_EDITOR_NAVIGATION_OPTION "/" #else #define DEFAULT_EDITOR "vi" + #define DEFAULT_EDITOR_NAVIGATION_OPTION "+" #endif #define DEFAULT_PROMPT1 "%/%R%# " *** ./src/bin/psql/tab-complete.c.orig 2010-07-20 05:54:19.000000000 +0200 --- ./src/bin/psql/tab-complete.c 2010-07-23 19:51:39.909687081 +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 }; *************** *** 2501,2506 **** --- 2501,2509 ---- 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);
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers