Setting PAGER=less in the environment causes ANSI escape sequences to appear by default

[64]

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

[martinvonz]

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.

[jmr]

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?

[spectral54]

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:

1. user set $PAGER to less (or equivalent) for legacy reasons such as carry over from when more was 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 $LESS that conflicts with -F, -R, -X, or our expected use of less, but that's very uncommon AFAICT.

2. user set $PAGER to something other than less because they wanted to use a pager other than less, and we didn't respect it. User grumbles that we didn't respect their $PAGER. Likelihood: pretty low, setting $PAGER to something other than less seems pretty niche in my experience. UX penalty: relatively low, it's easy enough to set ui.pager=; in the meantime, user got the standard jj experience.

3. user set $PAGER to something other than less because less isn't available on their machine, and we didn't respect it. Likelihood: very low, less is available on essentially all non-Windows machines by default as far as I know. UX penalty: we emit an error message when trying to execute less, 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:

1. 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 about ui.pager and can't even run jj help to get assistance.
2. set $LESS (iff $LESS isn't already set). This is what git does and it's awkward because there's no guarantee that R is in there, or F and X are either both in there or both NOT in there. Likelihood: setting LESS seems less common than setting PAGER=less. So this would probably work. It also has the effect of carrying over configuration that worked from git. UX penalty: if the user DOES have LESS set, then this can lead to awkward interactions; I asked a friend about some of this and when I told them what git does, they finally understood why they had to adjust their LESS environment variable to make git usable. So even users that set $LESS don't necessarily want it to apply when run by git or jj.
3. merge FRX into $LESS. UX penalty: somewhat less than the previous option, but it feels really weird to do this kind of manipulation
4. if $PAGER is 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 to jj might 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.
5. if $PAGER is 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.

[PhilipMetzger]

I think what you propose is similar to what Martin does with #3657, it probably just needs someone to carry it over the line.

[spectral54]

I was going for simpler, and just saying we should delete these three lines:

jj/cli/src/config.rs

Lines 673 to 675 in f4b5460

	if let Ok(value) = env::var("PAGER") {
	layer.set_value("ui.pager", value).unwrap();
	}

.

[PhilipMetzger]

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.

[spectral54]

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.

[PhilipMetzger]

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 man to use less instead of more a 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.

[jplatte]

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 🤷🏼