Thanks a lot for the review. My responses are inline below.
On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <[email protected]>wrote:
> I had a chance to give this patch a look. This review is of the second
> patch posted by Gurjeet, at:
>
> http://archives.postgresql.org/message-id/[email protected]
>
> == Summary ==
> This patch implements the \ir command for psql, with a long alias
> \include_relative. This new backslash command is similar to the
> existing \i or \include command, but it allows loading .sql files with
> paths in reference to the currently-executing script's directory, not
> the CWD of where psql is called from. This feature would be used when
> one .sql file needs to load another .sql file in a related directory.
>
> == Utility ==
> I would find the \ir command useful for constructing small packages of
> SQL to be loaded together. For example, I keep the DDL for non-trivial
> databases in source control, often broken down into one file or more
> per schema, with a master file loading in all the sub-files; this
> command would help the master file be sure it's referencing the
> locations of other files correctly.
>
> == General ==
> The patch applies cleanly to HEAD. No regression tests are included,
> but I don't think they're needed here.
>
> == Documentation ==
> The patch includes the standard psql help output description for the
> new \ir command. I think ./doc/src/sgml/ref/psql-ref.sgml needs to be
> patched as well, though.
>
Done.
>
> Tangent: AFAICT we're not documenting the long form of psql commands,
> such as \print, anywhere. Following that precedent, this patch doesn't
> document \include_relative. Not sure if we want to document such
> options anywhere, but in any case a separate issue from this patch.
>
> == Code ==
> 1.) I have some doubts about whether the memory allocated here:
> char *relative_file = pg_malloc(dir_len + 1 + file_len + 1);
> is always free()'d, particularly if this condition is hit:
>
> if (!fd)
> {
> psql_error("%s: %s\n", filename, strerror(errno));
> return EXIT_FAILURE;
> }
>
Fixed.
>
> 2.) This comment should mention \ir
> * Handler for \i, but can be used for other things as well. ...
>
Added.
>
> 3.) settings.h has the comment about pset.inputfile :
> char *inputfile; /* for error reporting */
>
> But this variable is use for more than just "error reporting" now
> (i.e. determining whether psql is executing a file).
>
IMHO, this structure member was already being used for a bit more than error
reporting, so changed the comment to just say
/* File being currently processed, if any */
>
> 4.) I think the changes to process_file() merit another comment or
> two, e.g. describing what relative_file is supposed to be.
>
Added.
>
> 5.) I tried the patch out on Linux and OS X; perhaps someone should
> give it a quick check on Windows as well -- I'm not sure if pathname
> manipulations like:
> last_slash = strrchr(pset.inputfile, '/');
> work OK on Windows.
>
Yes, the canonicalize_path() function call done just a few lines above
converts the Windows style separator to Unix style one.
>
> 6.) The indentation of these lines in tab-complete.c around line 2876 looks
> off:
> strcmp(prev_wd, "\\i") == 0 || strcmp(prev_wd, "\\include") == 0
> ||
> strcmp(prev_wd, "\\ir") == 0 || strcmp(prev_wd,
> "\\include_relative") == 0 ||
>
> (I think the first of those lines was off before the patch, and the
> patch followed its example)
>
>
Yes, I just followed the precedent; that line still crosses the 80 column
limit, though. I'd leave for a pgindent run or the commiter to fix as I
don't know what the right fix would be.
>
> That's it for now. Overall, I think this patch provides a useful
> feature, and am hoping it could be ready for commit in 9.2 in the
> 2011-next commitfest with some polishing.
>
>
Thanks Josh. Updated patch attached.
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ac351d3..e37f75f 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1628,6 +1628,22 @@ Tue Oct 26 21:40:57 CEST 1999
<varlistentry>
+ <term><literal>\ir <replaceable class="parameter">filename</replaceable></literal></term>
+ <listitem>
+ <para>
+ When used within a script, if the <replaceable class="parameter">filename</replaceable>
+ uses relative path notation, then the file will be looked up relative to currently
+ executing file's location.
+
+ If the <replaceable class="parameter">filename</replaceable> uses an absolute path
+ notation, or if this command is being used in interactive mode, then it behaves the
+ same as <literal>\i</> command.
+ </para>
+ </listitem>
+ </varlistentry>
+
+
+ <varlistentry>
<term><literal>\l</literal> (or <literal>\list</literal>)</term>
<term><literal>\l+</literal> (or <literal>\list+</literal>)</term>
<listitem>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5eefcbf..ee887f0 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1974,7 +1974,7 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
* process_file
*
* Read commands from filename and then them to the main processing loop
- * Handler for \i, but can be used for other things as well. Returns
+ * Handler for \i and \ir, but can be used for other things as well. Returns
* MainLoop() error code.
*
* If use_relative_path is true and filename is not an absolute path, then open
@@ -1986,6 +1986,7 @@ process_file(char *filename, bool single_txn, bool use_relative_path)
FILE *fd;
int result;
char *oldfilename;
+ char *relative_file = NULL;
PGresult *res;
if (!filename)
@@ -1995,6 +1996,14 @@ process_file(char *filename, bool single_txn, bool use_relative_path)
{
canonicalize_path(filename);
+ /*
+ * If the currently processing file uses \ir command, and the parameter
+ * to the command is a relative file path, then we resolve this path
+ * relative to currently processing file.
+ *
+ * If the \ir command was executed in interactive mode (i.e. not in a
+ * script) the we treat it the same as \i command.
+ */
if (use_relative_path && pset.inputfile)
{
char *last_slash;
@@ -2007,7 +2016,7 @@ process_file(char *filename, bool single_txn, bool use_relative_path)
size_t dir_len = (last_slash - pset.inputfile) + 1;
size_t file_len = strlen(filename);
- char *relative_file = pg_malloc(dir_len + 1 + file_len + 1);
+ relative_file = pg_malloc(dir_len + 1 + file_len + 1);
relative_file[0] = '\0';
strncat(relative_file, pset.inputfile, dir_len);
@@ -2026,6 +2035,9 @@ process_file(char *filename, bool single_txn, bool use_relative_path)
if (!fd)
{
+ if (relative_path != NULL)
+ free(relative_path);
+
psql_error("%s: %s\n", filename, strerror(errno));
return EXIT_FAILURE;
}
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 7228f9d..2e28d86 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -81,7 +81,7 @@ typedef struct _psqlSettings
bool cur_cmd_interactive;
int sversion; /* backend server version */
const char *progname; /* in case you renamed psql */
- char *inputfile; /* for error reporting */
+ char *inputfile; /* File being currently processed, if any */
char *dirname; /* current directory for \s display */
uint64 lineno; /* also for error reporting */
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
