Re: Review: psql include file using relative path

On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <schmiddy@gmail.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

Cool.  I see you (or someone) has added this to the entry for that
patch on commitfest.postgresql.org as well, which is great.  I have
updated that entry to list you as the reviewer and changed the status
of the patch to "Waiting on Author" pending resolution of the issues
you observed.

> == General  ==
> The patch applies cleanly to HEAD. No regression tests are included,
> but I don't think they're needed here.

I agree.

> == 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.

I agree with this too.

> 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.

And this.

[...snip...]
> 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.

Depends if canonicalize_path() has already been applied to that path.

> 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)

pgindent likes to move things backward to make them fit within 80 columns.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company