Minor bugs and a formatting gripe - Mailing list pgsql-hackers

Bruce mentioned that he'd had some trouble applying my last set of
patches, because he'd already run his reformatting script against
the files involved.  So, I took the trouble to grovel through a
"diff --ignore-all-space" comparison of tonight's cvs tree with what
I had last week.  (I didn't have the energy to go through the *whole*
tree, but I did look through all of src/interfaces/.)  It was a
worthwhile exercise because I found a couple of minor errors --- see
attached patch.

I was mostly pretty happy with what the formatting script had done with
the code, especially in libpgtcl which had not been anywhere close to
matching the formatting conventions of the rest of pgsql.  Nice job!

BUT: I am not happy that the script chooses to reflow block comments.
Line breaks chosen on the basis of how much fits are not a readability
improvement over line breaks placed where a human author felt they made
sense.  For example, is this really an improvement in readability?
(And if it's *not* a clear gain, why are we doing it?)

*** 201,209 ****

  /* === in fe-misc.c === */

!     /* "Get" and "Put" routines return 0 if successful, EOF if not.
!      * Note that for Get, EOF merely means the buffer is exhausted,
!      * not that there is necessarily any error.
       */
      extern int    pqGetc(char *result, PGconn *conn);
      extern int    pqGets(char *s, int maxlen, PGconn *conn);
--- 201,210 ----

  /* === in fe-misc.c === */

!  /*
!   * "Get" and "Put" routines return 0 if successful, EOF if not. Note that
!   * for Get, EOF merely means the buffer is exhausted, not that there is
!   * necessarily any error.
    */
  extern int    pqGetc(char *result, PGconn *conn);
  extern int    pqGets(char *s, int maxlen, PGconn *conn);
***************

The only case within src/interfaces for which I am sufficiently annoyed
to submit a reversing patch is this one:

*** 1462,1476 ****
      if (strncmp(res->cmdStatus, "INSERT ", 7) != 0)
          return "";

!     /* The cmdStatus string looks like
!      *     INSERT oid count\0
!      * In order to be able to return an ordinary C string without
!      * damaging the result for PQcmdStatus or PQcmdTuples, we copy
!      * the oid part of the string to just after the null, so that
!      * cmdStatus looks like
!      *     INSERT oid count\0oid\0
!      *                       ^ our return value points here
!      * Pretty klugy eh?  This routine should've just returned an Oid value.
       */

      slen = strlen(res->cmdStatus);
--- 1488,1500 ----
      if (strncmp(res->cmdStatus, "INSERT ", 7) != 0)
          return "";

!     /*
!      * The cmdStatus string looks like INSERT oid count\0 In order to be
!      * able to return an ordinary C string without damaging the result for
!      * PQcmdStatus or PQcmdTuples, we copy the oid part of the string to
!      * just after the null, so that cmdStatus looks like INSERT oid
!      * count\0oid\0 ^ our return value points here Pretty klugy eh?  This
!      * routine should've just returned an Oid value.
       */

      slen = strlen(res->cmdStatus);
***************

but I hope this one drives home the point that automatic reflow of
block comments is a pretty stupid idea.  I strongly recommend that
that "feature" be disabled forthwith.  I'd further recommend that
someone look through the full source tree to see if any other block
comments have been rendered equally illegible.  I haven't got time
to do it myself right now, however.

Patch attached that corrects two minor errors introduced into
libpgtcl and libpq, and corrects the above loss of readability
of one block comment.

            regards, tom lane

*** src/interfaces/libpgtcl/pgtclCmds.c.orig    Thu Sep  3 01:08:28 1998
--- src/interfaces/libpgtcl/pgtclCmds.c    Thu Sep  3 23:06:36 1998
***************
*** 616,623 ****
              for (i = 1; i < PQnfields(result); i++)
              {
                  sprintf(workspace, "%s,%.200s%s", field0, PQfname(result,i),
!                     appendstr);
!                 sprintf(workspace, "%s,%.200s", field0, PQfname(result,i));
                  if (Tcl_SetVar2(interp, arrVar, workspace,
                                  PQgetvalue(result, tupno, i),
                                  TCL_LEAVE_ERR_MSG) == NULL)
--- 616,622 ----
              for (i = 1; i < PQnfields(result); i++)
              {
                  sprintf(workspace, "%s,%.200s%s", field0, PQfname(result,i),
!                         appendstr);
                  if (Tcl_SetVar2(interp, arrVar, workspace,
                                  PQgetvalue(result, tupno, i),
                                  TCL_LEAVE_ERR_MSG) == NULL)
*** src/interfaces/libpq/libpq-fe.h.orig    Wed Sep  2 22:10:51 1998
--- src/interfaces/libpq/libpq-fe.h    Thu Sep  3 23:37:43 1998
***************
*** 182,188 ****
      extern ConnStatusType PQstatus(PGconn *conn);
      extern char *PQerrorMessage(PGconn *conn);
      extern int    PQsocket(PGconn *conn);
-     extern int    PQsocket(PGconn *conn);
      extern int    PQbackendPID(PGconn *conn);

      /* Enable/disable tracing */
--- 182,187 ----
*** src/interfaces/libpq/fe-exec.c.orig    Wed Sep  2 22:10:47 1998
--- src/interfaces/libpq/fe-exec.c    Thu Sep  3 23:33:42 1998
***************
*** 1489,1500 ****
          return "";

      /*
!      * The cmdStatus string looks like INSERT oid count\0 In order to be
!      * able to return an ordinary C string without damaging the result for
!      * PQcmdStatus or PQcmdTuples, we copy the oid part of the string to
!      * just after the null, so that cmdStatus looks like INSERT oid
!      * count\0oid\0 ^ our return value points here Pretty klugy eh?  This
!      * routine should've just returned an Oid value.
       */

      slen = strlen(res->cmdStatus);
--- 1489,1503 ----
          return "";

      /*
!      * The cmdStatus string looks like
!      *     INSERT oid count\0
!      * In order to be able to return an ordinary C string without
!      * damaging the result for PQcmdStatus or PQcmdTuples, we copy
!      * the oid part of the string to just after the null, so that
!      * cmdStatus looks like
!      *     INSERT oid count\0oid\0
!      *                       ^ our return value points here
!      * Pretty klugy eh?  This routine should've just returned an Oid value.
       */

      slen = strlen(res->cmdStatus);