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

[Jeff King <peff@peff.net>]

On Wed, Aug 03, 2016 at 01:57:09PM -0700, Junio C Hamano wrote:

> 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.

Sounds good.

> 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);

I thought you'd need a strbuf_reset() here, but I guess it is handled by
the one at the end of the loop. That's OK because there are no other
loop exits, though IMHO putting it at the top makes the reusable-strbuf
idiom easier to understand.

-Peff