Re: [PATCH v2 1/7] git-sh-setup: remove unused git_pager() function

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Sep 07 2021, Phillip Wood wrote:

> On 06/09/2021 23:27, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Sep 06 2021, Phillip Wood wrote:
>> 
>>> Hi Ævar
>>>
>>> On 06/09/2021 08:05, Ævar Arnfjörð Bjarmason wrote:
>>>> Remove the git_pager() function last referenced by non-test code in
>>>> 49eb8d39c78 (Remove contrib/examples/*, 2018-03-25).
>>>> We can also remove the test for this added in 995bc22d7f8 (pager:
>>>> move
>>>> pager-specific setup into the build, 2016-08-04), the test that
>>>> actually matters is the one added in e54c1f2d253 (pager: set LV=-c
>>>> alongside LESS=FRSX, 2014-01-06) just above the removed test.
>>>> I.e. we don't care if the "LESS" and "LV" variables are set by
>>>> git-sh-setup anymore, no built-in uses them, we do care that pager.c
>>>> sets them, which we still test for.
>>>
>>> git_pager() might not be documented but I think it is useful for
>>> script authors and I wouldn't be surprised if someone out there is
>>> using it. The same goes for peel_committish(). It does not seem like a
>>> huge maintenance burden to keep and maybe document these two
>>> functions.
>> The git_pager() and peel_committish() seem to thoroughly be in the
>> same
>> camp as the now-removed git-parse-remote.sh (see a89a2fbfccd
>> (parse-remote: remove this now-unused library, 2020-11-14)) and say its
>> get_remote_merge_branch(). I.e. we carried it for a while, but the
>> function was never publicly documented.
>> I think rather than document these it makes sense to just kick that
>> maintenance burden over to whoever decided they'd rely on undocumented
>> shellscript functions git was shipping.
>> In these cases they can rather easily use the documented GIT_PAGER
>> environment variable directly, 
>
> No, they need to know to call 'git var GIT_PAGER' rather than using
> the environment variable directly to pick up core.pager[...]

Sorry, I should have said "...directly via git var GIT_PAGER". I also
see that we could improve some of the doc cross-referencing here,
i.e. "git help git") doesn't make this explicit or point to "git var",
but we cover this in "git help var" itself.

> [...]should be checking whether stdout is a tty. That is why this function
> existed and we didn't just check the value of GIT_PAGER in our scripts

For a hypothetical out-of-tree user is this really something anyone
strictly needs? It's just an optimization. If you don't do it you'll
just use your pager to pipe output to a non-tty.

>> and their own invocation of "git rev-parse" for peel_committish().
>
> The reason the function exists is that you cannot just call 'git
> rev-parse $OID^{commit}' if $OID starts with :/

Sure, but is the answer to that & the pager case above that we need to
support an always-undocumented function that someone could only have
found via source spelunking, as opposed to them maintaining that case,
or submitting a patch to "git rev-parse" to make their use-case easier?

> I'm not sure what the maintenance burden of keeping these functions is
> that makes it worth removing them

I was hoping that we could head towards just entirely removing
git-sh-setup in the near-ish future, but per the evolution of this
series it seems that we've got out-of-tree users for its *documented*
functions, so we can't simply do that.

I would like to be able to rip out the shellscript support for i18n
sooner than later, I think a way to get there would be to only emit
strings in the C locale from these remaining git-sh-setup functions, and
eventually move that script to live in contrib/ or something (and be
able to install it). I.e. make it a backwards-compatability-only
interface.

So yes, the maintenance burden of any two functions being removed here
is trivial and we could just keep them around.

I'm pursuing this because I'd really like to get clarity on where
exactly we're drawing the line with this git-sh-setup interface, since
we can anticipate a near-future where our own remaining user won't use
it anymore (or the 1-2 things they do can be moved to
git-filter-branch.sh or whatever).

The burden of that *is* non-trivial. I.e. us having to project gettext
to shellscript land, which in turn is something that's kept me from
improving git's i18n interface, i.e. provide some light alternative to
it that won't require libintl, as long as I've got to support that sort
of thing in shellscript land...

Do you have out-of-tree uses of git_pager(), or do we know about anyone
who does? I understand the argument that we've documented certain things
in git-sh-setup for years, but needing to support what's amounted to an
undocumented internal implementation detail seems like it's going too
far.