Rubén Justo <rjusto@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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.
