Re: review: psql: edit function, show function commands patch

On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I hope so I found and fixed last issue - the longer functions was
> showed directly - without a pager.

As a matter of style, I suggest leaving bool *edited as the last
argument to do_edit() and inserting int lineno as the second-to-last
argument.

! 			int  lineno = -1;
[...]
! 					if (atoi(ln) < 1)
! 					{
! 						psql_error("invalid line number\n");
! 						status = PSQL_CMD_ERROR;
! 					}
! 					else
! 						lineno = atoi(ln);

Why call atoi(n) twice?  Can't you just write:

lineno = atoi(n);
if (lineno < 1) {
   ...error stuff...
}

I suggested writing psql_assert(datag != NULL) rather than just
psql_assert(datag).

Instead of: cannot use a editor navigation without setting
EDITOR_LINENUMBER_SWITCH variable
I suggest: cannot edit a specific line because the
EDITOR_LINENUMBER_SWITCH variable is not set

I don't find the name get_dg_tag() to be very mnemonic.  How about
get_function_dollarquote_tag()?

In help.c, it looks like you've only added one line but incremented
the pager count by three.

In this bit:

! 				dqtag = get_dq_tag(lines);
! 				if (dqtag)
! 				{
! 					free(dqtag);
! 					break;
! 				}
! 				else
! 					lineno++;

The "else" is unnecessary.  And just after that:

! 				if (end_of_line)
! 					lines = end_of_line + 1;
! 				else
! 					break;

You can write this more cleanly by saying if (!end_of_line) break;
lines = end_of_line + 1.

The following diff hunk (2213,2218) just adds a blank line and is
therefore unnecessary.  There's a similar hunk in the docs portion of
the patch.

In this part:

  			func = psql_scan_slash_option(scan_state,
  										  OT_WHOLE_LINE, NULL, true);
! 			lineno = extract_lineno_from_funcdesc(func, &status);
! 			
! 			if (status != PSQL_CMD_ERROR)
  			{
! 				if (!func)
! 				{
! 					/* set up an empty command to fill in */
! 					printfPQExpBuffer(query_buf,
! 									  "CREATE FUNCTION ( )\n"
! 									  " RETURNS \n"
! 									  " LANGUAGE \n"
! 									  " -- common options:  IMMUTABLE  STABLE  STRICT  SECURITY
DEFINER\n"
! 									  "AS $function$\n"
! 									  "\n$function$\n");
! 				}
! 				else if (!lookup_function_oid(pset.db, func, &foid))
! 				{
! 					/* error already reported */
! 					status = PSQL_CMD_ERROR;
! 				}
! 				else if (!get_create_function_cmd(pset.db, foid, query_buf))
! 				{
! 					/* error already reported */
! 					status = PSQL_CMD_ERROR;
! 				}
! 				if (func)
! 					free(func);
  			}

Why is it correct for if (func) free(func) to be inside the if (status
!= PSQL_CMD_ERROR) block?  It looks to me like func gets initialized
first, before status is set.

It seems unnecessary for extract_lineno_from_funcdesc() to return the
line number and have a separate out parameter to return a
backslashResult.  Can't you just return -1 to indicate an error?  (You
might need to move the if (!func) test at the top to the caller, but
that seems OK.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company