Re: Patch : PGPASSFILE fix

On Wed, Mar 4, 2015 at 8:44 AM, Prasad <prasad(dot)s(at)mail(dot)com> wrote:

> Ashesh,
>
> Thanks for reviewing patch,
> Code I have removed in I think, was switch statement inside if condition,
> which doesn't make sense.
> ie.
> if (var == 2)
> {
> switch (var)
> case 2:
> .....
> break;
> }
>
> that's why I removed it, because it's redundant.
>
Agree about redundancy, but you've also removed the code for checking the
PGPASS check at the start of the function.
i.e.

*@@ -762,35 +762,33 @@ void sysSettings::SetCanonicalLanguage(const
wxLanguage
&lang) //////////////////////////////////////////////////////////////////////////
wxString
sysSettings::GetConfigFile(configFileName cfgname) {- if (cfgname ==
PGPASS)- {*

I am not agree with that.

> About creation of directory, I'm not sure if this validation is required.
> Existing code creates directory postgresql (only on windows) according to
> http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html , and it
> doesn't create file. I'm not sure whether this kind of validation is
> expected in this function.
>
I think - it is.
Because - it could be used to save the updated password in the PGPASS file.

-- Ashesh

>
> regards,
> Prasad
>
> Sent: Wednesday, March 04, 2015 at 7:15 AM
> From: "Ashesh Vashi" <ashesh(dot)vashi(at)enterprisedb(dot)com>
> To: Prasad <prasad(dot)s(at)mail(dot)com>
> Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
> Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
>
> Hi Prasad,
> I see couple of issues with your patch.* Please generate the patch using
> 'git diff'.
> I could not apply your patch straight forwardly.
> I had to use the patch utility.
> * Please follow the coding style of pgAdmin.
> You can find it at
> https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.* Do not
> remove any of the existing code.
> It has been kept there keeping in mind about future development
> extending support of the existing functionality.
> You've removed couple of lines in the sysSettings::GetConfigFile(...)
> function, which is not good.
>
> In your code:* Checked only for PGPASSFILE environment variable.
> * Need to check the existence of the file.
> * Take required actions (if that file/parent directory does not exists).
> i.e. Create parent directory
>
>
>
> --
> Thanks & Regards,
>
> Ashesh Vashi
> EnterpriseDB INDIA: Enterprise PostgreSQL Company[
> http://www.enterprisedb.com]
>
>
> http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi]
>
> On Sun, Mar 1, 2015 at 11:08 PM, Prasad <prasad(dot)s(at)mail(dot)com[
> prasad(dot)s(at)mail(dot)com]> wrote:
> Hi,
>
> Find attached fix for reading PGPASSFILE environment variable for pg
> password file.
>
> regards,
> Prasad
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org[
> pgadmin-hackers(at)postgresql(dot)org])
> To make changes to your subscription:
>
> http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers]
>
>