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.

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

bat leaves text behind in pager mode #749

Closed
dickshaydle opened this issue Nov 12, 2019 · 14 comments · Fixed by #786
Closed

bat leaves text behind in pager mode #749

dickshaydle opened this issue Nov 12, 2019 · 14 comments · Fixed by #786
Labels
feature-request New feature or request good first issue Good for newcomers

Comments

@dickshaydle
Copy link

version: bat 0.12.1

Bat leaves the text behind if it is started like
bat somefile or bat somefile --pager='less'

if i start it like
bat somefile --pager='less -r'
it works fine and leaves nothing behind as less does.

@eth-p
Copy link
Collaborator

eth-p commented Nov 13, 2019

This would be because of the way that bat provides --quit-if-one-screen to less, unless the pager is specified (via BAT_PAGER, --pager, etc.) with arguments.

If you prefer it always pages, you can add export BAT_PAGER='less -R' to your .bash_profile script.

@dickshaydle
Copy link
Author

Thanks for your answer.

Shouldn't this be the default behavior?
I mean, the default looks kind of buggy on my computer. When i scroll up and down then there are some lines printed multiple times. When i scroll a lot, then the terminal buffer gets spammed with previous bat screens.
I can't imagine it is meant to look like that.

@eth-p
Copy link
Collaborator

eth-p commented Nov 14, 2019

Oh, that sounds like a different issue than I first thought. If the pager isn't printing and exiting immediately, then we're talking about the --no-init flag being automatically passed to less.

With that flag, less is told to not swap to the alternate screen. Without the alternate screen, scrolling the pager contents will cause redraw artifacts to appear in the scrollback. If that's the case, you can set BAT_PAGER to less -R --quit-if-one-screen to avoid all those issues.

The reason --no-init was added as a default is because some distros (and the latest version of macOS) ship with an old version of less that would initialize the alternate screen and then immediately exit when --quit-if-one-screen was passed to it, which effectively made it print nothing.

@sharkdp
Copy link
Owner

sharkdp commented Nov 24, 2019

@dickshaydle Could you please post the output of

set -x

bat --version
bat --config-file
bat --cache-dir
less --version

bat "$(bat --config-file)"
ls "$(bat --cache-dir)"

set +x

echo "BAT_PAGER = '$BAT_PAGER'"
echo "BAT_CONFIG_PATH = '$BAT_CONFIG_PATH'"
echo "BAT_STYLE = '$BAT_STYLE'"
echo "BAT_THEME = '$BAT_THEME'"
echo "BAT_TABS = '$BAT_TABS'"
echo "PAGER = '$PAGER'"
echo "LESS = '$LESS'"

And also give some information about your less version (less --version + your OS)

@dickshaydle
Copy link
Author

