| Message ID | patch-v2-1.7-8eb1dfbff5d-20210906T070201Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remove dead & undocumented shell code | expand |
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. Best Wishes Phillip > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > git-sh-setup.sh | 16 ---------------- > t/t7006-pager.sh | 13 ------------- > 2 files changed, 29 deletions(-) > > diff --git a/git-sh-setup.sh b/git-sh-setup.sh > index 10d97641856..7ee4b0edff5 100644 > --- a/git-sh-setup.sh > +++ b/git-sh-setup.sh > @@ -157,22 +157,6 @@ git_editor() { > eval "$GIT_EDITOR" '"$@"' > } > > -git_pager() { > - if test -t 1 > - then > - GIT_PAGER=$(git var GIT_PAGER) > - else > - GIT_PAGER=cat > - fi > - for vardef in @@PAGER_ENV@@ > - do > - var=${vardef%%=*} > - eval ": \"\${$vardef}\" && export $var" > - done > - > - eval "$GIT_PAGER" '"$@"' > -} > - > sane_grep () { > GREP_OPTIONS= LC_ALL=C grep @@SANE_TEXT_GREP@@ "$@" > } > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh > index 0e7cf75435e..08f712a4497 100755 > --- a/t/t7006-pager.sh > +++ b/t/t7006-pager.sh > @@ -49,19 +49,6 @@ test_expect_success TTY 'LESS and LV envvars are set for pagination' ' > grep ^LV= pager-env.out > ' > > -test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' ' > - ( > - sane_unset LESS LV && > - PAGER="env >pager-env.out; wc" && > - export PAGER && > - PATH="$(git --exec-path):$PATH" && > - export PATH && > - test_terminal sh -c ". git-sh-setup && git_pager" > - ) && > - grep ^LESS= pager-env.out && > - grep ^LV= pager-env.out > -' > - > test_expect_success TTY 'some commands do not use a pager' ' > rm -f paginated.out && > test_terminal git rev-list HEAD && >
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, and their own invocation of "git rev-parse" for peel_committish().
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
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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> 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. The question we should be asking when we advocate to remove things is "is this really something we absolutely cannot live with?" But answering your question, if an out-of-tree user wants to behave just like Git, pretending that it would have been part of Git and the only reason why it is not is because it weren't invented here, yes, not forcing the end-user to pipe the tool's output to pager is something they would want to have a handy way to mimic, I would think. Thanks.
On Tue, Sep 07 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> 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. > > The question we should be asking when we advocate to remove things > is "is this really something we absolutely cannot live with?" > > But answering your question, if an out-of-tree user wants to behave > just like Git, pretending that it would have been part of Git and > the only reason why it is not is because it weren't invented here, > yes, not forcing the end-user to pipe the tool's output to pager is > something they would want to have a handy way to mimic, I would > think. I've made my preferences clear, but can live with whatever criteria we come up with. I am having trouble squaring the desire to keep git_pager() with the view you're describing, unless it's also an implicit endorsement of reverting a89a2fbfccd (parse-remote: remove this now-unused library, 2020-11-14). I'd obviously prefer to see git-parse-remote stay gone. But if we're worried about removing once-documented "git-sh-*" libraries from under users who peeked under the hood at some point to see & use functions within them, I'd think bringing back "git-parse-remote" would be more likely to help those users than having a git_pager(). And once we're rid of all our own use of these libraries but still want to ship them forever for such users, I'd think we'd want to bring some version of a revert of 49eb8d39c78 (Remove contrib/examples/*, 2018-03-25) back, i.e. just to make sure we don't break these going forward, as once our own use of them is removed they'll be completely untested in-tree. Anyway, as noted in <87eea0n04u.fsf@evledraar.gmail.com> I was hoping to take a small step towards finishing up removing the libintl dependency. But after this discussion I think I'm back to mentally classifying that as too tedious of a task to even try, so I wouldn't mind dropping this series of cleanups if we've landed on a consensus of keeping git-sh-setup bug-for-bug compatible going forward, and by extension git-sh-i18n.
diff --git a/git-sh-setup.sh b/git-sh-setup.sh index 10d97641856..7ee4b0edff5 100644 --- a/git-sh-setup.sh +++ b/git-sh-setup.sh @@ -157,22 +157,6 @@ git_editor() { eval "$GIT_EDITOR" '"$@"' } -git_pager() { - if test -t 1 - then - GIT_PAGER=$(git var GIT_PAGER) - else - GIT_PAGER=cat - fi - for vardef in @@PAGER_ENV@@ - do - var=${vardef%%=*} - eval ": \"\${$vardef}\" && export $var" - done - - eval "$GIT_PAGER" '"$@"' -} - sane_grep () { GREP_OPTIONS= LC_ALL=C grep @@SANE_TEXT_GREP@@ "$@" } diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 0e7cf75435e..08f712a4497 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -49,19 +49,6 @@ test_expect_success TTY 'LESS and LV envvars are set for pagination' ' grep ^LV= pager-env.out ' -test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' ' - ( - sane_unset LESS LV && - PAGER="env >pager-env.out; wc" && - export PAGER && - PATH="$(git --exec-path):$PATH" && - export PATH && - test_terminal sh -c ". git-sh-setup && git_pager" - ) && - grep ^LESS= pager-env.out && - grep ^LV= pager-env.out -' - test_expect_success TTY 'some commands do not use a pager' ' rm -f paginated.out && test_terminal git rev-list HEAD &&
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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- git-sh-setup.sh | 16 ---------------- t/t7006-pager.sh | 13 ------------- 2 files changed, 29 deletions(-)