Re: [PATCH v4 4/6] pager: introduce setup_custom_pager
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 4, 2024, 16:43 UTC
- Message-ID
- <xmqq8qzkzmjr.fsf@gitster.g>
- In-Reply-To
- <3fa0ade6-d710-47ce-8bcd-81d7821dfffb@gmail.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.
Show 17 quoted lines
> > 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.
Show 9 quoted lines
> {
> - 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.
Show 8 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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.
Show 5 quoted lines
> {
> 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));
...Show 14 quoted lines
> 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.