Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

From

Junio C Hamano <gitster@pobox.com>

Date

Aug 3, 2016, 20:57 UTC

Message-ID

<xmqq4m718tay.fsf@gitster.mtv.corp.google.com>

In-Reply-To

<20160803161911.dxucq7f2pvnoovoc@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Mon, Aug 01, 2016 at 09:49:37PM +0000, Eric Wong wrote:
>
> You'd want:
>
>   argv_array_pushf(env, "%.*s", (int)(cp - pager_env), pager_env);
>
> Also:
>
>> +		strbuf_reset(&buf);
>
> should this be strbuf_release()? If we didn't follow the conditional
> above (because getenv() told us the variable was already set), then we
> would not do do the detach/release there, and would finish the loop with
> memory still allocated by "buf".

All bugs are from my original, I think.  Here is a proposed squash.

 * This shouldn't make much difference for @@PAGER_ENV@@, but not
   quoting the default assignment, i.e.

   	: "${VAR=VAL}" && export VAR

   is bad manners.  cf. 01247e02 (sh-setup: enclose setting of
   ${VAR=default} in double-quotes, 2016-06-19)

 * Again, this shouldn't make much difference for PAGER_ENV, but
   let's follow the "reset per iteration, release at the end"
   pattern to avoid repeated allocation.

 * Let's avoid a hand-rolled "skip blanks" loop and let strspn() do
   it.

 git-sh-setup.sh |  2 +-
 pager.c         | 15 ++++++---------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index b6b75e9..cda32d0 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -163,7 +163,7 @@ git_pager() {
 	for vardef in @@PAGER_ENV@@
 	do
 		var=${vardef%%=*}
-		eval ": \${$vardef} && export $var"
+		eval ": \"\${$vardef}\" && export $var"
 	done
 
 	eval "$GIT_PAGER" '"$@"'
diff --git a/pager.c b/pager.c
index cd1ac54..7199c31 100644
--- a/pager.c
+++ b/pager.c
@@ -66,25 +66,22 @@ const char *git_pager(int stdout_is_tty)
 static void setup_pager_env(struct argv_array *env)
 {
 	const char *pager_env = PAGER_ENV;
+	struct strbuf buf = STRBUF_INIT;
 
 	while (*pager_env) {
-		struct strbuf buf = STRBUF_INIT;
 		const char *cp = strchrnul(pager_env, '=');
 
 		if (!*cp)
 			die("malformed build-time PAGER_ENV");
 		strbuf_add(&buf, pager_env, cp - pager_env);
 		cp = strchrnul(pager_env, ' ');
-		if (!getenv(buf.buf)) {
-			strbuf_reset(&buf);
-			strbuf_add(&buf, pager_env, cp - pager_env);
-			argv_array_push(env, strbuf_detach(&buf, NULL));
-		}
+		if (!getenv(buf.buf))
+			argv_array_pushf(env, "%.*s",
+					 (int)(cp - pager_env), pager_env);
+		pager_env = cp + strspn(cp, " ");
 		strbuf_reset(&buf);
-		while (*cp && *cp == ' ')
-			cp++;
-		pager_env = cp;
 	}
+	strbuf_release(&buf);
 }
 
 void prepare_pager_args(struct child_process *pager_process, const char *pager)