I think we should try to create the full path if necessary, and simply
throw an error if we can't.
On Wed, Mar 4, 2015 at 10:01 AM, Prasad <prasa...@mail.com> wrote:
> Alright , I'll revert to PGPASS check.
> Existing function only creates folder containing file. With this case, whats
> expected ? Reading value in PGPASSFILE and try to create folder containing
> pgpass file (Assuming it's valid path)? Remember, it's environment variable.
> User can specify anything in there. Some garbage value as well. If we don't
> do any validation there, user will automatically see error with complain
> about file ?
>
> thanks and regards,
> Prasad
>
>
> Sent: Wednesday, March 04, 2015 at 7:48 AM
> From: "Ashesh Vashi" <ashesh.va...@enterprisedb.com>
> To: Prasad <prasa...@mail.com>
> Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
> Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
>
> On Wed, Mar 4, 2015 at 8:44 AM, Prasad <prasa...@mail.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[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.va...@enterprisedb.com[ashesh.va...@enterprisedb.com]>
> To: Prasad <prasa...@mail.com[prasa...@mail.com]>
> Cc: pgadmin-hackers
> <pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.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.*[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.enterprisedb.com]]
>
> http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi][http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi]]
>
> On Sun, Mar 1, 2015 at 11:08 PM, Prasad
> <prasa...@mail.com[prasa...@mail.com][prasa...@mail.com[prasa...@mail.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@postgresql.org[pgadmin-hackers@postgresql.org][pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org]])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers][http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers]]
>
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
