Re: TAP output format in pg_regress

You guys are really fast... I only think about problem, it is already
mentioned here... Most issues I've noticed are already fixed before I was able
to say something.

Nevertheless...

/*
* Bailing out is for unrecoverable errors which prevents further testing to
* occur and after which the test run should be aborted. By passing non_rec
* as true the process will exit with _exit(2) and skipping registered exit
* handlers.
*/
static void
bail_out(bool non_rec, const char *fmt,...)
{

In original code, where _exit were called, there were mention about recursion
(whatever it is) as a reason for using _exit() instead of exit(). Now this
mention is gone:

- _exit(2); /* not exit(), that could be recursive */
+ /* Not using the normal bail() as we want _exit */
+ _bail(_("could not stop postmaster: exit code was %d\n"), r);

The only remaining part of recursion is _rec suffix.

I guess we should keep mention of recursion in comments, and for me _rec
stands for "record", not "recursion", I will not guess original meaning by
_rec suffix. I would suggest to change it to _recurs or _recursive to keep
things clear

-----

+#define _bail(...) bail_out(true, __VA_ARGS__)
+#define bail(...) bail_out(false, __VA_ARGS__)

I will never guess what the difference between bail, _bail and bail_out. We
need really good explanation here, or better to use self-explanatory names
here...

I would start bail_out with _ as it is "internal", not used in the code.
And for _bail I would try to find some name that shows it's nature. Like
bail_in_recursion or something.

-----

+ if (ok || ignore)
{
- va_start(ap, fmt);
- vfprintf(logfile, fmt, ap);
- va_end(ap);
+ emit_tap_output(TEST_STATUS, "ok %-5i %s%-*s %8.0f ms%s\n",
+ testnumber,
+ (parallel ? PARALLEL_INDENT : ""),
+ padding, testname,
+ runtime,
+ (ignore ? " # SKIP (ignored)" : ""));
+ }
+ else
+ {
+ emit_tap_output(TEST_STATUS, "not ok %-5i %s%-*s %8.0f ms\n",
+ testnumber,
+ (parallel ? PARALLEL_INDENT : ""),
+ padding, testname,
+ runtime);
}

This magic spell "...%-5i %s%-*s %8.0f ms\n" is too dark to repeat it even two
times. I understand problems with spaces... But may be it would be better
somehow narrow it to one ugly print... Print "ok %-5i "|"not ok %-5i" to
buffer first, and then have one "%s%-*s %8.0f ms%s\n" print or something like
that...

I am not strongly insisting on that, but I feel strong urge to remove code
duplication here...

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su