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

[Phillip Wood <phillip.wood123@gmail.com>]

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 and they 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

> 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 :/

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

Best Wishes

Phillip