Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

environment.nix: Split "less -R" into PAGER and LESS #110960

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

environment.nix: Split "less -R" into PAGER and LESS #110960

wants to merge 1 commit into from

Conversation

toonn
Copy link
Contributor

@toonn toonn commented Jan 27, 2021

Setting PAGER="less -R" relies on word splitting to work out properly.
Less also reads options from from a variable named LESS, by setting
PAGER="less" and LESS="-R" we don't have to rely on word splitting.

Motivation for this change

The PAGER="less -R" setting has always worked fine for me until today. I was trying out a bash script for note-taking and it errored with "command not found: less -R". By setting the option in LESS="-R" instead we can avoid relying on word splitting. I believe this should only make it work in more cases not less.

One backwards incompatibility issue is that people who don't like the -R flag and have overridden PAGER with less would have to clear the setting for LESS.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@toonn
environment.nix: Split "less -R" into PAGER and LESS
10e2d7f 
Setting `PAGER="less -R"` relies on word splitting to work out properly.
Less also reads options from from a variable named `LESS`, by setting
`PAGER="less"` and `LESS="-R"` we don't have to rely on word splitting.
@toonn toonn mentioned this pull request Jan 27, 2021
Open
@lukegb
Copy link
Contributor

lukegb commented May 3, 2021

Thinking about the constellation of possible settings:

PAGER unset PAGER set
LESS unset No change Tools which directly invoke less will now pick up -R
LESS set Things using PAGER will no longer pass -R, if they want the behaviour they'll have to add it to LESS themselves No change

In any case, I think this is probably low impact beyond mild annoyance.

@deviant
Copy link
Member

deviant commented May 3, 2021

While I don't have a particularly strong opinion on this, my understanding is that EDITOR/PAGER/etc are conventionally word-split to allow for arguments to be passed like this. While I can't find a useful source, there are numerous bug reports for software that doesn't do this. Given that these are usually fixed, I think it's safe to assume that splitting is the expected behaviour. I've listed a few examples below.

sudoedit(1)'s handling of EDITOR:

v@january ~> EDITOR='echo hi' sudoedit /etc/passwd
[sudo] password for v:
hi /var/tmp/passwd.XXxBDZbR
sudoedit: /etc/passwd unchanged

From man(1):

       MANPAGER, PAGER
              If  $MANPAGER  or  $PAGER is set ($MANPAGER is used in preference), its
              value is used as the name of the program used  to  display  the  manual
              page.   By  default,  less  is used, falling back to cat if less is not
              found or is not executable.

              The value may be a simple command name or a command with arguments, and
              may  use  shell quoting (backslashes, single quotes, or double quotes).
              It may not use pipes to connect multiple commands; if  you  need  that,
              use  a  wrapper script, which may take the file to display either as an
              argument or on standard input.

@deviant
Copy link
Member

deviant commented May 3, 2021

To be clear, I'm not against this change (although I'll have to override LESS after this, since I don't like -R as default behaviour). This might be a bug you should file with the author(s) of nb, however.

@deviant
Copy link
Member

deviant commented May 3, 2021

Looks like this has already been fixed for EDITOR: xwmx/nb#15

@toonn
Copy link
Contributor Author

toonn commented May 3, 2021

I was told in the #bash channel that relying on word-splitting causes no end of trouble. Hence the solution here. Note that I did at first intend to simply fix nb but was discouraged by the people who frequent that channel (I assumed they know what they're talking about).

Doesn't eval do parameter expansion? So now you might end up with a file path that is no longer properly quoted?

@stale
Copy link

stale bot commented Nov 9, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 9, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 7, 2023
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank marked this pull request as draft March 20, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bqv bqv bqv approved these changes

@teto teto teto approved these changes

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 12.approvals: 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@toonn @lukegb @deviant @bqv @teto @wegank