| Message ID | 3fa0ade6-d710-47ce-8bcd-81d7821dfffb@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use the pager in 'add -p' | expand |
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.
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) { - const char *pager; - if (!stdout_is_tty) return NULL; - pager = getenv("GIT_PAGER"); + if (!pager || !*pager) + pager = getenv("GIT_PAGER"); if (!pager) { if (!pager_program) read_early_config(core_pager_config, NULL); @@ -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); +} + static void setup_pager_env(struct strvec *env) { const char **argv; @@ -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) { static int once = 0; - const char *pager = git_pager(isatty(1)); + + pager = git_pager_custom(isatty(1), pager); if (!pager) return; 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); +} void wait_for_pager(void); int pager_in_use(void); int term_columns(void);
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. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- pager.c | 17 +++++++++++------ pager.h | 6 +++++- 2 files changed, 16 insertions(+), 7 deletions(-)