From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>,  Peter Eisentraut
 <peter.eisentraut@enterprisedb.com>,  Japin Li <japinli@hotmail.com>,
  Kyotaro Horiguchi <horikyota.ntt@gmail.com>,  Laurenz Albe
 <laurenz.albe@cybertec.at>,  "smithpb2250@gmail.com"
 <smithpb2250@gmail.com>,  "david.zhang@highgo.ca" <david.zhang@highgo.ca>,
  "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: Support tab completion for upper character inputs in psql
References: 
 <OS0PR01MB611303B647967B6949B759F1FB4E9@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<OS0PR01MB6113CA04E06D5BF221BC4FE2FB429@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<OS0PR01MB61136721363BD08576725F22FB089@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<80b093ae-88bd-8d8a-68b9-b535a3ab11be@enterprisedb.com>
	<OS0PR01MB61137C7B2B09C0431415B9DBFBD69@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<54bd7839-f463-ef03-f2d2-ccb8184b9b27@enterprisedb.com>
	<1068530.1641480996@sss.pgh.pa.us>
	<TYCPR01MB6128BD8EA1529F4270114C18FB4D9@TYCPR01MB6128.jpnprd01.prod.outlook.com>
	<MEYP282MB1669EAF2183E67EAF86F7F61B64D9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
	<TYCPR01MB6128D4933189A74AAD326F98FB4D9@TYCPR01MB6128.jpnprd01.prod.outlook.com>
	<3547066.1642272686@sss.pgh.pa.us>
	<OS0PR01MB6113C130461FB8A9DBA0DAEBFB5A9@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<OS0PR01MB6113DB08D8CEA4E214DA37BBFB5F9@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<2304294.1643152287@sss.pgh.pa.us> <2645164.1643315022@sss.pgh.pa.us>
	<OS0PR01MB611360D58EA41FC6557D92C0FB229@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<2771362.1643385796@sss.pgh.pa.us> <2805502.1643405157@sss.pgh.pa.us>
	<2818574.1643408192@sss.pgh.pa.us>
	<OS0PR01MB61130E73CFDB2C0E8FD1A892FB249@OS0PR01MB6113.jpnprd01.prod.outlook.com>
	<324281.1643567679@sss.pgh.pa.us>
Date: Mon, 31 Jan 2022 01:28:45 +0000
In-Reply-To: <324281.1643567679@sss.pgh.pa.us> (Tom Lane's message of "Sun, 30
	Jan 2022 13:34:39 -0500")
Message-ID: <8735l41ynm.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: 
 <https://www.postgresql.org/message-id/8735l41ynm.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain

Tom Lane <tgl@sss.pgh.pa.us> writes:

> "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> writes:
>> Thanks for your V16 patch, I tested it. 
>> The results LGTM.
>
> Pushed, thanks for looking.

I wasn't following this thread, but I noticed a few small potential
improvements when I saw the commit.

First, as noted in the test, it doesn't preserve the case of the input
for keywords appended to the query result.  This is easily fixed by
using `pg_strdup_keyword_case()`, per the first attached patch.

The second might be more of a matter of style or opinion, but I noticed
a bunch of `if (foo) free(foo);`, which is redundant given that
`free(NULL)` is a no-op.  To simplify the code further, I also made
`escape_string(NULL)` be a no-op, returning `NULL`.

- ilmari


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=0001-Respect-COMP_KEYWORD_CASE-for-additional-keywords.patch

From 7b278ea5c48d386e1c6f277f3d94520505205cd2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 31 Jan 2022 00:42:00 +0000
Subject: [PATCH 1/2] Respect COMP_KEYWORD_CASE for additional keywords

When tacking on keywords to the result of a completion query, use
pg_strdup_keyword_case().
---
 src/bin/psql/t/010_tab_completion.pl | 26 ++++++++++++++++++++------
 src/bin/psql/tab-complete.c          |  4 ++--
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index c4f6552ac9..c4a1d5e1bc 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -317,12 +317,26 @@ sub clear_line
 
 clear_line();
 
-# check completion of a keyword offered in addition to object names
-# (that code path currently doesn't preserve case of what's typed)
-check_completion(
-	"comment on constraint foo on dom\t",
-	qr|DOMAIN|,
-	"offer keyword in addition to query result");
+foreach (
+	['lower', 'DOM', 'domain'],
+	['upper', 'dom', 'DOMAIN'],
+	['preserve-lower', 'dom', 'domain'],
+	['preserve-upper', 'DOM', 'DOMAIN'],
+) {
+	my ($case, $in, $out) = @$_;
+
+	# check completion of a keyword offered in addition to object names
+	# and obeys COMP_KEYWORD_CASE
+	check_completion(
+		"\\set COMP_KEYWORD_CASE $case\n",
+		qr/postgres=#/,
+		"set completion case to '$case'");
+	check_completion(
+		"comment on constraint foo on $in\t",
+		qr|$out|,
+		"offer keyword in addition to query result (COMP_KEYWORD_CASE=$case)");
+	clear_line();
+}
 
 clear_query();
 
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b2ec50b4f2..0c7a99be0a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -5032,7 +5032,7 @@ _complete_from_query(const char *simple_query,
 				if (pg_strncasecmp(text, item, strlen(text)) == 0)
 				{
 					num_other++;
-					return pg_strdup(item);
+					return pg_strdup_keyword_case(item, text);
 				}
 			}
 		}
