Fix #1063: Do not use 'more' or 'most' PAGER, as they are not compatible with bats output

[Enselic]

Hi! Here is a proposal on how to solve #1063, including integration tests.

There is a warning printed to stderr with instructions on how to silence. It looks like this:

% PAGER=most bat tests/examples/test.txt
WARNING: Ignoring PAGER="most": Coloring not supported. Override with BAT_PAGER="most" or --pager "most"
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: tests/examples/test.txt
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ hello world
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────

the fix is a bit rough in the edges (for example, I just discovered it has some rust linter clippy warnings), but before I polish it, I just wanted to check what you think about the overall approach?

Looking forward to your comments!

Great project btw!

[sharkdp]

Thank you for your contribution.

Before I take a closer look .... more or most or both? 😄

[Enselic]

Thank you for your contribution.

The pleasure is mine!

Before I take a closer look .... more or most or both? 😄

Sorry about the confusion... Looks like the issue #1063 has the wrong title? That made me accidentally give this PR also the wrong title. But the code itself only deals with most. That's how we want it be, isn't it?

Anyway, I just pushed an update to this PR that is free of (relevant) lint warnings and without formatting issues, so a bit more polished than before. Also made use of the new and shiny bat_warning! macro. Looking forward to your comments, whatever they might be!

(Btw, seems like #1063 was accidentally marked as closed after merging my bat_warning! commit, so should probably be reopened until this PR lands)

[sharkdp]

Both pagers do exist: more and most. I can't make either of them properly display incoming ANSI escape sequences (like less -R does). So I guess we should ignore both of them.

[sharkdp]

After thinking about this a bit longer, I wonder if we should print a warning at all. If a user has set PAGER=most, do we really want to force them to set BAT_PAGER (or create a config file) to get rid of the warning, or would it be better to silently ignore any incompatible pagers (and try to use less)?

I think I would prefer the latter. @eth-p? @keith-hall?

[eth-p]

I would also prefer the latter.

Maybe as a happy medium between the two options, we could eventually add the warning message to the --diagnostics option?

[Enselic]

Both pagers do exist: more and most. I can't make either of them properly display incoming ANSI escape sequences (like less -R does). So I guess we should ignore both of them.

I'm on macOS, and here, more Is less in disguise:

% which more
/usr/bin/more
% more --version
less 487 (POSIX regular expressions)
Copyright (C) 1984-2016  Mark Nudelman
[...]

and I can my make my more display colors, like this:

bat --pager="more -R" README.md

so I'm not sure we should not allow more. We could add some extra logic to figure out if the more in use supports colors or not, but IMHO that is a lot of effort (with possible significant performance impacts, depending on how we do it) for a minor gain.

I would also prefer the latter.

Sounds good to me, I'll update the PR

Maybe as a happy medium between the two options, we could eventually add the warning message to the --diagnostics option?

Yes that is a good idea, but I'd say we can wait with that until we know for sure what behavior we want in the first place, so if it's OK with you I propose we keep that out-of-scope for this PR.

[sharkdp]

I'm on macOS, and here, more Is less in disguise:

Ah, I see. But in this case there's no advantage in allowing more to be used instead of less. I have the BSD version (Eric Shienbrood, UC Berkeley) of more on Arch Linux and I can not make it work properly with incoming ANSI escape sequences. It messes up the wrapping when used without any options and wrongly displays some escape sequences when used with the -f, --no-pause or -l, --logical options. I think "more" should be ignored as PAGER as well.

Maybe as a happy medium between the two options, we could eventually add the warning message to the --diagnostics option?

Yes that is a good idea, but I'd say we can wait with that until we know for sure what behavior we want in the first place, so if it's OK with you I propose we keep that out-of-scope for this PR.

👍

[Enselic]

But in this case there's no advantage in allowing more to be used instead of less.

Very good point, and I completely agree. I will update the PR accordingly.

[Enselic]

I updated the PR now after generalizing the code quite a bit, to get the code clean and nice (that was my intention at least). I did some sanity checks and I think the behavior is correct, including for edge cases like PAGER="" and PAGER="mismatched-quotes 'a".

Before I figure out a way to write integration tests for this change, I would like to check with you if you think the big-picture-code looks good, and if the behavior is good now?

(Make sure to review the changes with whitespace changes excluded due to indentation changes)

[Enselic]

I got some time over and wrote integration tests as well, using a strategy based on mocked versions of more and most. Turns out I need to tweak the test fixture some more to also run on Windows, but shouldn't be too hard.

The app code remains unchanged and is still ready for code review.

[Enselic]

After some additional hackery I got the tests working on Windows too.

So this PR is ready for full code review, and if Approved, can be merged.
Reminder: Enable 'Hide whitespace changes' when you review the code, for a nicer diff.

If you think my testing method that uses mocks is horrible, no worries, just let me know and we'll figure something out. I think the second-best option for testing this is to get info about the bat child process to see if it is less, more, or most. I looked briefly at it, and Rust didn't seem to have a mature API for that yet, but maybe I just need to look closer. But I'm worried it will require a lot of platform-specific code to use that strategy. It will also be messy to wait for the child process to start, for example. So what is in this PR is the best strategy I could come up with. If you have other ideas, I'm all ears.

[sharkdp]

Planning to take a look soon - thank you very much!

[Enselic]

Oh dear, some weird clippy error after I updated Cargo.lock.

Is it OK if I wait with sorting this out until you have done one round of CR?

[Enselic]

I understand you're a busy person, so if it would make life easier for you, I could split this PR up into smaller parts if you wish? I could for example move out the pager mocking testing support and some regression tests into a separate PR.

[sharkdp]

Is it OK if I wait with sorting this out until you have done one round of CR?

Of course.

I understand you're a busy person, so if it would make life easier for you, I could split this PR up into smaller parts if you wish? I could for example move out the pager mocking testing support and some regression tests into a separate PR.

Thank you. The PR is completely fine. I just need to invest some time to think through this again before I can properly review it.

That said, the diff is a bit hard to read as a lot of code has been moved. But I don't think that smaller PRs would really help there. That's something that I can manage 😄

[sharkdp]

Seems like you accidentally committed changes to assets/syntaxes/02_Extra/VimL. You probably need a git submodule update --init.

[Enselic]

I have updated the PR now and I think I took care of all comments.

I decided to wait with testing out the struct+Drop approach for mocked pagers and do it in a separate PR, because it would be nice to get this merged due to the relatively high risk of merge conflicts otherwise (I already had to take care of a couple).

As far as I know this PR can be merged now if Approved, but of course just let me know if you have more CR comments, no worries.

[sharkdp]

I decided to wait with testing out the struct+Drop approach for mocked pagers and do it in a separate PR, because it would be nice to get this merged due to the relatively high risk of merge conflicts otherwise (I already had to take care of a couple).

Sounds great. Only if you are interested. It's not really necessary. And as I said before, I'm not sure if that would actually be better.

I have updated the PR now and I think I took care of all comments.

Looks great! Thank you for the updates.