Do not parse arguments in PAGER

[majecty]

This fixes #430

[majecty]

@sharkdp I am struggling with fixing the test case pager_basic.
Could you give me some hints?

bat/tests/integration_tests.rs

Line 313 in b68c5d8

fn pager_basic() {

[majecty]

I tried by creating pager_output.sh file, but the windows test failed. :(

[sharkdp]

Thank you very much for looking into this!

Maybe we could do the following (this would also fix the echo-based tests, I think):

• Split the PAGER environment variable into arguments with shell_words::split.
• Check if the (basename of) the first component is less
• If yes, call the given version of less with our fixed set of arguments (-FRX).
• If no, call the pager as specified with all of its arguments.

This way, if someone wants to use my_special_pager with a given set of command-line arguments, it can still be done via PAGER.

[majecty]

Thanks. I'll try it.

[sharkdp]

The whole point of this was to have a different handling of PAGER and BAT_PAGER. We do not want to modify anything for BAT_PAGER.

[majecty]

env::var("BAT_PAGER") is not modified. Only env::var("pager") is mapped by the function add_default_flags_to_less

If I am misunderstanding your comment, please say freely about it.

[sharkdp]

This does not work if the first argument is /usr/bin/less, for example. See the is_less variable example in the same file.

[majecty]

Thanks. I missed it. I'll fix it now!

[majecty]

When I run the same test in my repository, the test succeeded. I don't know why the test failed. Could you help me?
@sharkdp

[majecty]

@sharkdp Thanks! the test passed. Could you review this PR again?

[sharkdp]

@sharkdp Thanks! the test passed. Could you review this PR again?

Yes, sorry for the delay. I'll do a final review soon.

[sharkdp]

I don't really like the duplication of the logic here. We already have a piece of code that adds the default arguments in a specific way:

bat/src/output.rs

Lines 52 to 57 in abf0229

	if args.is_empty() {
	p.args(vec!["--RAW-CONTROL-CHARS", "--no-init"]);
	if quit_if_one_screen {
	p.arg("--quit-if-one-screen");
	}
	}

We should strive to reuse that part of the code.

[majecty]

@sharkdp Sorry for late.

Then how about this?

        let pager_from_env = match (env::var("BAT_PAGER"), env::var("PAGER")) {
            (Ok(bat_pager), _) => Some(bat_pager),
            (_, Ok(pager)) => {
                if PathBuf::from(pager.clone()).file_stem() == Some(&OsString::from("less")) {
                    Some(String::from("less"))
                } else {
                    Some(pager)
                }
            }
            _ => None,
        };

If the pager_from_env value is set to "less", the default arguments will be added later.

[sharkdp]

Then how about this?

This wouldn't work if PAGER="less -F", for example, because we need to split off the argument first, before checking the file_stem. I have added an implementation which should hopefully work. It is an adapted version of your latest proposal.

In any case, thank you very much for your work and your patience.