Patchwork [3,of,4] ui: defer setting pager related properties until the pager has spawned

login
register
mail settings
Submitter Matt Harbison
Date March 26, 2017, 4:41 a.m.
Message ID <84bda5db69dbe3d550f4.1490503266@Envy>
Download mbox | patch
Permalink /patch/19676/
State Accepted
Headers show

Comments

Matt Harbison - March 26, 2017, 4:41 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1490490720 14400
#      Sat Mar 25 21:12:00 2017 -0400
# Node ID 84bda5db69dbe3d550f45ccd6d6eda2aad760ee4
# Parent  a263702b064a5a3ce1ca74b227e8e624e4b05874
ui: defer setting pager related properties until the pager has spawned

When --pager=on is given, dispatch.py spawns a pager before setting up color.
If the pager failed to launch, ui.pageractive was left set to True, so color
configured itself based on 'color.pagermode'.  A typical MSYS setting would be
'color.mode=auto, color.pagermode=ansi'.  In the failure case, this would print
a warning, disable the pager, and then print the raw ANSI codes to the terminal.

Care needs to be taken, because it appears that leaving ui.pageractive=True was
the only thing that prevented an attempt at running the pager again from inside
the command.  This results in a double warning message, so pager is simply
disabled on failure.

I don't understand what chg is doing with pager, or how it signals a failure to
spawn the pager.  But this is no worse that the previous behavior.  The ui
config settings didn't need to be moved to fix this, but it seemed like the
right thing to do for consistency.
Yuya Nishihara - March 26, 2017, 2:41 p.m.
On Sun, 26 Mar 2017 00:41:06 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1490490720 14400
> #      Sat Mar 25 21:12:00 2017 -0400
> # Node ID 84bda5db69dbe3d550f45ccd6d6eda2aad760ee4
> # Parent  a263702b064a5a3ce1ca74b227e8e624e4b05874
> ui: defer setting pager related properties until the pager has spawned

This looks good. I slightly prefer propagating OSError instead of returning
False, but that isn't important.

  # ui.pager()
  if pagercmd == 'cat':
      return
  try:
      self._runpager(pagercmd)
  except OSError as e:
      if e.errno == errno.ENOENT:
          self.warn(_("missing pager command '%s', skipping pager\n")

> I don't understand what chg is doing with pager, or how it signals a failure to
> spawn the pager.  But this is no worse that the previous behavior.

chg never reports error since it always spawns a pager through /bin/sh.

Patch

diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
--- a/mercurial/chgserver.py
+++ b/mercurial/chgserver.py
@@ -193,6 +193,7 @@ 
         def _runpager(self, cmd):
             self._csystem(cmd, util.shellenviron(), type='pager',
                           cmdtable={'attachio': attachio})
+            return True
 
     return chgui(srcui)
 
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -850,15 +850,22 @@ 
 
         self.debug('starting pager for command %r\n' % command)
         self.flush()
-        self.pageractive = True
-        # Preserve the formatted-ness of the UI. This is important
-        # because we mess with stdout, which might confuse
-        # auto-detection of things being formatted.
-        self.setconfig('ui', 'formatted', self.formatted(), 'pager')
-        self.setconfig('ui', 'interactive', False, 'pager')
+
+        wasformatted = self.formatted()
         if util.safehasattr(signal, "SIGPIPE"):
             signal.signal(signal.SIGPIPE, _catchterm)
-        self._runpager(pagercmd)
+        if self._runpager(pagercmd):
+            self.pageractive = True
+            # Preserve the formatted-ness of the UI. This is important
+            # because we mess with stdout, which might confuse
+            # auto-detection of things being formatted.
+            self.setconfig('ui', 'formatted', wasformatted, 'pager')
+            self.setconfig('ui', 'interactive', False, 'pager')
+        else:
+            # If the pager can't be spawned in dispatch when --pager=on is
+            # given, don't try again when the command runs, to avoid a duplicate
+            # warning about a missing pager command.
+            self.disablepager()
 
     def _runpager(self, command):
         """Actually start the pager and set up file descriptors.
@@ -868,7 +875,7 @@ 
         """
         if command == 'cat':
             # Save ourselves some work.
-            return
+            return False
         # If the command doesn't contain any of these characters, we
         # assume it's a binary and exec it directly. This means for
         # simple pager command configurations, we can degrade
@@ -898,7 +905,7 @@ 
             if e.errno == errno.ENOENT and not shell:
                 self.warn(_("missing pager command '%s', skipping pager\n")
                           % command)
-                return
+                return False
             raise
 
         # back up original file descriptors
@@ -919,6 +926,8 @@ 
             pager.stdin.close()
             pager.wait()
 
+        return True
+
     def interface(self, feature):
         """what interface to use for interactive console features?