Re: Review: psql include file using relative path

Thanks a lot for the review. My responses are inline below.

On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com>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/AANLkTi=YJb_A+GGt_pXmRqhBhyiD6aSWWB8h-Lw-KVi0@mail.gmail.com
>
> == 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