Patchwork [1,of,2] pager: set some environment variables if they're not set

login
register
mail settings
Submitter Jun Wu
Date April 12, 2017, 11:51 p.m.
Message ID <c8f21c8e1a9a8b5ca97b.1492041065@x1c>
Download mbox | patch
Permalink /patch/20152/
State Accepted
Headers show

Comments

Jun Wu - April 12, 2017, 11:51 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1492039375 25200
#      Wed Apr 12 16:22:55 2017 -0700
# Node ID c8f21c8e1a9a8b5ca97bb87916ede5770d6f7021
# Parent  1c398f7f4aa437a7c692025f0434c0564dbf72ff
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r c8f21c8e1a9a
pager: set some environment variables if they're not set

Git did this already [1] [2]. We want this behavior too [3].

This provides a better default user experience (like, supporting colors) if
users have things like "PAGER=less" set, which is not uncommon.

[1]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/pager.c#L87
[2]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/Makefile#L1545
[3]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/094780.html
Jun Wu - April 13, 2017, 12:05 a.m.
I know it's semi-freeze so new features are not welcome now. But we have got
enough complaints (pager loses color, draws raw characters, enters
fullscreen unexpectedly) internally.

Strictly speaking, this is an user error of setting "PAGER" to "less".
However other software like git works just fine. Therefore I think this is
an important fix to match git's behavior.

