Re: [PATCH v4 4/6] pager: introduce setup_custom_pager

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> Introduce a new function setup_custom_pager() to allow setting up our
> pager mechanism using a custom pager.  If the custom pager specified is
> NULL or an empty string, use the normal pager as setup_pager() currently
> does.

We often see "if the pointer is NULL or points at an empty string"
in code that were originally ported from the scripted Porcelain, but
I doubt we would want to follow that pattern in new code paths.

>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  pager.c | 17 +++++++++++------
>  pager.h |  6 +++++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/pager.c b/pager.c
> index 925f860335..21a7d9cd60 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -74,14 +74,13 @@ static int core_pager_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> -const char *git_pager(int stdout_is_tty)
> +static const char *git_pager_custom(int stdout_is_tty, const char* pager)

Call it git_custom_pager(), to be more grammatical and also match
the other one, setup_custom_pager().

The asterisk should stick to "pager", not its type.

>  {
> -	const char *pager;
> -
>  	if (!stdout_is_tty)
>  		return NULL;
>  
> -	pager = getenv("GIT_PAGER");
> +	if (!pager || !*pager)
> +		pager = getenv("GIT_PAGER");

We often see "if the pointer is NULL or points at an empty string"
in code that were originally ported from the scripted Porcelain, but
I doubt we would want to follow that pattern in new code paths.

> @@ -97,6 +96,11 @@ const char *git_pager(int stdout_is_tty)
>  	return pager;
>  }
>  
> +const char *git_pager(int stdout_is_tty)
> +{
> +	return git_pager_custom(stdout_is_tty, NULL);
> +}

OK.

> @@ -132,10 +136,11 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>  	pager_process->trace2_child_class = "pager";
>  }
>  
> -void setup_pager(void)
> +void setup_custom_pager(const char* pager)

The asterisk should stick to "pager", not its type.

>  {
>  	static int once = 0;
> -	const char *pager = git_pager(isatty(1));
> +
> +	pager = git_pager_custom(isatty(1), pager);

This feels a bit too convoluted.  This thing already knows if it got
a custom one from its caller because "*pager" is _its_ parameter.
Perhaps rip out all the changes before and including the hunk
"@@ -132,10 +136,11 @@" and start it like so, perhaps?

	void setup_custom_pager(const char *pager)
	{
		if (!pager)
			pager = git_pager(isatty(1));
		...

> diff --git a/pager.h b/pager.h
> index 103ecac476..2166662361 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -4,7 +4,11 @@
>  struct child_process;
>  
>  const char *git_pager(int stdout_is_tty);
> -void setup_pager(void);
> +void setup_custom_pager(const char*);
> +static inline void setup_pager(void)
> +{
> +	setup_custom_pager(NULL);
> +}

A good approach to help existing callers---there are more than half
a dozen existing callers to setup_pager() in the code base.  We
could migrate them all away to pass NULL but it would be totally
outside the scope of this topic.