Spacing of options in getopt_long processing

On 04.11.25 20:32, Andrew Dunstan wrote:

Over at [1] Vaibhav complained that the patch was deleting a line
following one of the case branches for handling command line options in
pg_restore.c, and said this was not pertinent to the patch. That's
reasonable, but it made me look into $subject a bit. pg_restore.c has a
mixture, with some options being followed by blank lines and some not.
pg_dumpall.c and pg_dump.c have a blank line after each option, while
psql's startup.c has none. It would be nice to clean this up and have a
consistent style. But what style? Personally I think having a blank line
after each option looks cleaner, and we're not nearly so concerned with
preserving vertical space as we might once have been. I haven't surveyed
other utilities in our suite. Is this worth even pursuing? Do we care
about making each file consistent, or making all the code consistent?

I think it depends. For example, looking through getopt_long() in
initdb.c or pg_receivewal.c, each option processing is very simple.
Would adding blank lines there add anything in terms of clarity? I
doubt it. But then there is pg_resetwal.c, where each option processing
is rather complex, and so the extra blank lines seem almost necessary.

Along those lines, I would suggest that pg_waldump.c adds some blank
lines, but perhaps pg_rewind.c could remove them.

Only what pg_restore.c is doing is clearly wrong. ;-)

I am mindful of the vertical space. Horizontal space is rather cheaper
and the stuff toward the right is usually less important, but that
doesn't apply vertically.

On 2025-11-05 We 11:41 AM, Peter Eisentraut wrote:

On 04.11.25 20:32, Andrew Dunstan wrote:

Over at [1] Vaibhav complained that the patch was deleting a line
following one of the case branches for handling command line options
in pg_restore.c, and said this was not pertinent to the patch. That's
reasonable, but it made me look into $subject a bit. pg_restore.c has
a mixture, with some options being followed by blank lines and some
not. pg_dumpall.c and pg_dump.c have a blank line after each option,
while psql's startup.c has none. It would be nice to clean this up
and have a consistent style. But what style? Personally I think
having a blank line after each option looks cleaner, and we're not
nearly so concerned with preserving vertical space as we might once
have been. I haven't surveyed other utilities in our suite. Is this
worth even pursuing? Do we care about making each file consistent, or
making all the code consistent?

I think it depends.  For example, looking through getopt_long() in
initdb.c or pg_receivewal.c, each option processing is very simple.
Would adding blank lines there add anything in terms of clarity?  I
doubt it.  But then there is pg_resetwal.c, where each option
processing is rather complex, and so the extra blank lines seem almost
necessary.

Along those lines, I would suggest that pg_waldump.c adds some blank
lines, but perhaps pg_rewind.c could remove them.

Only what pg_restore.c is doing is clearly wrong. ;-)

I am mindful of the vertical space.  Horizontal space is rather
cheaper and the stuff toward the right is usually less important, but
that doesn't apply vertically.

OK, I will clean up pg_restore.c and leave it at that.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-Nov-04, Andrew Dunstan wrote:

Over at [1] Vaibhav complained that the patch was deleting a line following
one of the case branches for handling command line options in pg_restore.c,
and said this was not pertinent to the patch. That's reasonable, but it made
me look into $subject a bit. pg_restore.c has a mixture, with some options
being followed by blank lines and some not. pg_dumpall.c and pg_dump.c have
a blank line after each option, while psql's startup.c has none. It would be
nice to clean this up and have a consistent style. But what style?
Personally I think having a blank line after each option looks cleaner, and
we're not nearly so concerned with preserving vertical space as we might
once have been. I haven't surveyed other utilities in our suite. Is this
worth even pursuing? Do we care about making each file consistent, or making
all the code consistent?

I think not all switch blocks are the same. In getopt_long() switch
blocks, I think the empty lines are mostly useless, because the 'break'
plus the changing indentation for the next line is sufficient visual
cue, and the vertical space _is_ worth more than zero. But if you go to
more complex switches (say xact.c ones) then the empty lines are quite
necessary, especially given pgindent's tendency to add indentation to
comments immediately preceding a case line (but even without that).

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
[…] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu machen. Seyd es!
A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues!
Heiligenstädter Testament, L. v. Beethoven, 1802
https://de.wikisource.org/wiki/Heiligenstädter_Testament