Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless) - Mailing list pgsql-hackers

Hello Corey,

Some comments about v4:

Patch applies. "git apply" complained about a space or line somewhere, not 
sure why. make check ok.

> - rebased on post-psql hooks master

Good.

> - took nearly every suggestion for change to documentation

Indeed. Looks ok to me.

> - \if ERROR will throw the entire \if..\endif into IGNORE mode

Ok. I think that it is the better behavior, but other people opinion may 
differ. Opinions are welcome.

> - state is now pushed on all \ifs

Ok.

> - reinstated leveraging of ParseVariableBool

Ok.

> - history is now kept in interactive mode regardless of \if-truth

Ok.

> - reworked coding example to cause less agita

Yep.

> - removed MainLoop "are endifs balanced" variable in favor of in-place
> check which respects ON_ERROR_STOP.

Ok.

> - make changes to psql/Makefile to enable TAP tests and created t/ directory
> - wrote an intentionally failing TAP test to see if "make check" executes
> it. it does not. need help.

A failing test expects code 3, not 0 as written in "t/001_if.pl". With
  is($retcode,'3','Invalid \if respects ON_ERROR_STOP');

instead, the tests succeeds because psql returned 3.

More check should be done about stdout to check that it failed for the 
expected reasons though. And maybe more tests could be added to trigger 
all possible state transition errors (eg else after else, elif after else, 
endif without if, if without endif, whatever...).

> I'm hoping my failure in that last bit is easy to spot/fix, so I can move
> forward with testing unbalanced branching, etc.

Other comments and suggestions:

Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"?

There is a spurious empty line added at the very end of "mainloop.h":
  +   #endif   /* MAINLOOP_H */


I would suggest to add a short one line comment before each test to 
explain what is being tested, like "-- test \elif execution", "-- test 
\else execution"...

Debatable suggestion about "psql_branch_empty": the function name somehow 
suggests an "empty branch", where it is really testing that the stack is 
empty. Maybe the function could be removed and "psql_branch_get_state(...) 
== IF_STATE_NONE" used instead. Not sure.

"psql_branch_end_state": it is a pop, it could be named "psql_branch_pop" 
which would be symmetrical to "psql_branch_push"? Or maybe push should be 
named "begin_state" or "new_state"...

C style details: I would avoid non mandatory parentheses, eg:
  +       return ((strcmp(cmd, "if") == 0 || \  +                       strcmp(cmd, "elif") == 0 || \  +
      strcmp(cmd, "else") == 0 || \  +                       strcmp(cmd, "endif") == 0));
 

I would remove the external double parentheses (( ... )). Also I'm not 
sure why there are end-of-line backslashes on the first instance, maybe a 
macro turned into a function?
  +       return ((s == IFSTATE_NONE) ||  +                       (s == IFSTATE_TRUE) ||  +                       (s ==
IFSTATE_ELSE_TRUE));

I would remove all parenthenses.
 +       return (state->branch_stack == NULL);

Idem.

-- 
Fabien.