__fish_print_help invokes less in a somewhat annoying way (on macOS + iTerm)

[thomcc]

__fish_print_help doesn't support whatever control codes normally are used to make pager scrolling work. This is because of the X flag set here: https://github.com/fish-shell/fish-shell/blob/master/share/functions/__fish_print_help.fish#L122. If I unset that flag from there, everything works fine.

Git used to have the same issue for me, and I had to set core.pager to something that includes less -R, and I've also had to set LESS=-R in my environment for... well, I don't actually remember which one that was for. Honestly, it might have been for fish (it looks like you respect $LESS if it's set here, although I'm not sure I've ever invoked history).

So I guess one resolution is just to respect the value for $LESS. It also seems like setting PAGER to less -R in the env doesn't work either, but maybe that's intentional (I guess there's some tokenizing).

Also, the main case this prints out is for stuff like string --help, and honestly I'd be totally fine without invoking the pager for that, even if it's a bit long. That said, I don't really care, I just want scrolling to work if the pager is invoked.

FWIW while this may be misconfiguration on my system (truly I don't know, it seems likely this is caused by something weird in my terminfo), I also believe that it's a "stock" misconfiguration — I've never customized my terminfo or $TERM value. So, I think it's caused by an interaction between iTerm2 and the terminfo that's provided on macOS for xterm-256color. (I've tried it on a machine that hasn't had nearly the amount of customization that this one has, and it still shows the issue)

---

$ fish --version
fish, version 3.2.2
$ echo $version
3.2.2
$ less --version
less 581.2 (PCRE regular expressions)
Copyright (C) 1984-2021  Mark Nudelman
...

Other relevant versions: macOS Big Sur 11.3, in iTerm2 3.4.5beta3, although I've had this issue with other programs in the past and don't think it's related to using iTerm2 beta. I also updated less to check if the issue persisted, but I updated fish as well, and no longer know what the terminal version is.

I've confirmed the issue happens on a fresh shell with a mktemp config dir.

---

P.S. while filing this issue I noticed that we're using -r here, and not -R

fish-shell/share/functions/__fish_print_help.fish

Line 126 in bcbfd70

# -r (--raw-control-chars) to display bold, underline and colors

man less goes out of its way to say to use -R for stuff like styling — note that while it only mentions colors, I can confirm that with -R

Copypaste from `man less` about the -R vs -r flag

       -r or --raw-control-chars
              Causes "raw" control characters to be displayed.  The default is
              to  display control characters using the caret notation; for ex-
              ample, a control-A (octal 001) is displayed as  "^A".   Warning:
              when the -r option is used, less cannot keep track of the actual
              appearance of the screen (since this depends on how  the  screen
              responds to each type of control character).  Thus, various dis-
              play problems may result, such as long lines being split in  the
              wrong place.
          USE OF THE -r OPTION IS NOT RECOMMENDED.

   -R or --RAW-CONTROL-CHARS
          Like -r, but only ANSI "color" escape sequences and OSC 8 hyper-
          link sequences are output in "raw" form.  Unlike -r, the  screen
          appearance  is  maintained correctly, provided that there are no
          escape sequences in the file other than these  types  of  escape
          sequences.   Color  escape sequences are only supported when the
          color is changed within one line, not across  lines.   In  other
          words,  the beginning of each line is assumed to be normal (non-
          colored), regardless of any escape sequences in previous  lines.
          For the purpose of keeping track of screen appearance, these es-
          cape sequences are assumed to not move the cursor.

          OSC 8 hyperlinks are sequences of the form:

               ESC ] 8 ; ... \7

          The terminating sequence may be either a BEL character  (\7)  or
          the two-character sequence "ESC \".

          ANSI color escape sequences are sequences of the form:

               ESC [ ... m

          where  the "..." is zero or more color specification characters.
          You can make less think that characters other than "m"  can  end
          ANSI  color escape sequences by setting the environment variable
          LESSANSIENDCHARS to the list of characters which can end a color
          escape  sequence.   And  you can make less think that characters
          other than the standard ones may appear between the ESC and  the
          m  by  setting  the environment variable LESSANSIMIDCHARS to the
          list of characters which can appear.

[faho]

doesn't support whatever control codes normally are used to make pager scrolling work

Wait... what kind of "pager scrolling"? Just the keybinding stuff? Up or down-arrow or j or k? That sort of thing? Or mouse?

The tricky thing here is that we absolutely want at the very least -F in there to not (visibly) trigger less at all if unnecessary (which should be a bunch, e.g. for pwd --help). I'm not sure we can just add it.

We could probably also change -r to -R, but I don't know what that does e.g. to italics, given macOS' broken terminfo that doesn't feature them.

Or allowing the user's $LESS to be used, but that would mean you might not have -F.

[thomcc]

Sorry, mouse wheel scrolling. Instead, it scrolls the whole shell window.

We could probably also change -r to -R, but I don't know what that does e.g. to italics, given macOS' broken terminfo that doesn't feature them.

Yeah, italics still work on mac with -R. This doesn't fix my issue tho, fwiw, its just something I noticed when investigating.

[faho]

Alright, the comment says we don't actually know why we're using -X and I can't see a reason for it (it's not like less has a massive config file that could break anything here). So I've removed it, and allowed $LESS to take precedence.

I've also switched to -R - less really wants that one, so that's what it gets.

[krobelus]

Looks good, except that we used to include $LESS but that caused problem if users set $LESS to something that's not suitable for help pages. I can't find the thread right now but I think @zx8 reported it.

@thomcc Does removing -X (so d15a518 with empty $LESS) fix the issue?

EDIT: nevermind, not overriding $LESS is what Git does, so we should probably follow.

[thomcc]

Yes.