Thread: doc: modify the comment in function libpqrcv_check_conninfo()
Hi, The function 'libpqrcv_check_conninfo()' returns 'void', but the comment above says that the function returns true or false. I've attached a patch to modify the comment. Regard, Rintaro Ikeda
Attachment
On Wed, 26 Jun 2024 at 14:53, ikedarintarof <ikedarintarof@oss.nttdata.com> wrote: > The function 'libpqrcv_check_conninfo()' returns 'void', but the comment > above says that the function returns true or false. > I've attached a patch to modify the comment. Agreed that the current comment is wrong, but the new comment should mention the must_use_password argument. Because only if must_use_password is true, will it throw an error.
Thank you for your comment! I've added the must_use_password argument. A new patch is attached. On 2024-06-26 23:36, Jelte Fennema-Nio wrote: > On Wed, 26 Jun 2024 at 14:53, ikedarintarof > <ikedarintarof@oss.nttdata.com> wrote: >> The function 'libpqrcv_check_conninfo()' returns 'void', but the >> comment >> above says that the function returns true or false. >> I've attached a patch to modify the comment. > > Agreed that the current comment is wrong, but the new comment should > mention the must_use_password argument. Because only if > must_use_password is true, will it throw an error.
Attachment
On Thu, 27 Jun 2024 at 09:09, ikedarintarof <ikedarintarof@oss.nttdata.com> wrote: > > Thank you for your comment! > > I've added the must_use_password argument. A new patch is attached. s/whther/whether nit: "it will do nothing" sounds a bit strange to me (but I'm not native english). Something like this reads more natural to me: an error. If must_use_password is true, the function raises an error if no password is provided in the connection string. In any other case it successfully completes.
Thanks for your suggestion. I used ChatGPT to choose the wording, but it's still difficult for me. The new patch includes your suggestion. On 2024-06-27 17:18, Jelte Fennema-Nio wrote: > On Thu, 27 Jun 2024 at 09:09, ikedarintarof > <ikedarintarof@oss.nttdata.com> wrote: >> >> Thank you for your comment! >> >> I've added the must_use_password argument. A new patch is attached. > > s/whther/whether > > nit: "it will do nothing" sounds a bit strange to me (but I'm not > native english). Something like this reads more natural to me: > > an error. If must_use_password is true, the function raises an error > if no password is provided in the connection string. In any other case > it successfully completes.
Attachment
On Thu, 27 Jun 2024 at 12:27, ikedarintarof <ikedarintarof@oss.nttdata.com> wrote: > Thanks for your suggestion. I used ChatGPT to choose the wording, but > it's still difficult for me. Looks good to me now (but obviously biased since you took my wording). Adding Robert, since he authored the commit that introduced this comment.
On 2024/07/01 18:15, Jelte Fennema-Nio wrote: > On Thu, 27 Jun 2024 at 12:27, ikedarintarof > <ikedarintarof@oss.nttdata.com> wrote: >> Thanks for your suggestion. I used ChatGPT to choose the wording, but >> it's still difficult for me. > > Looks good to me now (but obviously biased since you took my wording). LGTM, too. * Validate connection info string, and determine whether it might cause * local filesystem access to be attempted. The later part of the above comment for libpqrcv_check_conninfo() seems unclear. My understanding is that if must_use_password is true and this function completes without error, the password must be in the connection string, so there's no need to read the .pgpass file (i.e., no local filesystem access). That part seems to be trying to explaing something like that. Is this correct? Anyway, wouldn't it be better to either remove this unclear part or rephrase it for clarity? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2024-07-08 15:28, Fujii Masao wrote: > On 2024/07/01 18:15, Jelte Fennema-Nio wrote: >> On Thu, 27 Jun 2024 at 12:27, ikedarintarof >> <ikedarintarof@oss.nttdata.com> wrote: >>> Thanks for your suggestion. I used ChatGPT to choose the wording, but >>> it's still difficult for me. >> >> Looks good to me now (but obviously biased since you took my wording). > > LGTM, too. > > > * Validate connection info string, and determine whether it might > cause > * local filesystem access to be attempted. > > The later part of the above comment for libpqrcv_check_conninfo() > seems unclear. My understanding is that if must_use_password is true > and this function completes without error, the password must be > in the connection string, so there's no need to read the .pgpass file > (i.e., no local filesystem access). That part seems to be trying to > explaing something like that. Is this correct? > > Anyway, wouldn't it be better to either remove this unclear part or > rephrase it for clarity? > > Regards, Thank you for your comment. I agree "local filesystem access" is vague. I think the reference to .pgpass (local file system) is not necessary in the comment for libpqrcv_check_conninfo() because it is explained in libpqrcv_connect() as shown below. > /* > * Re-validate connection string. The validation already happened at DDL > * time, but the subscription owner may have changed. If we don't > recheck > * with the correct must_use_password, it's possible that the connection > * will obtain the password from a different source, such as PGPASSFILE > or > * PGPASSWORD. > */ > libpqrcv_check_conninfo(conninfo, must_use_password); I remove the unclear part from the previous patch and add some explanation for later part of the function. Or, I think it is also good to make the comment more simple: > * The function checks that > * 1. connection info string is properly parsed and > * 2. a password is provided if must_use_password is true. > * If the check is failed, the it will raise an error. > */ > static void > libpqrcv_check_conninfo(const char *conninfo, bool must_use_password) Regards, Rintaro Ikeda
Attachment
On 2024/07/08 20:44, ikedarintarof wrote: > I remove the unclear part from the previous patch and add some explanation for > later part of the function. - * Validate connection info string, and determine whether it might cause - * local filesystem access to be attempted. + * The function + * 1. validates connection info string and + * 2. checks a password is provided if must_use_password is true. IMO, "Validate connection info string" is sufficient. The mention of must_use_password is redundant since it's explained in the latter part of the comment. Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
> - * Validate connection info string, and determine whether it might > cause > - * local filesystem access to be attempted. > + * The function > + * 1. validates connection info string and > + * 2. checks a password is provided if must_use_password is true. > > IMO, "Validate connection info string" is sufficient. The mention of > must_use_password is redundant since it's explained in the latter part > of > the comment. That is reasonable for me. I've removed the second one. The modified patch is attached. Regards, Rintaro Ikeda
Attachment
On 2024/07/09 15:56, ikedarintarof wrote: >> - * Validate connection info string, and determine whether it might cause >> - * local filesystem access to be attempted. >> + * The function >> + * 1. validates connection info string and >> + * 2. checks a password is provided if must_use_password is true. >> >> IMO, "Validate connection info string" is sufficient. The mention of >> must_use_password is redundant since it's explained in the latter part of >> the comment. > > That is reasonable for me. I've removed the second one. > The modified patch is attached. Thanks for updating the patch! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION