Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager
From: Jeff King <hidden>
Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 03:06:56PM -0800, Junio C Hamano wrote:
quoted hunk ↗ jump to hunk
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