| Message ID | 20230627195251.1973421-7-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce Git Standard Library | expand |
Calvin Wan <calvinwan@google.com> writes: > pager_in_use() is simply a wrapper around > git_env_bool("GIT_PAGER_IN_USE", 0). Other places that call > git_env_bool() in this fashion also do not have a wrapper function > around it. By removing pager_in_use(), we can also get rid of the > pager.h dependency from a few files. > > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > builtin/log.c | 2 +- > color.c | 2 +- > column.c | 2 +- > date.c | 4 ++-- > git.c | 2 +- > pager.c | 5 ----- > pager.h | 1 - > 7 files changed, 6 insertions(+), 12 deletions(-) With so many (read: more than 3) callsites, I am not sure if this is an improvement. pager_in_use() cannot be misspelt without getting noticed by compilers, but git_env_bool("GIT_PAGOR_IN_USE", 0) will go silently unnoticed. Is there no other way to lose the dependency you do not like?
> With so many (read: more than 3) callsites, I am not sure if this is > an improvement. pager_in_use() cannot be misspelt without getting > noticed by compilers, but git_env_bool("GIT_PAGOR_IN_USE", 0) will > go silently unnoticed. Is there no other way to lose the dependency > you do not like? I thought about only changing this call site, but that creates an inconsistency that shouldn't exist. The other way is to move this function into a different file, but it is also unclear to me which file that would be. It would be awkward in parse.c and if it was in environment.c then we would have many more inherited dependencies from that. I agree that the value of this patch is dubious in and of itself, which is why it's coupled together with this series rather than in a separate standalone cleanup series.
Junio C Hamano <gitster@pobox.com> writes: >> pager_in_use() is simply a wrapper around >> git_env_bool("GIT_PAGER_IN_USE", 0). Other places that call >> git_env_bool() in this fashion also do not have a wrapper function >> around it. By removing pager_in_use(), we can also get rid of the >> pager.h dependency from a few files. > > With so many (read: more than 3) callsites, I am not sure if this is > an improvement. pager_in_use() cannot be misspelt without getting > noticed by compilers, but git_env_bool("GIT_PAGOR_IN_USE", 0) will > go silently unnoticed. Is there no other way to lose the dependency > you do not like? Having the function isn't just nice for typo prevention - it's also a reasonable boundary around the pager subsystem. We could imagine a world where we wanted to track the pager status using a static var instead of an env var (not that we'd even want that :P), and this inlining makes that harder. From the cover letter, it seems like we only need this to remove "#include pager.h" from date.c, and that's only used in parse_date_format(). Could we add a is_pager/pager_in_use to that function and push the pager.h dependency upwards?
Glen Choo <chooglen@google.com> writes: > Could we add a is_pager/pager_in_use to that > function and push the pager.h dependency upwards? Bleh, I meant "Could we add a new is_pager/pager_in_use parameter to that function?"
> Glen Choo <chooglen@google.com> writes: > > > Could we add a is_pager/pager_in_use to that > > function and push the pager.h dependency upwards? > > Bleh, I meant "Could we add a new is_pager/pager_in_use parameter to > that function?" Refactoring the function signature to: parse_date_format(const char *format, struct date_mode *mode, int pager_in_use) as you suggested is a much better solution, thanks! I'll make that change in the next reroll.
Calvin Wan <calvinwan@google.com> writes: >> Glen Choo <chooglen@google.com> writes: >> >> > Could we add a is_pager/pager_in_use to that >> > function and push the pager.h dependency upwards? >> >> Bleh, I meant "Could we add a new is_pager/pager_in_use parameter to >> that function?" > > Refactoring the function signature to: > > parse_date_format(const char *format, struct date_mode *mode, int pager_in_use) > > as you suggested is a much better solution, thanks! I'll make that > change in the next reroll. Yeah, the date format "auto:" that changes behaviour between the output medium feels a serious layering violation, but given the constraints, it looks like the best thing to do. Thanks.
Glen Choo <chooglen@google.com> writes: > Having the function isn't just nice for typo prevention - it's also a > reasonable boundary around the pager subsystem. We could imagine a > world where we wanted to track the pager status using a static > var instead of an env var (not that we'd even want that :P), and this > inlining makes that harder. > > From the cover letter, it seems like we only need this to remove > "#include pager.h" from date.c, and that's only used in > parse_date_format(). Could we add a is_pager/pager_in_use to that > function and push the pager.h dependency upwards? Thanks---I think that may show a good direction. parse_date_format() reacts to "auto:foo" and as long as that feature needs to be there, pager_in_use() must be available to the function.
diff --git a/builtin/log.c b/builtin/log.c index 03954fb749..d5e979932f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -82,7 +82,7 @@ struct line_opt_callback_data { static int session_is_interactive(void) { - return isatty(1) || pager_in_use(); + return isatty(1) || git_env_bool("GIT_PAGER_IN_USE", 0); } static int auto_decoration_style(void) diff --git a/color.c b/color.c index f3c0a4659b..dd6f26b8db 100644 --- a/color.c +++ b/color.c @@ -388,7 +388,7 @@ static int check_auto_color(int fd) int *is_tty_p = fd == 1 ? &color_stdout_is_tty : &color_stderr_is_tty; if (*is_tty_p < 0) *is_tty_p = isatty(fd); - if (*is_tty_p || (fd == 1 && pager_in_use() && pager_use_color)) { + if (*is_tty_p || (fd == 1 && git_env_bool("GIT_PAGER_IN_USE", 0) && pager_use_color)) { if (!is_terminal_dumb()) return 1; } diff --git a/column.c b/column.c index ff2f0abf39..e15ca70f36 100644 --- a/column.c +++ b/column.c @@ -214,7 +214,7 @@ int finalize_colopts(unsigned int *colopts, int stdout_is_tty) if (stdout_is_tty < 0) stdout_is_tty = isatty(1); *colopts &= ~COL_ENABLE_MASK; - if (stdout_is_tty || pager_in_use()) + if (stdout_is_tty || git_env_bool("GIT_PAGER_IN_USE", 0)) *colopts |= COL_ENABLED; } return 0; diff --git a/date.c b/date.c index 619ada5b20..95c0f568ba 100644 --- a/date.c +++ b/date.c @@ -7,7 +7,7 @@ #include "git-compat-util.h" #include "date.h" #include "gettext.h" -#include "pager.h" +#include "parse.h" #include "strbuf.h" /* @@ -1009,7 +1009,7 @@ void parse_date_format(const char *format, struct date_mode *mode) /* "auto:foo" is "if tty/pager, then foo, otherwise normal" */ if (skip_prefix(format, "auto:", &p)) { - if (isatty(1) || pager_in_use()) + if (isatty(1) || git_env_bool("GIT_PAGER_IN_USE", 0)) format = p; else format = "default"; diff --git a/git.c b/git.c index eb69f4f997..3bfb673a4c 100644 --- a/git.c +++ b/git.c @@ -131,7 +131,7 @@ static void commit_pager_choice(void) void setup_auto_pager(const char *cmd, int def) { - if (use_pager != -1 || pager_in_use()) + if (use_pager != -1 || git_env_bool("GIT_PAGER_IN_USE", 0)) return; use_pager = check_pager_config(cmd); if (use_pager == -1) diff --git a/pager.c b/pager.c index 63055d0873..9b392622d2 100644 --- a/pager.c +++ b/pager.c @@ -149,11 +149,6 @@ void setup_pager(void) atexit(wait_for_pager_atexit); } -int pager_in_use(void) -{ - return git_env_bool("GIT_PAGER_IN_USE", 0); -} - /* * Return cached value (if set) or $COLUMNS environment variable (if * set and positive) or ioctl(1, TIOCGWINSZ).ws_col (if positive), diff --git a/pager.h b/pager.h index b77433026d..6832c6168d 100644 --- a/pager.h +++ b/pager.h @@ -5,7 +5,6 @@ struct child_process; const char *git_pager(int stdout_is_tty); void setup_pager(void); -int pager_in_use(void); int term_columns(void); void term_clear_line(void); int decimal_width(uintmax_t);
pager_in_use() is simply a wrapper around git_env_bool("GIT_PAGER_IN_USE", 0). Other places that call git_env_bool() in this fashion also do not have a wrapper function around it. By removing pager_in_use(), we can also get rid of the pager.h dependency from a few files. Signed-off-by: Calvin Wan <calvinwan@google.com> --- builtin/log.c | 2 +- color.c | 2 +- column.c | 2 +- date.c | 4 ++-- git.c | 2 +- pager.c | 5 ----- pager.h | 1 - 7 files changed, 6 insertions(+), 12 deletions(-)