Excerpts from Jun Wu's message of 2017-04-12 16:51:05 -0700:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1492039375 25200
> #      Wed Apr 12 16:22:55 2017 -0700
> # Node ID c8f21c8e1a9a8b5ca97bb87916ede5770d6f7021
> # Parent  1c398f7f4aa437a7c692025f0434c0564dbf72ff
> # Available At https://bitbucket.org/quark-zju/hg-draft 
> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r c8f21c8e1a9a
> pager: set some environment variables if they're not set
> 
> Git did this already [1] [2]. We want this behavior too [3].
> 
> This provides a better default user experience (like, supporting colors) if
> users have things like "PAGER=less" set, which is not uncommon.
> 
> [1]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/pager.c#L87
> [2]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/Makefile#L1545
> [3]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/094780.html 
> 
> diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> --- a/mercurial/chgserver.py
> +++ b/mercurial/chgserver.py
> @@ -191,6 +191,6 @@ def _newchgui(srcui, csystem, attachio):
>              return self._csystem(cmd, util.shellenviron(environ), cwd)
>  
> -        def _runpager(self, cmd):
> -            self._csystem(cmd, util.shellenviron(), type='pager',
> +        def _runpager(self, cmd, env=None):
> +            self._csystem(cmd, util.shellenviron(env), type='pager',
>                            cmdtable={'attachio': attachio})
>              return True
> diff --git a/mercurial/help/pager.txt b/mercurial/help/pager.txt
> --- a/mercurial/help/pager.txt
> +++ b/mercurial/help/pager.txt
> @@ -34,2 +34,11 @@ To globally turn off all attempts to use
>  
>  which will prevent the pager from running.
> +
> +In order to provide a better default user experience, some environment
> +variables will be set if they are not set. For example, if LESS is not set, it
> +will be set to FRX. To override the list of environment variables, set::
> +
> +  [pager]
> +  defaultenv = LESS=FRQXi, LESSKEY=~/.lesskey
> +
> +Commas are needed to separate multiple default environment variables.
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -855,4 +855,13 @@ class ui(object):
>              return
>  
> +        pagerenv = encoding.environ.copy()
> +        for entry in self.configlist('pager', 'defaultenv',
> +                                     ['LESS=FRX', 'LV=-c']):
> +            if '=' not in entry:
> +                raise error.Abort(_('invalid pager.defaultenv config: %s')
> +                                  % entry)
> +            name, value = entry.split('=', 1)
> +            pagerenv.setdefault(name, value)
> +
>          self.debug('starting pager for command %r\n' % command)
>          self.flush()
> @@ -861,5 +870,5 @@ class ui(object):
>          if util.safehasattr(signal, "SIGPIPE"):
>              signal.signal(signal.SIGPIPE, _catchterm)
> -        if self._runpager(pagercmd):
> +        if self._runpager(pagercmd, pagerenv):
>              self.pageractive = True
>              # Preserve the formatted-ness of the UI. This is important
> @@ -880,5 +889,5 @@ class ui(object):
>              self.disablepager()
>  
> -    def _runpager(self, command):
> +    def _runpager(self, command, env=None):
>          """Actually start the pager and set up file descriptors.
>  
> @@ -913,5 +922,6 @@ class ui(object):
>                  command, shell=shell, bufsize=-1,
>                  close_fds=util.closefds, stdin=subprocess.PIPE,
> -                stdout=util.stdout, stderr=util.stderr)
> +                stdout=util.stdout, stderr=util.stderr,
> +                env=util.shellenviron(env))
>          except OSError as e:
>              if e.errno == errno.ENOENT and not shell:
> diff --git a/tests/test-pager.t b/tests/test-pager.t
> --- a/tests/test-pager.t
> +++ b/tests/test-pager.t
> @@ -255,2 +255,41 @@ Put annotate in the ignore list for page
>     9: a 9
>    10: a 10
> +
> +pager.defaultenv controls default environments
> +
> +  $ cat > $TESTTMP/printless.py <<EOF
> +  > import os, sys
> +  > sys.stdin.read()
> +  > for name in ['LESS', 'LV']:
> +  >     sys.stdout.write(('%s=%s\n') % (name, os.environ.get(name, '-')))
> +  > sys.stdout.flush()
> +  > EOF
> +
> +  $ cat >> $HGRCPATH <<EOF
> +  > [alias]
> +  > noop = log -r 0 -T ''
> +  > [ui]
> +  > formatted=1
> +  > [pager]
> +  > pager = $PYTHON $TESTTMP/printless.py
> +  > EOF
> +  $ unset LESS
> +  $ unset LV
> +  $ hg noop --pager=on
> +  LESS=FRX
> +  LV=-c
> +  $ hg noop --pager=on --config pager.defaultenv=
> +  LESS=-
> +  LV=-
> +  $ hg noop --pager=on --config pager.defaultenv=LESS=ABCD
> +  LESS=ABCD
> +  LV=-
> +  $ hg noop --pager=on --config pager.defaultenv=UNRELATED=X,LV=ABCD
> +  LESS=-
> +  LV=ABCD
> +  $ hg noop --pager=on --config pager.defaultenv=LV=ABCD,LESS=DCBA
> +  LESS=DCBA
> +  LV=ABCD
> +  $ LESS=EFGH hg noop --pager=on --config pager.defaultenv=LESS=ABCD,LV=IJKL
> +  LESS=EFGH
> +  LV=IJKL
Yuya Nishihara - April 13, 2017, 11:56 a.m.
On Wed, 12 Apr 2017 16:51:05 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1492039375 25200
> #      Wed Apr 12 16:22:55 2017 -0700
> # Node ID c8f21c8e1a9a8b5ca97bb87916ede5770d6f7021
> # Parent  1c398f7f4aa437a7c692025f0434c0564dbf72ff
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r c8f21c8e1a9a
> pager: set some environment variables if they're not set
> 
> Git did this already [1] [2]. We want this behavior too [3].
> 
> This provides a better default user experience (like, supporting colors) if
> users have things like "PAGER=less" set, which is not uncommon.
> 
> [1]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/pager.c#L87
> [2]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/Makefile#L1545
> [3]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/094780.html

> diff --git a/mercurial/help/pager.txt b/mercurial/help/pager.txt
> --- a/mercurial/help/pager.txt
> +++ b/mercurial/help/pager.txt
> @@ -34,2 +34,11 @@ To globally turn off all attempts to use
>  
>  which will prevent the pager from running.
> +
> +In order to provide a better default user experience, some environment
> +variables will be set if they are not set. For example, if LESS is not set, it
> +will be set to FRX. To override the list of environment variables, set::
> +
> +  [pager]
> +  defaultenv = LESS=FRQXi, LESSKEY=~/.lesskey
> +
> +Commas are needed to separate multiple default environment variables.
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -855,4 +855,13 @@ class ui(object):
>              return
>  
> +        pagerenv = encoding.environ.copy()
> +        for entry in self.configlist('pager', 'defaultenv',
> +                                     ['LESS=FRX', 'LV=-c']):
> +            if '=' not in entry:
> +                raise error.Abort(_('invalid pager.defaultenv config: %s')
> +                                  % entry)
> +            name, value = entry.split('=', 1)
> +            pagerenv.setdefault(name, value)

Instead of introducing defaultenv config, how about always setting
"LESS=FRX LV=-c" unless they are in environ dict? Users can easily override
them by pager command:

  [pager]
  pager = LESS=whatever less

I feel writing environs in the configlist syntax isn't nice.

> @@ -913,5 +922,6 @@ class ui(object):
>                  command, shell=shell, bufsize=-1,
>                  close_fds=util.closefds, stdin=subprocess.PIPE,
> -                stdout=util.stdout, stderr=util.stderr)
> +                stdout=util.stdout, stderr=util.stderr,
> +                env=util.shellenviron(env))

Nit: util.shellenviron() only needs a partial environ dict to override.
Augie Fackler - April 13, 2017, 1:54 p.m.
On Wed, Apr 12, 2017 at 05:05:58PM -0700, Jun Wu wrote:
> I know it's semi-freeze so new features are not welcome now. But we have got
> enough complaints (pager loses color, draws raw characters, enters
> fullscreen unexpectedly) internally.
>
> Strictly speaking, this is an user error of setting "PAGER" to "less".
> However other software like git works just fine. Therefore I think this is
> an important fix to match git's behavior.

Agreed. Send a v2 addressing Yuya's comments?

>
> Excerpts from Jun Wu's message of 2017-04-12 16:51:05 -0700:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1492039375 25200
> > #      Wed Apr 12 16:22:55 2017 -0700
> > # Node ID c8f21c8e1a9a8b5ca97bb87916ede5770d6f7021
> > # Parent  1c398f7f4aa437a7c692025f0434c0564dbf72ff
> > # Available At https://bitbucket.org/quark-zju/hg-draft
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r c8f21c8e1a9a
> > pager: set some environment variables if they're not set
> >
> > Git did this already [1] [2]. We want this behavior too [3].
> >
> > This provides a better default user experience (like, supporting colors) if
> > users have things like "PAGER=less" set, which is not uncommon.
> >
> > [1]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/pager.c#L87
> > [2]: https://github.com/git/git/blob/6a5ff7acb5965718cc7016c0ab6c601454fd7cde/Makefile#L1545
> > [3]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/094780.html
> >
> > diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> > --- a/mercurial/chgserver.py
> > +++ b/mercurial/chgserver.py
> > @@ -191,6 +191,6 @@ def _newchgui(srcui, csystem, attachio):
> >              return self._csystem(cmd, util.shellenviron(environ), cwd)
> >
> > -        def _runpager(self, cmd):
> > -            self._csystem(cmd, util.shellenviron(), type='pager',
> > +        def _runpager(self, cmd, env=None):
> > +            self._csystem(cmd, util.shellenviron(env), type='pager',
> >                            cmdtable={'attachio': attachio})
> >              return True
> > diff --git a/mercurial/help/pager.txt b/mercurial/help/pager.txt
> > --- a/mercurial/help/pager.txt
> > +++ b/mercurial/help/pager.txt
> > @@ -34,2 +34,11 @@ To globally turn off all attempts to use
> >
> >  which will prevent the pager from running.
> > +
> > +In order to provide a better default user experience, some environment
> > +variables will be set if they are not set. For example, if LESS is not set, it
> > +will be set to FRX. To override the list of environment variables, set::
> > +
> > +  [pager]
> > +  defaultenv = LESS=FRQXi, LESSKEY=~/.lesskey
> > +
> > +Commas are needed to separate multiple default environment variables.
> > diff --git a/mercurial/ui.py b/mercurial/ui.py
> > --- a/mercurial/ui.py
> > +++ b/mercurial/ui.py
> > @@ -855,4 +855,13 @@ class ui(object):
> >              return
> >
> > +        pagerenv = encoding.environ.copy()
> > +        for entry in self.configlist('pager', 'defaultenv',
> > +                                     ['LESS=FRX', 'LV=-c']):
> > +            if '=' not in entry:
> > +                raise error.Abort(_('invalid pager.defaultenv config: %s')
> > +                                  % entry)
> > +            name, value = entry.split('=', 1)
> > +            pagerenv.setdefault(name, value)
> > +
> >          self.debug('starting pager for command %r\n' % command)
> >          self.flush()
> > @@ -861,5 +870,5 @@ class ui(object):
> >          if util.safehasattr(signal, "SIGPIPE"):
> >              signal.signal(signal.SIGPIPE, _catchterm)
> > -        if self._runpager(pagercmd):
> > +        if self._runpager(pagercmd, pagerenv):
> >              self.pageractive = True
> >              # Preserve the formatted-ness of the UI. This is important
> > @@ -880,5 +889,5 @@ class ui(object):
> >              self.disablepager()
> >
> > -    def _runpager(self, command):
> > +    def _runpager(self, command, env=None):
> >          """Actually start the pager and set up file descriptors.
> >
> > @@ -913,5 +922,6 @@ class ui(object):
> >                  command, shell=shell, bufsize=-1,
> >                  close_fds=util.closefds, stdin=subprocess.PIPE,
> > -                stdout=util.stdout, stderr=util.stderr)
> > +                stdout=util.stdout, stderr=util.stderr,
> > +                env=util.shellenviron(env))
> >          except OSError as e:
> >              if e.errno == errno.ENOENT and not shell:
> > diff --git a/tests/test-pager.t b/tests/test-pager.t
> > --- a/tests/test-pager.t
> > +++ b/tests/test-pager.t
> > @@ -255,2 +255,41 @@ Put annotate in the ignore list for page
> >     9: a 9
> >    10: a 10
> > +
> > +pager.defaultenv controls default environments
> > +
> > +  $ cat > $TESTTMP/printless.py <<EOF
> > +  > import os, sys
> > +  > sys.stdin.read()
> > +  > for name in ['LESS', 'LV']:
> > +  >     sys.stdout.write(('%s=%s\n') % (name, os.environ.get(name, '-')))
> > +  > sys.stdout.flush()
> > +  > EOF
> > +
> > +  $ cat >> $HGRCPATH <<EOF
> > +  > [alias]
> > +  > noop = log -r 0 -T ''
> > +  > [ui]
> > +  > formatted=1
> > +  > [pager]
> > +  > pager = $PYTHON $TESTTMP/printless.py
> > +  > EOF
> > +  $ unset LESS
> > +  $ unset LV
> > +  $ hg noop --pager=on
> > +  LESS=FRX
> > +  LV=-c
> > +  $ hg noop --pager=on --config pager.defaultenv=
> > +  LESS=-
> > +  LV=-
> > +  $ hg noop --pager=on --config pager.defaultenv=LESS=ABCD
> > +  LESS=ABCD
> > +  LV=-
> > +  $ hg noop --pager=on --config pager.defaultenv=UNRELATED=X,LV=ABCD
> > +  LESS=-
> > +  LV=ABCD
> > +  $ hg noop --pager=on --config pager.defaultenv=LV=ABCD,LESS=DCBA
> > +  LESS=DCBA
> > +  LV=ABCD
> > +  $ LESS=EFGH hg noop --pager=on --config pager.defaultenv=LESS=ABCD,LV=IJKL
> > +  LESS=EFGH
> > +  LV=IJKL
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
--- a/mercurial/chgserver.py
+++ b/mercurial/chgserver.py
@@ -191,6 +191,6 @@  def _newchgui(srcui, csystem, attachio):
             return self._csystem(cmd, util.shellenviron(environ), cwd)
 
