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

login
register
mail settings
Submitter Matt Harbison
Date March 28, 2017, 2:50 a.m.
Message ID <1c73c5cde39463e03c3c.1490669427@Envy>
Download mbox | patch
Permalink /patch/19763/
State Accepted
Headers show

Comments

Matt Harbison - March 28, 2017, 2:50 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 1c73c5cde39463e03c3c7be10b6fe1256a6e5143
# Parent  2566b7eac73c4851edc21b73a833f86bf878285e
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.

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 28, 2017, 1:09 p.m.
On Mon, 27 Mar 2017 22:50:27 -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 1c73c5cde39463e03c3c7be10b6fe1256a6e5143
> # Parent  2566b7eac73c4851edc21b73a833f86bf878285e
> ui: defer setting pager related properties until the pager has spawned

>      def _runpager(self, command):
>          """Actually start the pager and set up file descriptors.
> @@ -866,7 +873,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
> @@ -896,7 +903,7 @@
>              if e.errno == errno.ENOENT and not shell:
>                  self.warn(_("missing pager command '%s', skipping pager\n")
>                            % command)
> -                return
> +                return False
>              raise

There should be one more 'return False'. Fixed in flight.

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
@@ -848,15 +848,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.
@@ -866,7 +873,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
@@ -896,7 +903,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
@@ -917,6 +924,8 @@ 
             pager.stdin.close()
             pager.wait()
 
+        return True
+
     def interface(self, feature):
         """what interface to use for interactive console features?