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

On 06/09/2021 23:27, Ævar Arnfjörð Bjarmason wrote:

On Mon, Sep 06 2021, Phillip Wood wrote:

quoted

Hi Ævar

On 06/09/2021 08:05, Ævar Arnfjörð Bjarmason wrote:

quoted

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