@@ -5050,7 +5050,7 @@ _complete_from_query(const char *simple_query,
 				if (pg_strncasecmp(text, item, strlen(text)) == 0)
 				{
 					num_other++;
-					return pg_strdup(item);
+					return pg_strdup_keyword_case(item, text);
 				}
 			}
 		}
-- 
2.30.2


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=0002-psql-tab-complete-remove-unnecessary-null-checks.patch

From f4d38e9a819fa8650bea201784511654ec1d858e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 31 Jan 2022 00:09:14 +0000
Subject: [PATCH 2/2] psql tab-complete: remove unnecessary null checks

free(NULL) is specified to be a no-op, so no need to check before
calling it.  Also make escape_string(NULL) a no-op to match.
---
 src/bin/psql/tab-complete.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0c7a99be0a..5115f02934 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4555,11 +4555,9 @@ psql_completion(const char *text, int start, int end)
 	free(previous_words);
 	free(words_buffer);
 	free(text_copy);
-	if (completion_ref_object)
-		free(completion_ref_object);
+	free(completion_ref_object);
 	completion_ref_object = NULL;
-	if (completion_ref_schema)
-		free(completion_ref_schema);
+	free(completion_ref_schema);
 	completion_ref_schema = NULL;
 
 	/* Return our Grand List O' Matches */
@@ -4790,21 +4788,9 @@ _complete_from_query(const char *simple_query,
 		 * up suitably-escaped copies of all the strings we need.
 		 */
 		e_object_like = make_like_pattern(objectname);
-
-		if (schemaname)
-			e_schemaname = escape_string(schemaname);
-		else
-			e_schemaname = NULL;
-
-		if (completion_ref_object)
-			e_ref_object = escape_string(completion_ref_object);
-		else
-			e_ref_object = NULL;
-
-		if (completion_ref_schema)
-			e_ref_schema = escape_string(completion_ref_schema);
-		else
-			e_ref_schema = NULL;
+		e_schemaname = escape_string(schemaname);
+		e_ref_object = escape_string(completion_ref_object);
+		e_ref_schema = escape_string(completion_ref_schema);
 
 		initPQExpBuffer(&query_buffer);
 
@@ -4959,12 +4945,9 @@ _complete_from_query(const char *simple_query,
 		/* Clean up */
 		termPQExpBuffer(&query_buffer);
 		free(e_object_like);
-		if (e_schemaname)
-			free(e_schemaname);
-		if (e_ref_object)
-			free(e_ref_object);
-		if (e_ref_schema)
-			free(e_ref_schema);
+		free(e_schemaname);
+		free(e_ref_object);
+		free(e_ref_schema);
 	}
 
 	/* Return the next result, if any, but not if the query failed */
@@ -5401,6 +5384,7 @@ pg_strdup_keyword_case(const char *s, const char *ref)
  * escape_string - Escape argument for use as string literal.
  *
  * The returned value has to be freed.
+ * Returns NULL if called with NULL.
  */
 static char *
 escape_string(const char *text)
@@ -5408,6 +5392,9 @@ escape_string(const char *text)
 	size_t		text_length;
 	char	   *result;
 
+	if (!text)
+		return text;
+
 	text_length = strlen(text);
 
 	result = pg_malloc(text_length * 2 + 1);
-- 
2.30.2


--=-=-=--