-        def _runpager(self, cmd):
-            self._csystem(cmd, util.shellenviron(), type='pager',
+        def _runpager(self, cmd, env=None):
+            self._csystem(cmd, util.shellenviron(env), type='pager',
                           cmdtable={'attachio': attachio})
             return True
diff --git a/mercurial/help/pager.txt b/mercurial/help/pager.txt
--- a/mercurial/help/pager.txt
+++ b/mercurial/help/pager.txt
@@ -34,2 +34,11 @@  To globally turn off all attempts to use
 
 which will prevent the pager from running.
+
+In order to provide a better default user experience, some environment
+variables will be set if they are not set. For example, if LESS is not set, it
+will be set to FRX. To override the list of environment variables, set::
+
+  [pager]
+  defaultenv = LESS=FRQXi, LESSKEY=~/.lesskey
+
+Commas are needed to separate multiple default environment variables.
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -855,4 +855,13 @@  class ui(object):
             return
 
+        pagerenv = encoding.environ.copy()
+        for entry in self.configlist('pager', 'defaultenv',
+                                     ['LESS=FRX', 'LV=-c']):
+            if '=' not in entry:
+                raise error.Abort(_('invalid pager.defaultenv config: %s')
+                                  % entry)
+            name, value = entry.split('=', 1)
+            pagerenv.setdefault(name, value)
+
         self.debug('starting pager for command %r\n' % command)
         self.flush()
@@ -861,5 +870,5 @@  class ui(object):
         if util.safehasattr(signal, "SIGPIPE"):
             signal.signal(signal.SIGPIPE, _catchterm)
-        if self._runpager(pagercmd):
+        if self._runpager(pagercmd, pagerenv):
             self.pageractive = True
             # Preserve the formatted-ness of the UI. This is important
@@ -880,5 +889,5 @@  class ui(object):
             self.disablepager()
 
-    def _runpager(self, command):
+    def _runpager(self, command, env=None):
         """Actually start the pager and set up file descriptors.
 
@@ -913,5 +922,6 @@  class ui(object):
                 command, shell=shell, bufsize=-1,
                 close_fds=util.closefds, stdin=subprocess.PIPE,
-                stdout=util.stdout, stderr=util.stderr)
+                stdout=util.stdout, stderr=util.stderr,
+                env=util.shellenviron(env))
         except OSError as e:
             if e.errno == errno.ENOENT and not shell:
diff --git a/tests/test-pager.t b/tests/test-pager.t
--- a/tests/test-pager.t
+++ b/tests/test-pager.t
@@ -255,2 +255,41 @@  Put annotate in the ignore list for page
    9: a 9
   10: a 10
+
+pager.defaultenv controls default environments
+
+  $ cat > $TESTTMP/printless.py <<EOF
+  > import os, sys
+  > sys.stdin.read()
+  > for name in ['LESS', 'LV']:
+  >     sys.stdout.write(('%s=%s\n') % (name, os.environ.get(name, '-')))
+  > sys.stdout.flush()
+  > EOF
+
+  $ cat >> $HGRCPATH <<EOF
+  > [alias]
+  > noop = log -r 0 -T ''
+  > [ui]
+  > formatted=1
+  > [pager]
+  > pager = $PYTHON $TESTTMP/printless.py
+  > EOF
+  $ unset LESS
+  $ unset LV
+  $ hg noop --pager=on
+  LESS=FRX
+  LV=-c
+  $ hg noop --pager=on --config pager.defaultenv=
+  LESS=-
+  LV=-
+  $ hg noop --pager=on --config pager.defaultenv=LESS=ABCD
+  LESS=ABCD
+  LV=-
+  $ hg noop --pager=on --config pager.defaultenv=UNRELATED=X,LV=ABCD
+  LESS=-
+  LV=ABCD
+  $ hg noop --pager=on --config pager.defaultenv=LV=ABCD,LESS=DCBA
+  LESS=DCBA
+  LV=ABCD
+  $ LESS=EFGH hg noop --pager=on --config pager.defaultenv=LESS=ABCD,LV=IJKL
+  LESS=EFGH
+  LV=IJKL