Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager

From: Jeff King <peff@peff.net>
Date: Feb 16, 2016, 23:26 UTC
Message-ID: <20160216232657.GB18634@sigill.intra.peff.net>
In-Reply-To: <1455664017-27588-3-git-send-email-gitster@pobox.com>

On Tue, Feb 16, 2016 at 03:06:56PM -0800, Junio C Hamano wrote:

Show 24 quoted lines

> diff --git a/pager.c b/pager.c
> index 5dbcc5a..1406370 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -53,6 +53,23 @@ const char *git_pager(int stdout_is_tty)
>  	return pager;
>  }
>  
> +void prepare_pager_args(struct child_process *pager_process, ...)
> +{
> +	va_list ap;
> +	const char *arg;
> +
> +	va_start(ap, pager_process);
> +	while ((arg = va_arg(ap, const char *)))
> +		argv_array_push(&pager_process->args, arg);
> +	va_end(ap);
> +
> +	pager_process->use_shell = 1;
> +	if (!getenv("LESS"))
> +		argv_array_push(&pager_process->env_array, "LESS=FRX");
> +	if (!getenv("LV"))
> +		argv_array_push(&pager_process->env_array, "LV=-c");
> +}

When reading this, I had to wonder what the "..." args were supposed to be. I figured it out when I read the caller, but I wonder if a comment would help. Also, we are expecting the pager here as the first argument, so maybe:

  void prepare_pager_args(struct child_process *pager_process,
                          const char *pager, ...);

would be a better signature. That also made me wonder if we could simply get away with:

  void prepare_pager_args(struct child_process *pager_process,
                          const char *pager);

and have callers argv_array_push() themselves afterwards.

And if you put the git_pager() call inside prepare_pager_args (which I agree would be cleaner), we just have:

  void prepare_pager_args(struct child_process *pager_process);

which is pretty self-explanatory (though it might need a new name; I'd be tempted to call it init_pager_process() or something, and actually have it do the child_process_init() to make sure it is working with a sane clean slate).

-Peff