Re: [RFC PATCH 6/8] pager: remove pager_in_use()

From: Glen Choo <chooglen@google.com>
Date: Jun 28, 2023, 00:30 UTC
Message-ID: <kl6lv8f8qvhd.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq1qhwfr46.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

>> 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?