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
