Patchwork D5019: pager: disable auto pager if we think it doesn't support F, R, X

login
register
mail settings
Submitter phabricator
Date Oct. 12, 2018, 5:45 p.m.
Message ID <differential-rev-PHID-DREV-s6tetsbs62tn645vbtxs-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/35760/
State New
Headers show

Comments

phabricator - Oct. 12, 2018, 5:45 p.m.
spectral created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5019

AFFECTED FILES
  mercurial/ui.py

CHANGE DETAILS




To: spectral, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 12, 2018, 6:24 p.m.
indygreg added a comment.


  I need to review this on a fresher brain. But I'll be honest, I'm not sure how I feel about this.
  
  Do you know of other programs that jump through similar hoops to properly set up a pager?

INLINE COMMENTS

> ui.py:1120
> +        desiredargs = 'FRrX'
> +        for ch in lessenvvar:
> +            if ch == '-':

Won't this iterate integers on Python 3?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5019

To: spectral, #hg-reviewers
Cc: indygreg, mercurial-devel

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -1030,6 +1030,164 @@ 
     def disablepager(self):
         self._disablepager = True
 
+    def _checkpager(self, explicitpager, pagercmd, pagerenv):
+        """Check that the pager can handle things properly.
+
+        >>> u = ui()
+        >>> savedenviron = encoding.environ
+        >>> u._checkpager(True, '', {'LESS': '-FRX'})
+        True
+        >>> u._checkpager(True, 'less -F', {'LESS': '-FRX'})
+        True
+        >>> encoding.environ['PAGER'] = 'more'
+        >>> encoding.environ['LESS'] = '-F'
+        >>> u._checkpager(False, 'sensible-pager', {})
+        True
+        >>> encoding.environ['PAGER'] = 'less -F'
+        >>> encoding.environ['LESS'] = '-FRM'
+        >>> u._checkpager(False, 'sensible-pager', {})
+        False
+        >>> encoding.environ['PAGER'] = 'less -X'
+        >>> encoding.environ['LESS'] = '-FRM'
+        >>> u._checkpager(False, 'sensible-pager', {})
+        True
+        >>> encoding.environ['LESS'] = '-F'
+        >>> u._checkpager(False, 'less -RX', {})
+        True
+        >>> u._checkpager(False, 'less -FrX', {})
+        True
+        >>> encoding.environ = savedenviron
+        >>> u._checkpager(False, 'less -RX', {'LESS': '-FRX'})
+        True
+        >>> u._checkpager(False, 'less -RX', {'LESS': '-FRX'})
+        True
+        >>> u._checkpager(False, '/bin/less --no-init --quit-if-one-screen --RAW-CONTROL-CHARS', {'LESS': '-FRX'})
+        True
+        >>> u._checkpager(False, '/bin/less --no-init --quit-if-one-screen --raw-control-chars', {'LESS': '-FRX'})
+        True
+        >>> u._checkpager(False, '/bin/less --no-init --quit-if-one-screen', {'LESS': '-FRX'})
+        True
+        >>> u._checkpager(False, 'less --unrelated -FRX', {'LESS': '-FRX'})
+        True
+        >>> u._checkpager(False, '/bin/less', {'LESS': 'Dn9.2$FRXDs4.1'})
+        True
+        >>> u._checkpager(False, '/bin/less', {'LESS': 'Dn9.2$Ds4.1FRX'})
+        False
+        >>> u._checkpager(False, '/bin/less', {'LESS': 'FRXDn9.2$Ds4.1'})
+        True
+        >>> encoding.environ['TERM'] = 'tmux'
+        >>> u._checkpager(False, 'less -FRX', {'LESS': '-FRX'})
+        False
+        >>> u._checkpager(False, 'less -FrX', {'LESS': '-FRX'})
+        True
+        >>> encoding.environ['TERM'] = 'screen-256color-bce'
+        >>> u._checkpager(False, 'less', {'LESS': '-FRX'})
+        False
+        >>> u._checkpager(False, 'less -r', {'LESS': '-FRX'})
+        True
+        """
+        if explicitpager:
+            return True  # Trust the user
+        if pagercmd == 'sensible-pager':
+            # Looks like we're on a Debian system. This should respect PAGER
+            # and the alternatives system, but we're only checking PAGER here,
+            # and assuming it's 'less' otherwise. The only time that we should
+            # misbehave here and *de*activate the pager is if the user is using
+            # the Debian alternatives system to pick a different pager AND the
+            # user has the $LESS environment variable set.
+            pagercmd = encoding.environ.get('PAGER', 'less')
+        pager = pycompat.shlexsplit(pagercmd)
+        if not pager:
+            return True  # No idea how this could happen, but don't error
+
+        if not pager[0].endswith('less'):
+            return True  # We don't verify non-less pagers right now
+
+        # The options that were detected as being enabled
+        lessoptions = set()
+
+        lessenvvar = pagerenv.get('LESS', encoding.environ.get('LESS', None))
+        assert lessenvvar is not None
+
+        # Search the $LESS environment variable for the options we care about
+        # (if the user does not have the variable set, then the LESS value from
+        # rcutil.defaultpagerenv will be used).
+        instrval = False
+        strvalargs = 'DkoOpPtT'
+        # Important: if you add any additional lower-case letters to this, check
+        # the calls below that uppercase things.
+        desiredargs = 'FRrX'
+        for ch in lessenvvar:
+            if ch == '-':
+                continue
+            if not instrval and ch in strvalargs:
+                # These flags are documented to take a string value, so we need
+                # to start ignoring characters until it's over (a $ character)
+                instrval = True
+                continue
+            if instrval:
+                if ch == '$':
+                    instrval = False
+                continue
+            if ch == '$':
+                # Encountered a $ outside of a string. Looks like we may have
+                # missed an option that takes a string argument? Let's just
+                # trust the user knows what they're doing.
+                return True
+            if ch in desiredargs:
+                lessoptions.add(ch.upper())
+                # This only works for 'r' implying 'R'.
+                lessoptions.add(ch)
+
+        # Really hacky commandline parser - search the commandline (as provided
+        # in .hgrc, $PAGER, etc.) for the flags.
+        for arg in pager:
+            if arg == '--quit-if-one-screen':
+                lessoptions.add('F')
+                continue
+            if arg == '--RAW-CONTROL-CHARS':
+                lessoptions.add('R')
+                continue
+            if arg == '--raw-control-chars':
+                lessoptions.add('r')
+                lessoptions.add('R')
+                continue
+            if arg == '--no-init':
+                lessoptions.add('X')
+                continue
+            if arg.startswith('-') and not arg.startswith('--'):
+                for ch in arg:
+                    if ch in strvalargs:
+                        break
+                    if ch in desiredargs:
+                        lessoptions.add(ch.upper())
+                        # This only works for 'r' implying 'R'.
+                        lessoptions.add(ch)
+                        continue
+
+        # If any of F, R, or X are NOT in lessoptions...
+        desired = {'F': 'quit if one screen',
+                   'R': 'color support',
+                   'X': 'avoid clearing screen'}
+        missing = set(desired.keys()) - lessoptions
+        if missing:
+            self.debug(
+                'NOT enabling pager, config appears to be missing:\n - %s\n'
+                % '\n - '.join(desired[k] for k in missing))
+            return False
+
+        # On tmux and screen, sgr0 prints ^O, which requires `less -r` (`less
+        # -R` is insufficient), so refuse to use the pager in that case too.
+        term = encoding.environ.get('TERM', '')
+        if term in ['tmux', 'tmux-256color', 'screen'] or \
+            term.startswith('screen-'):
+            if 'r' not in lessoptions:
+                self.debug(
+                    'NOT enabling pager, it might not display correctly.\n')
+                return False
+
+        return True
+
     def pager(self, command):
         """Start a pager for subsequent command output.
 
@@ -1048,7 +1206,8 @@ 
             # how pager should do is already determined
             return
 
-        if not command.startswith('internal-always-') and (
+        explicitpager = command.startswith('internal-always-')
+        if not explicitpager and (
             # explicit --pager=on (= 'internal-always-' prefix) should
             # take precedence over disabling factors below
             command in self.configlist('pager', 'ignore')
@@ -1076,6 +1235,9 @@ 
             if name not in encoding.environ:
                 pagerenv[name] = value
 
+        if not self._checkpager(explicitpager, pagercmd, pagerenv):
+            return
+
         self.debug('starting pager for command %r\n' % command)
         self.flush()