Alastair, * Alastair Turner (b...@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
signature.asc
Description: Digital signature