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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 7, 2021, 10:22 UTC
Message-ID: <87eea0n04u.fsf@evledraar.gmail.com>
In-Reply-To: <8912317f-7eb0-edae-29e3-2e05099bc696@gmail.com>

On Tue, Sep 07 2021, Phillip Wood wrote:

Show 36 quoted lines

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