Alastair,
* Alastair Turner (bell@ctrlf5.co.za) wrote:
> Patch for the changes discussed in
> http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php
> attached (eventually ...)
>
> In summary: If the input file (-f) doesn't exist or the ouput or log
> files (-o and -l) can't be created psql exits before prompting for a
> password.
On a once-over, the patch looks reasonably good. A couple things
though:
Please be good with the whitespace:
Above 'if (options.logfilename)' you introduce an extra \n, while you
don't have one between the end of that if() { } block and the next. You
also have a whole diff block that's just adding a \n (between
"pset.inputfile = oldfilename;" and "return result;"). Reviewing your
patches before sending them is a good way to find these things. :)
Silly stuff, sure, but since it's your first patch, I figured I'd
mention it. :)
Also, if you're doing the error-reporting in get_fd_for_process and then
every time it's called and returns failure immediately exiting, why not
just error-exit from get_fd_for_process directly..?
Lastly, I'm not convinced that how you broke up process_file() and
process_fd() actually works. Inside the existing process_file(),
filename will be updated to '<stdin>' for error reporting when the input
in 'stdin', but that's now lost in the new process_file() and
process_fd() will always get whatever is in options.action_string, which
could be a '-' instead. In reviewing the patch, I was hoping that
process_fd() wouldn't actually need to have the filename passed in with
the fd, but it does because psql_error() depends on pset.inputfile being
set, which has to be done by the code which calls into MainLoop(), which
is process_fd() with your patch.
Perhaps there's a better way to handle that?
Thanks,
Stephen