On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <schmi...@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-k...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
