cli: ignore $PAGER in environment
#7821
Conversation
When `$PAGER` is set in the environment, jj uses that instead of the default (`:builtin` on Windows, `less -FRX` everywhere else). Commonly, users will have `PAGER=less` in their environment for various reasons, and this is respected by jj. This means that every jj command, even one that only outputs one or two lines, will still invoke a full screen pager. It also means that every jj command which uses escape sequences for color, which is most of them, will be output through a pager that doesn't handle that well, so users see output that looks like this, which isn't very readable: ``` ESC[1mESC[38;5;2m@ESC[0m ESC[1mESC[38;5;13mspmESC[38;5;8mwzlkq... ``` To fix this issue that new users stumble upon, ignore `$PAGER` from the environment, and always use our per-platform default. Users can override this and select whatever pager they want by setting ui.pager in their jj config. This seems like the least bad option for resolving jj-vcs#3502. The cases I considered were: 1. User doesn't have `PAGER` set. No change. 2. User has `PAGER=less` in their environment. We'll still run `less`, just with `-FRX`, so this seems fine. This case is surprisingly common. 3. User has `PAGER` set because they prefer another pager. We'll ignore that preference and run `less -FRX`. 4. User has `PAGER` set because `less` isn't available on their platform. This is uncommon except for Windows, where we'll run `:builtin` instead of `less -FRX` by default anyway. This may cause some users who have intentionally set and configured `PAGER` to be frustrated that we aren't respecting that value, but it's generally not possible to respect that value in all cases _and_ have a consistent and usable experience out of the box for the majority of users. #### Alternatives considered 1. Disable color and OSC8 hyperlinks if `PAGER` is set, since we can't be sure the pager supports the color codes. 2. Don't paginate by default if `PAGER` is set. This seems counterintuitive, but would at least resolve the problem. Users would assume that the `jj` CLI doesn't support paginating, and either wrap it in a pager themselves (this is a bad outcome) or find `ui.pager` and change the setting. 3. Set `LESS` (iff it's not set already), then invoke `PAGER`. This means that users setting things like `LESS=i` breaks our output as well, and cases where `PAGER` isn't 'less' aren't fixed. Fixes jj-vcs#3502
| } else if let Ok(value) = env::var("EDITOR") { | ||
| layer.set_value("ui.editor", value).unwrap(); | ||
| } | ||
| // [#3502] Intentionally NOT respecting $PAGER here. |
There was a problem hiding this comment.
Need to update at least config.md
There was a problem hiding this comment.
Updated config.md.
| - `diff.format` | ||
|
|
||
| * `jj` now ignores `$PAGER` set in the environment. To override the default | ||
| pager, set this via the `ui.pager` config instead. |
There was a problem hiding this comment.
Do you think it's useful to suggest running jj config set --user ui.pager "$PAGER" here for users who have the variable set?
There was a problem hiding this comment.
I think here we can (I'd be wary about doing this in a deprecation warning because it'll likely be run blindly and perpetuate problems). We could also theoretically suggest something like:
jj config set --user ui.pager '["sh", "-c", "$PAGER"]'
to make jj respect the $PAGER value as it is in the environment when executing jj (instead of locking it in). I'm really reluctant to suggeset this because it relies on sh and I really don't think this is going to come up often at all :)
| if let Ok(value) = env::var("PAGER") { | ||
| layer.set_value("ui.pager", value).unwrap(); | ||
| } |
There was a problem hiding this comment.
nit: I think we can sensibly deprecate this in 3 releases, so currently adding a warning here may be the option before straight up removing it. I'm up making the removal processes shorter if you deem this to hard for Google.
| } else if let Ok(value) = env::var("EDITOR") { | ||
| layer.set_value("ui.editor", value).unwrap(); | ||
| } | ||
| // [#3502] Intentionally NOT respecting $PAGER here. |
There was a problem hiding this comment.
nit: Move the comment to the place where you removed the code and explain the decision a bit more.
3 participants
When
$PAGERis set in the environment, jj uses that instead of the default (:builtinon Windows,less -FRXeverywhere else). Commonly, users will havePAGER=lessin their environment for various reasons, and this is respected by jj. This means that every jj command, even one that only outputs one or two lines, will still invoke a full screen pager. It also means that every jj command which uses escape sequences for color, which is most of them, will be output through a pager that doesn't handle that well, so users see output that looks like this, which isn't very readable:To fix this issue that new users stumble upon, ignore
$PAGERfrom the environment, and always use our per-platform default. Users can override this and select whatever pager they want by setting ui.pager in their jj config.This seems like the least bad option for resolving #3502. The cases I considered were:
PAGERset. No change.PAGER=lessin their environment. We'll still runless, just with-FRX, so this seems fine. This case is surprisingly common.PAGERset because they prefer another pager. We'll ignore that preference and runless -FRX.PAGERset becauselessisn't available on their platform. This is uncommon except for Windows, where we'll run:builtininstead ofless -FRXby default anyway.This may cause some users who have intentionally set and configured
PAGERto be frustrated that we aren't respecting that value, but it's generally not possible to respect that value in all cases and have a consistent and usable experience out of the box for the majority of users.Alternatives considered
PAGERis set, since we can't be sure the pager supports the color codes.PAGERis set. This seems counterintuitive, but would at least resolve the problem. Users would assume that thejjCLI doesn't support paginating, and either wrap it in a pager themselves (this is a bad outcome) or findui.pagerand change the setting.LESS(iff it's not set already), then invokePAGER. This means that users setting things likeLESS=ibreaks our output as well, and cases wherePAGERisn't 'less' aren't fixed.Fixes #3502
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)