>>> set -x   # i removed all lines with "_zsh_autosuggest" in it because there was a lot
+precmd:1> [ -z 24708 ']'                                                                           
+precmd:2> print -Pn '\e]0;24708 - myusername@: /home/myusername\a'
+omz_termsupport_precmd:1> emulate -L zsh
+omz_termsupport_precmd:3> [[ true == true ]]
+omz_termsupport_precmd:4> return
+_zsh_highlight_main__precmd_hook:1> _zsh_highlight_main__command_type_cache=( ) 
+zsh:3> prompt_context
+prompt_context:1> [[ myusername !=  ]]
+prompt_context:2> echo -n $'%{\C-[[00m%}%{\C-[[31m%}myusername@%m%{\C-[[00m%}%{\C-[[01;33m%}%~%<<%{\C-[[00m%}'
+zsh:3> git_prompt_info
+git_prompt_info:1> local ref
+git_prompt_info:2> [[+git_prompt_info:2> git config --get oh-my-zsh.hide-status
+git_prompt_info:2> [[ '' != 1 ]]
+git_prompt_info:3> ref=+git_prompt_info:3> git symbolic-ref HEAD
+git_prompt_info:3> ref='' 
+git_prompt_info:4> ref=+git_prompt_info:4> git rev-parse --short HEAD
+git_prompt_info:4> ref='' 
+git_prompt_info:4> return 0
+zsh:3> svn_prompt_info
+svn_prompt_info:1> local _DISPLAY
+svn_prompt_info:2> in_svn
+in_svn:1> svn info

>>> bat --version                                                     
bat 0.12.1

>>> bat --config-file                                                 
/home/myusername/.config/bat/config

>>> bat --cache-dir                                                   
/home/myusername/.cache/bat

>>> less --version                                                    
less 487 (GNU regular expressions)
Copyright (C) 1984-2016  Mark Nudelman

less comes with NO WARRANTY, to the extent permitted by law.
For information about the terms of redistribution,
see the file named README in the less distribution.
Homepage: http://www.greenwoodsoftware.com/less

>>> bat "$(bat --config-file)"                                        
[bat error]: '/home/myusername/.config/bat/config': No such file or directory (os error 2)

>>> ls "$(bat --cache-dir)"                                           
"/home/myusername/.cache/bat": No such file or directory (os error 2)

>>> echo "BAT_PAGER = '$BAT_PAGER'"                               
echo "BAT_CONFIG_PATH = '$BAT_CONFIG_PATH'"
echo "BAT_STYLE = '$BAT_STYLE'"
echo "BAT_THEME = '$BAT_THEME'"
echo "BAT_TABS = '$BAT_TABS'"
echo "PAGER = '$PAGER'"
echo "LESS = '$LESS'"
BAT_PAGER = ''
BAT_CONFIG_PATH = ''
BAT_STYLE = ''
BAT_THEME = ''
BAT_TABS = ''
PAGER = 'less'
LESS = '-R -M'

my OS is Linux Mint 19 / Ubuntu 18.4:
>>> uname -a
Linux taschenrechner 4.15.0-20-generic #21-Ubuntu SMP Tue Apr 24 06:16:15 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

>>> lsb_release -a
No LSB modules are available.
Distributor ID: LinuxMint
Description:  Linux Mint 19 Tara
Release:  19
Codename: tara

I've set $LESS to '' but it didn't change the behaviour.

@sharkdp
Copy link
Owner

sharkdp commented Dec 9, 2019

Thank you very much for the feedback.

I can not reproduce this. Which terminal emulator are you using? Are you withing a tmux session or similiar?

How does less itself behave? Do you experience similar problems (with the options that are passed by bat)?

@dickshaydle
Copy link
Author

i'm using gnome-terminal 3.28.1, but same behavior in XTerm(330) and xfce4-terminal 0.8.7.4 (Xfce 4.12).

I can reproduce it with less alone now. With a little help of strace (strace -ff -s 10000 bat --pager="less" somefile.txt 2> bat.strace) i could see the less parameters:

...
[pid 12880] execve("/usr/bin/less", ["less", "--RAW-CONTROL-CHARS", "--no-init", "--quit-if-one-
screen"], 0x16f14a0 /* 28 vars */ <unfinished ...>
...

less --RAW-CONTROL-CHARS --no-init --quit-if-one-screen somefile.txt shows the same behavior.

the minimal command line to reproduce for me is
less --no-init somefile.txt

from man less

-X or --no-init
              Disables  sending  the  termcap  initialization and deinitialization strings to the
              terminal.  This is sometimes desirable if the deinitialization  string  does  some‐
              thing unnecessary, like clearing the screen.

clearing to screen seems to be pretty reasonable in this case, i guess.
I've also seen that this is intentionally to fix a bug in older less versions, but it might introduce a bug in later versions.

I've tested the --no-init with less 551 on another machine and it also does not clear the screen and leaves previous output behind.

So what about not using the --no-init as a default?

@sharkdp
Copy link
Owner

sharkdp commented Dec 15, 2019

So what about not using the --no-init as a default?

I guess we would have to parse the output of less --version if we really want to do that.

I'm afraid that lots of Linux distributions still ship older versions of less. Ubuntu 18.04 comes with less 487, for example.

@sharkdp
Copy link
Owner

sharkdp commented Dec 22, 2019

As for now, I recommend you simply add

--pager="less -FR"

to your configuration file, as suggested in the README.

@sharkdp sharkdp added feature-request New feature or request good first issue Good for newcomers labels Dec 22, 2019
@sharkdp
Copy link
Owner

sharkdp commented Dec 22, 2019

Let's try to implement this by parsing the output of less --version.

Then

  • If the version is old (older than 551?), pass --no-init by default.
  • If not, omit passing --no-init

@eth-p
Copy link
Collaborator

eth-p commented Dec 23, 2019

@sharkdp I'm all for adding this feature. It would greatly improve the default experience, especially since the scroll wheel only works when using an alternate screen (i.e. without --no-init).

If we implement this, I would recommend we either cache the results in a tempfile for 24 hours, or only try to parse less --version if the pager is being used though.

Adding more fork() and exec() overhead would likely hurt the performance of scripts that call bat for every input file.

@sharkdp
Copy link
Owner

sharkdp commented Dec 23, 2019

Good point about the performance impact.

I wrote a first implementation of this to run a simple benchmark:

use std::process::Command;

fn parse_less_version(output: &[u8]) -> Option<usize> {
    if output.starts_with(b"less ") {
        let version = std::str::from_utf8(&output[5..]).ok()?;
        let end = version.find(' ')?;
        version[..end].parse::<usize>().ok()
    } else {
        None
    }
}

fn retrieve_less_version() -> Option<usize> {
    let cmd = Command::new("less").arg("--version").output().ok()?;
    parse_less_version(&cmd.stdout)
}

fn main() {
    for _ in 0..1000 {
        println!("{:?}", retrieve_less_version());
    }
}

The whole program takes 1.755 s ± 0.057 s, so each iteration is below 2 ms. I would really try to avoid any kind of caching mechanism, as this will likely lead to other problems.

or only try to parse less --version if the pager is being used though.

Yes, absolutely. We would basically just call retrieve_less_version() directly before we call less anyway.

Adding more fork() and exec() overhead would likely hurt the performance of scripts that call bat for every input file.

If you are calling it for every input file, you would probably turn off the pager anyway?

sharkdp added a commit that referenced this issue Dec 23, 2019
With this change, we do not pass the `--no-init` option in newer
versions of less (530 or higher).

This fixes #749
@sharkdp
Copy link
Owner

sharkdp commented Dec 23, 2019

I opened a PR that attempts to fix this.

sharkdp added a commit that referenced this issue Dec 23, 2019
With this change, we do not pass the `--no-init` option in newer
versions of less (530 or higher).

This fixes #749
@sharkdp
Copy link
Owner

sharkdp commented Mar 22, 2020

This has been fixed in bat 0.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants