Re: [PATCH] PGSERVICEFILE as part of a normal connection string

On Thu, Mar 20, 2025 at 06:16:44PM +0900, Ryo Kanbayashi wrote:
> Sorry, I found a miss on 006_service.pl.
> Fixed patch is attached...

Please note that the commit fest app needs all the patches of a a set
to be posted in the same message. In this case, v2-0001 is not going
to get automatic test coverage.

Your patch naming policy is also a bit confusing. I would suggest to
use `git format-patch -vN -2`, where N is your version number. 0001
would be the new tests for service files, and 0002 the new feature,
with its own tests.

+if ($windows_os) {
+
+ # Windows: use CRLF
+ print $fh "[my_srv]", "\r\n";
+ print $fh join( "\r\n", split( ' ', $node->connstr ) ), "\r\n";
+}
+else {
+ # Non-Windows: use LF
+ print $fh "[my_srv]", "\n";
+ print $fh join( "\n", split( ' ', $node->connstr ) ), "\n";
+}
+close $fh;

That's duplicated. Let's perhaps use a $newline variable and print
into the file using the $newline?

Question: you are doing things this way in the test because fgets() is
what is used by libpq to retrieve the lines of the service file, is
that right?

Please note that the CI is failing. It seems to me that you are
missing a done_testing() at the end of the script. If you have a
github account, I'd suggest to set up a CI in your own fork of
Postgres, this is really helpful to double-check the correctness of a
patch before posting it to the lists, and saves in round trips between
author and reviewer. Please see src/tools/ci/README in the code tree
for details.

+# Copyright (c) 2023-2024, PostgreSQL Global Development Group

These dates are incorrect. Should be 2025, as it's a new file.

+++ b/src/interfaces/libpq/t/007_servicefile_opt.pl
@@ -0,0 +1,100 @@
+# Copyright (c) 2023-2024, PostgreSQL Global Development Group

Incorrect date again in the second path with the new feature. I'd
suggest to merge all the tests in a single script, with only one node
initialized and started.
--
Michael