On Fri, 2021-03-12 at 13:12 -0500, Tom Lane wrote: > I pushed the race-condition-fixing part of this, since that's an > unarguable bug fix and hence seems OK to back-patch. (I added a > check on change of file size, because why not.)
Thank you! > Attached is the rest, just to keep the cfbot happy. Thanks for that as well. > I don't think this is quite committable yet. The division of > labor between do_edit() and its callers seems to need more > thought: in particular, I see that \ef now fails to print > "No changes" when I would expect it to. Hm. My idea was that "No changes" is short for "No changes to the query buffer" and is printed only if you quit the editor, but the query buffer is retained. But it also makes sense to interpret it as "No changes to the function or view definition", so I have put it back according to your expectations. > But the real question > is whether there is any non-error condition in which do_edit > should not set the query_buffer, either to the edit result > or empty. I don't follow. That's exactly what happens... But I guess the confusion is due to my inadequate comments. > If we're going to improve the header comment for > do_edit, I would expect it to specify what happens to the > query_buf, and the fact that I can't write a concise spec > leads me to think that a bit more design effort is needed. I have described the fate of the query buffer in the function comment. I hope it is clearer now. > Also, somewhat cosmetic, but: I feel like the hunk that is > setting discard_on_quit in exec_command_edit is assuming > more than it ought to about what copy_previous_query will do. > Perhaps it's worth making copy_previous_query return a bool > indicating whether it copied previous_buf, and then > exec_command_edit becomes > > discard_on_quit = copy_previous_query(query_buf, previous_buf); That is a clear improvement, and I have done it like that. Perhaps I should restate the problem the patch is trying to solve: test=> INSERT INTO dummy VALUES (DEFAULT); INSERT 0 1 test=> -- quit the editor during the following test=> \e script.sql INSERT 0 1 Ouch. A second line was inserted into "dummy". The same happens with plain \e: if I edit the previous query and quit, I don't expect it to be executed again. Currently, the only way to achieve that is to empty the temporary file and then save it, which is annoying. Attached is version 6. Yours, Laurenz Albe
From 436f48e21adcf207dc267db834a2e80f846fb666 Mon Sep 17 00:00:00 2001 From: Laurenz Albe <laurenz.a...@cybertec.at> Date: Mon, 15 Mar 2021 17:09:22 +0100 Subject: [PATCH] Improve \e, \ef and \ev if the editor is quit Before, if you edited a function or view definition or a script file, quitting the editor without saving would cause the *previous query* to be executed, which is certainly not what anyone would expect. It is better to execute nothing and clear the query buffer in this case. The behavior when editing the current query buffer is unchanged: quitting without saving will retain it. In passing, fix the misplaced function comment for do_edit(). Author: Laurenz Albe <laurenz.a...@cybertec.at> Reviewed-by: Chapman Flack, Jacob Champion Discussion: https://postgr.es/m/0ba3f2a658bac6546d9934ab6ba63a805d46a49b.camel%40cybertec.at --- doc/src/sgml/ref/psql-ref.sgml | 10 ++++-- src/bin/psql/command.c | 63 +++++++++++++++++++++++++--------- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 13c1edfa4d..0bf69174ff 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1970,7 +1970,9 @@ testdb=> </para> <para> - The new contents of the query buffer are then re-parsed according to + If you edit a file or the previous query, and you quit the editor without + modifying the file, the query buffer is cleared. + Otherwise, the new contents of the query buffer are re-parsed according to the normal rules of <application>psql</application>, treating the whole buffer as a single line. Any complete queries are immediately executed; that is, if the query buffer contains or ends with a @@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999 in the form of a <command>CREATE OR REPLACE FUNCTION</command> or <command>CREATE OR REPLACE PROCEDURE</command> command. Editing is done in the same way as for <literal>\edit</literal>. - After the editor exits, the updated command is executed immediately + If you quit the editor without saving, the statement is discarded. + If you save and exit the editor, the updated command is executed immediately if you added a semicolon to it. Otherwise it is redisplayed; type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal> to cancel. @@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999 This command fetches and edits the definition of the named view, in the form of a <command>CREATE OR REPLACE VIEW</command> command. Editing is done in the same way as for <literal>\edit</literal>. - After the editor exits, the updated command is executed immediately + If you quit the editor without saving, the statement is discarded. + If you save and exit the editor, the updated command is executed immediately if you added a semicolon to it. Otherwise it is redisplayed; type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal> to cancel. diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 838f74bbbb..e2af0f56bd 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -148,11 +148,11 @@ static void save_query_text_state(PsqlScanState scan_state, ConditionalStack cst PQExpBuffer query_buf); static void discard_query_text(PsqlScanState scan_state, ConditionalStack cstack, PQExpBuffer query_buf); -static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf); +static bool copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf); static bool do_connect(enum trivalue reuse_previous_specification, char *dbname, char *user, char *host, char *port); static bool do_edit(const char *filename_arg, PQExpBuffer query_buf, - int lineno, bool *edited); + int lineno, bool *edited, bool discard_on_quit); static bool do_shell(const char *command); static bool do_watch(PQExpBuffer query_buf, double sleep); static bool lookup_object_oid(EditableObjectType obj_type, const char *desc, @@ -418,7 +418,7 @@ exec_command(const char *cmd, * the individual command subroutines. */ if (status == PSQL_CMD_SEND) - copy_previous_query(query_buf, previous_buf); + (void) copy_previous_query(query_buf, previous_buf); return status; } @@ -1004,14 +1004,20 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch, } if (status != PSQL_CMD_ERROR) { + bool discard_on_quit; + expand_tilde(&fname); if (fname) canonicalize_path(fname); - /* If query_buf is empty, recall previous query for editing */ - copy_previous_query(query_buf, previous_buf); + /* + * If query_buf is empty, recall previous query for editing. + * But in that case, the query buffer should be emptied if + * editing doesn't modify the file. + */ + discard_on_quit = copy_previous_query(query_buf, previous_buf); - if (do_edit(fname, query_buf, lineno, NULL)) + if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit)) status = PSQL_CMD_NEWEDIT; else status = PSQL_CMD_ERROR; @@ -1134,7 +1140,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch, { bool edited = false; - if (!do_edit(NULL, query_buf, lineno, &edited)) + if (!do_edit(NULL, query_buf, lineno, &edited, true)) status = PSQL_CMD_ERROR; else if (!edited) puts(_("No changes")); @@ -2637,7 +2643,7 @@ exec_command_watch(PsqlScanState scan_state, bool active_branch, } /* If query_buf is empty, recall and execute previous query */ - copy_previous_query(query_buf, previous_buf); + (void) copy_previous_query(query_buf, previous_buf); success = do_watch(query_buf, sleep); @@ -2961,12 +2967,19 @@ discard_query_text(PsqlScanState scan_state, ConditionalStack cstack, * This is used by various slash commands for which re-execution of a * previous query is a common usage. For convenience, we allow the * case of query_buf == NULL (and do nothing). + * + * Returns "true" if the previous query was copied into the query + * buffer, else "false". */ -static void +static bool copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf) { if (query_buf && query_buf->len == 0) + { appendPQExpBufferStr(query_buf, previous_buf->data); + return true; + } + return false; } /* @@ -3627,12 +3640,7 @@ UnsyncVariables(void) } -/* - * do_edit -- handler for \e - * - * If you do not specify a filename, the current query buffer will be copied - * into a temporary one. - */ +/* helper for do_edit() */ static bool editFile(const char *fname, int lineno) { @@ -3700,10 +3708,20 @@ editFile(const char *fname, int lineno) } -/* call this one */ +/* + * do_edit -- handler for \e + * + * If you do not specify a filename, the current query buffer will be copied + * into a temporary one. + * "edited" will be set to true if the file is modified. + * + * After this function is done, the resulting file will be copied back into the + * query buffer. As an exception to this, the query buffer will be emptied + * if the file was not modified and the caller set "discard_on_quit" to "true". + */ static bool do_edit(const char *filename_arg, PQExpBuffer query_buf, - int lineno, bool *edited) + int lineno, bool *edited, bool discard_on_quit) { char fnametmp[MAXPGPATH]; FILE *stream = NULL; @@ -3860,6 +3878,17 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf, fclose(stream); } } + else + { + /* + * If the file was not modified, and the caller requested it, discard + * the query buffer. + */ + if (discard_on_quit) + resetPQExpBuffer(query_buf); + else + puts(_("No changes")); + } /* remove temp file */ if (!filename_arg) -- 2.26.2
