Re: [PATCH v2 1/7] git-sh-setup: remove unused git_pager() function
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 7, 2021, 09:41 UTC
- Message-ID
- <8912317f-7eb0-edae-29e3-2e05099bc696@gmail.com>
- In-Reply-To
- <87czplnxn3.fsf@evledraar.gmail.com>
On 06/09/2021 23:27, Ævar Arnfjörð Bjarmason wrote:
Show 35 quoted lines
> > 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