Setting PAGER=less in the environment causes ANSI escape sequences to appear by default
Just tinkering with jj locally for the first time. I set PAGER=less in my environment. This seems to override the default pager 'less -FRX' and hence ANSI escape sequences are not rendered properly (since -FRX is not passed).
Steps to Reproduce the Problem
Run PAGER=less jj --help with a clean configuration
Expected Behavior
Help is rendered either in plain text or with escape sequences processed properly
Actual Behavior
ANSI escape sequences are shown:
$ PAGER=less jj --help
Jujutsu (An experimental VCS)
To get started, see the tutorial at https://github.com/martinvonz/jj/blob/main/docs/tutorial.md.
ESC[1mESC[4mUsage:ESC[0m ESC[1mjjESC[0m [OPTIONS] <COMMAND>
[...]
Note that env -u PAGER jj --help works as intended.
Specifications
- Platform: Linux
- Version: 0.16.0
If the color config is set to "auto" (the default), I suppose we could check if the pager is set to less without -R and disable color if it is. Or we can always set LESS=FRX when calling the pager. #2928 attempted that but was abandoned in favor of adding a builtin pager to use by default. Maybe we should revive that PR.
I was also surprised by this behavior, but I switched to the built-in pager, so it's no longer bothering me.
I suppose we could check if the pager is set to less without -R and disable color if it is
What if $PAGER == more or something else that doesn't support color?
Or we can always set
LESS=FRX
Seems not great. Users might have $LESS set to something and get annoyed its being overridden.
The ui.color docs say, "auto will use color only when writing to a terminal." Should $PAGER be considered not-a-terminal?
I believe every case we've encountered at $dayjob where this has come up has had PAGER=less (or equivalent like /usr/bin/less). Gut feeling is that 1-3% (let's call it 2%) of users have stumbled across this. Caveats: We've only deployed to Linux and only to a small number of users so far, and there's probably some sort of self-selection where if a user is using a non-less pager, they're probably aware of it and know what to do to make it work.
With that in mind, I'm going to suggest that we just ignore $PAGER and use less -FRX by default (except on Windows, where we'd use :builtin by default). If the user wants something else, they can specify it in ui.pager. The main argument for this is that there's three failure cases if we do it this way:
-
user set
$PAGERtoless(or equivalent) for legacy reasons such as carry over from whenmorewas the default. Likelihood: most common case of the three that I can tell. UX penalty: not really any. Since we wouldn't be unsetting$LESS, it seems like there's the possibility that users would have a$LESSthat conflicts with-F,-R,-X, or our expected use ofless, but that's very uncommon AFAICT. -
user set
$PAGERto something other thanlessbecause they wanted to use a pager other thanless, and we didn't respect it. User grumbles that we didn't respect their$PAGER. Likelihood: pretty low, setting$PAGERto something other thanlessseems pretty niche in my experience. UX penalty: relatively low, it's easy enough to set ui.pager=; in the meantime, user got the standard jjexperience. -
user set
$PAGERto something other thanlessbecause less isn't available on their machine, and we didn't respect it. Likelihood: very low,lessis available on essentially all non-Windows machines by default as far as I know. UX penalty: we emit an error message when trying to executeless, and user doesn't get paginated output:Warning: Failed to spawn pager 'less': No such file or directory (os error 2) Hint: Consider using the `:builtin` pager.
The alternatives to ignoring $PAGER would be:
- do nothing, keep what we have. Likelihood: kinda common. UX penalty: HIGH. We spit color codes at the user but less doesn't do anything except translate the ESC code to
^[visual sequences. The tool is essentially unusable in this state. Users don't know aboutui.pagerand can't even runjj helpto get assistance. - set $LESS (iff $LESS isn't already set). This is what git does and it's awkward because there's no guarantee that
Ris in there, orFandXare either both in there or both NOT in there. Likelihood: settingLESSseems less common than settingPAGER=less. So this would probably work. It also has the effect of carrying over configuration that worked fromgit. UX penalty: if the user DOES haveLESSset, then this can lead to awkward interactions; I asked a friend about some of this and when I told them whatgitdoes, they finally understood why they had to adjust theirLESSenvironment variable to make git usable. So even users that set$LESSdon't necessarily want it to apply when run bygitorjj. - merge FRX into
$LESS. UX penalty: somewhat less than the previous option, but it feels really weird to do this kind of manipulation - if
$PAGERis set, don't paginate by default at all. I realize that's counterintuitive, but it would at least be a viable user experience --ls, for example, doesn't paginate by default, so users new tojjmight not necessarily expect it to paginate by default (except that's what git does, so they probably will expect it). UX penalty: with the default revset for log, this can be high. The content is readable but probably scrolled off the screen way too fast. - if
$PAGERis set, disable color by default and require setting a config option to enable color. UX penalty: I personally love color so this would likely be a show stopper for me if my default interactions weren't colorized; I'd assume that it doesn't support color.
We could theoretically detect these cases and emit a warning message, telling users to set ui.pager to their preferred setup to make the warning go away. The question then would be: what do we do after emitting that message, and my assumption would be what I proposed: run less -FRX. So whether we show a warning or not seems like it's an orthogonal question.
I think what you propose is similar to what Martin does with https://github.com/jj-vcs/jj/pull/3657, it probably just needs someone to carry it over the line.
I was going for simpler, and just saying we should delete these three lines: https://github.com/jj-vcs/jj/blob/f4b5460a09a603aac1035ca7f853c4a3c93e0cb5/cli/src/config.rs#L673-L675.
First off I like how simple this change is and secondly it breaks a bunch of assumptions around $PAGER behavior for CLI tools so it will be unexpected and may break many users which is also a great from a stability perspective but that imho is far worth the improvement.
There's not that many tools that invoke $PAGER that I was able to think of: man, git, hg, jj. Debian's /usr/bin/sensible-pager. Is there really anything else that's commonly using $PAGER? So then the question becomes: if so few things are using $PAGER, why do people set it?
My heavily biased data from $dayjob implies that it's some carry over / cargo culting from trying to get man to use less instead of more a long time ago.
So then the question becomes: if so few things are using
$PAGER, why do people set it?
Probably cargo-culting it from other CLI tools, without ever reconsidering.
My heavily biased data from $dayjob implies that it's some carry over / cargo culting from trying to get
manto uselessinstead ofmorea long time ago.
I mean if you look at the pager issues in the Discord which have popped up during all these years, the same issue is a heavy factor so entirely removing this bad path just is good.
So then the question becomes: if so few things are using
$PAGER, why do people set it?
I subscribed to this issue after having the same problem described at the top, and I checked. For me, it's because I use the GRML ZSH config. If PAGER isn't set, it sets it to less. The zshrc file has no comment on why though 🤷🏼
Commenting to share that I ran into this, and that I found I had PAGER=less setup on my alpine system in /etc/profile.
Ultimately I believe this came from busybox, and in turn build root. Here I can see there is a commit where they decided to remove the PAGER= default: https://github.com/buildroot/buildroot/commit/40322675073dc17590c4d2dce067ca3f1fee96ad