Re: TAP output format in pg_regress - Mailing list pgsql-hackers

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