Patchwork [STABLE] pager: set MORE=FRX on OS X and FreeBSD

login
register
mail settings
Submitter Jun Wu
Date April 21, 2017, 11:16 p.m.
Message ID <166f5440301761a12a64.1492816564@x1c>
Download mbox | patch
Permalink /patch/20277/
State Rejected
Headers show

Comments

Jun Wu - April 21, 2017, 11:16 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1492816528 25200
#      Fri Apr 21 16:15:28 2017 -0700
# Node ID 166f5440301761a12a6441afda4aa9a87223ffdc
# Parent  6e0368b6e0bb2aa5210daec091c0200583553a78
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 166f54403017
pager: set MORE=FRX on OS X and FreeBSD

We use "more" as the fallback pager when no config is loaded (ex.
HGRCPATH=/dev/null). Interestingly, "more" on Linux supports colors out of
box. But on FreeBSD and OS X, "more" is backed by "less" and need the "R"
flag to support colors. This patch sets MORE=FRX on FreeBSD and OS X.

Note: we cannot set it on Linux because that leads to "unknown option -FRX".

This solves an OS X pager and color issue reported at [1].

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-April/097021.html
Gregory Szorc - April 22, 2017, 12:19 a.m.
On Fri, Apr 21, 2017 at 4:16 PM, Jun Wu <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1492816528 25200
> #      Fri Apr 21 16:15:28 2017 -0700
> # Node ID 166f5440301761a12a6441afda4aa9a87223ffdc
> # Parent  6e0368b6e0bb2aa5210daec091c0200583553a78
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> 166f54403017
> pager: set MORE=FRX on OS X and FreeBSD
>
> We use "more" as the fallback pager when no config is loaded (ex.
> HGRCPATH=/dev/null). Interestingly, "more" on Linux supports colors out of
> box. But on FreeBSD and OS X, "more" is backed by "less" and need the "R"
> flag to support colors. This patch sets MORE=FRX on FreeBSD and OS X.
>
> Note: we cannot set it on Linux because that leads to "unknown option
> -FRX".
>
> This solves an OS X pager and color issue reported at [1].
>
> [1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017
> -April/097021.html


I think this patch is acceptable. However, at the point we're sniffing
platforms and assuming that `more` is `less`, wouldn't we be better off
switching the default pager to `less` on these platforms? After all,
`MORE=FRX more` will cause actual/not-less `more` to error with "unknown
option -FRX." So if we know we're using `less,` let's just use it and avoid
less's "more compatibility mode" altogether.

In terms of failure scenarios, I think "less not found" on the output from
an hg command is easier to comprehend than "more: unknown option -FRX"
(which includes several lines of help output from `more` BTW). On that
front, I think we need better messaging around failures to invoke the
pager. Right now:

  $ hg log --config pager.pager=foobar
  /bin/sh: 1: foobar: not found

  $ PAGER=foobar hg log
  /bin/sh: 1: foobar: not found

I really think hg needs to be explicit that the pager failed to invoke.


>
>
> diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
> --- a/mercurial/rcutil.py
> +++ b/mercurial/rcutil.py
> @@ -9,4 +9,5 @@ from __future__ import absolute_import
>
>  import os
> +import re
>
>  from . import (
> @@ -96,3 +97,11 @@ def defaultpagerenv():
>      intended to be set before starting a pager.
>      '''
> -    return {'LESS': 'FRX', 'LV': '-c'}
> +    pagerenv = {'LESS': 'FRX', 'LV': '-c'}
> +    if re.search('freebsd|darwin', pycompat.sysplatform):
> +        # On OS X and FreeBSD, "more" is "less" [1], and needs "R" to not
> lose
> +        # color. On Linux, "more" is a different binary [2] that supports
> +        # colors by default and will crash if it sees those unrecognised
> flags.
> +        # [1]: http://www.greenwoodsoftware.com/less/
> +        # [2]: https://www.kernel.org/pub/linux/utils/util-linux/
> +        pagerenv['MORE'] = 'FRX'
> +    return pagerenv
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Jun Wu - April 22, 2017, 12:30 a.m.
Excerpts from Gregory Szorc's message of 2017-04-21 17:19:37 -0700:
> I think this patch is acceptable. However, at the point we're sniffing
> platforms and assuming that `more` is `less`, wouldn't we be better off
> switching the default pager to `less` on these platforms? After all,
> `MORE=FRX more` will cause actual/not-less `more` to error with "unknown
> option -FRX." So if we know we're using `less,` let's just use it and avoid
> less's "more compatibility mode" altogether.

I think both are nice to have. This patch also solves issues if people
specify "pager.pager=more" explicitly.

For changing "fallbackpager" from "more" to "less", I'm +0.5 but I may be
biased. Others should comment.

> In terms of failure scenarios, I think "less not found" on the output from
> an hg command is easier to comprehend than "more: unknown option -FRX"
> (which includes several lines of help output from `more` BTW). On that
> front, I think we need better messaging around failures to invoke the
> pager. Right now:
> 
>   $ hg log --config pager.pager=foobar
>   /bin/sh: 1: foobar: not found
> 
>   $ PAGER=foobar hg log
>   /bin/sh: 1: foobar: not found
> 
> I really think hg needs to be explicit that the pager failed to invoke.

This is a different (hard) issue, and vanilla hg already behaves better
after 9335dc6b2a9c. I think you're using chg, and chg's pager impl needs to
be updated to match 9335dc6b2a9c.

> >
> >
> > diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
> > --- a/mercurial/rcutil.py
> > +++ b/mercurial/rcutil.py
> > @@ -9,4 +9,5 @@ from __future__ import absolute_import
> >
> >  import os
> > +import re
> >
> >  from . import (
> > @@ -96,3 +97,11 @@ def defaultpagerenv():
> >      intended to be set before starting a pager.
> >      '''
> > -    return {'LESS': 'FRX', 'LV': '-c'}
> > +    pagerenv = {'LESS': 'FRX', 'LV': '-c'}
> > +    if re.search('freebsd|darwin', pycompat.sysplatform):
> > +        # On OS X and FreeBSD, "more" is "less" [1], and needs "R" to not
> > lose
> > +        # color. On Linux, "more" is a different binary [2] that supports
> > +        # colors by default and will crash if it sees those unrecognised
> > flags.
> > +        # [1]: http://www.greenwoodsoftware.com/less/ 
> > +        # [2]: https://www.kernel.org/pub/linux/utils/util-linux/
> > +        pagerenv['MORE'] = 'FRX'
> > +    return pagerenv
Gregory Szorc - April 22, 2017, 12:58 a.m.
On Fri, Apr 21, 2017 at 5:30 PM, Jun Wu <quark@fb.com> wrote:

> Excerpts from Gregory Szorc's message of 2017-04-21 17:19:37 -0700:
> > I think this patch is acceptable. However, at the point we're sniffing
> > platforms and assuming that `more` is `less`, wouldn't we be better off
> > switching the default pager to `less` on these platforms? After all,
> > `MORE=FRX more` will cause actual/not-less `more` to error with "unknown
> > option -FRX." So if we know we're using `less,` let's just use it and
> avoid
> > less's "more compatibility mode" altogether.
>
> I think both are nice to have. This patch also solves issues if people
> specify "pager.pager=more" explicitly.
>

That's a good point. So this patch has merits on its own and solves a real
problem. We can move discussion for swapping more for less to another patch.


>
> For changing "fallbackpager" from "more" to "less", I'm +0.5 but I may be
> biased. Others should comment.
>
> > In terms of failure scenarios, I think "less not found" on the output
> from
> > an hg command is easier to comprehend than "more: unknown option -FRX"
> > (which includes several lines of help output from `more` BTW). On that
> > front, I think we need better messaging around failures to invoke the
> > pager. Right now:
> >
> >   $ hg log --config pager.pager=foobar
> >   /bin/sh: 1: foobar: not found
> >
> >   $ PAGER=foobar hg log
> >   /bin/sh: 1: foobar: not found
> >
> > I really think hg needs to be explicit that the pager failed to invoke.
>
> This is a different (hard) issue, and vanilla hg already behaves better
> after 9335dc6b2a9c. I think you're using chg, and chg's pager impl needs to
> be updated to match 9335dc6b2a9c.
>
> > >
> > >
> > > diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
> > > --- a/mercurial/rcutil.py
> > > +++ b/mercurial/rcutil.py
> > > @@ -9,4 +9,5 @@ from __future__ import absolute_import
> > >
> > >  import os
> > > +import re
> > >
> > >  from . import (
> > > @@ -96,3 +97,11 @@ def defaultpagerenv():
> > >      intended to be set before starting a pager.
> > >      '''
> > > -    return {'LESS': 'FRX', 'LV': '-c'}
> > > +    pagerenv = {'LESS': 'FRX', 'LV': '-c'}
> > > +    if re.search('freebsd|darwin', pycompat.sysplatform):
> > > +        # On OS X and FreeBSD, "more" is "less" [1], and needs "R" to
> not
> > > lose
> > > +        # color. On Linux, "more" is a different binary [2] that
> supports
> > > +        # colors by default and will crash if it sees those
> unrecognised
> > > flags.
> > > +        # [1]: http://www.greenwoodsoftware.com/less/
> > > +        # [2]: https://www.kernel.org/pub/linux/utils/util-linux/
> > > +        pagerenv['MORE'] = 'FRX'
> > > +    return pagerenv
>
Augie Fackler - April 22, 2017, 1:02 a.m.
> On Apr 21, 2017, at 20:19, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
> On Fri, Apr 21, 2017 at 4:16 PM, Jun Wu <quark@fb.com <mailto:quark@fb.com>> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com <mailto:quark@fb.com>>
> # Date 1492816528 25200
> #      Fri Apr 21 16:15:28 2017 -0700
> # Node ID 166f5440301761a12a6441afda4aa9a87223ffdc
> # Parent  6e0368b6e0bb2aa5210daec091c0200583553a78
> # Available At https://bitbucket.org/quark-zju/hg-draft <https://bitbucket.org/quark-zju/hg-draft>
> #              hg pull https://bitbucket.org/quark-zju/hg-draft <https://bitbucket.org/quark-zju/hg-draft> -r 166f54403017
> pager: set MORE=FRX on OS X and FreeBSD
> 
> We use "more" as the fallback pager when no config is loaded (ex.
> HGRCPATH=/dev/null). Interestingly, "more" on Linux supports colors out of
> box. But on FreeBSD and OS X, "more" is backed by "less" and need the "R"
> flag to support colors. This patch sets MORE=FRX on FreeBSD and OS X.
> 
> Note: we cannot set it on Linux because that leads to "unknown option -FRX".
> 
> This solves an OS X pager and color issue reported at [1].
> 
> [1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-April/097021.html <https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-April/097021.html>
> 
> I think this patch is acceptable. However, at the point we're sniffing platforms and assuming that `more` is `less`, wouldn't we be better off switching the default pager to `less` on these platforms?

The vision I had for this was that packages would specify a sane pager for their platform in /etc/mercurial/pager.rc or something. I'm opposed to this patch, because it's putting the OS-specific preferences in Mercurial instead of in the packaging where it belongs.

> After all, `MORE=FRX more` will cause actual/not-less `more` to error with "unknown option -FRX." So if we know we're using `less,` let's just use it and avoid less's "more compatibility mode" altogether.
> 
> In terms of failure scenarios, I think "less not found" on the output from an hg command is easier to comprehend than "more: unknown option -FRX" (which includes several lines of help output from `more` BTW). On that front, I think we need better messaging around failures to invoke the pager. Right now:
> 
>   $ hg log --config pager.pager=foobar
>   /bin/sh: 1: foobar: not found
> 
>   $ PAGER=foobar hg log
>   /bin/sh: 1: foobar: not found
> 
> I really think hg needs to be explicit that the pager failed to invoke.
>  
> 
> 
> diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
> --- a/mercurial/rcutil.py
> +++ b/mercurial/rcutil.py
> @@ -9,4 +9,5 @@ from __future__ import absolute_import
> 
>  import os
> +import re
> 
>  from . import (
> @@ -96,3 +97,11 @@ def defaultpagerenv():
>      intended to be set before starting a pager.
>      '''
> -    return {'LESS': 'FRX', 'LV': '-c'}
> +    pagerenv = {'LESS': 'FRX', 'LV': '-c'}
> +    if re.search('freebsd|darwin', pycompat.sysplatform):
> +        # On OS X and FreeBSD, "more" is "less" [1], and needs "R" to not lose
> +        # color. On Linux, "more" is a different binary [2] that supports
> +        # colors by default and will crash if it sees those unrecognised flags.
> +        # [1]: http://www.greenwoodsoftware.com/less/ <http://www.greenwoodsoftware.com/less/>
> +        # [2]: https://www.kernel.org/pub/linux/utils/util-linux/ <https://www.kernel.org/pub/linux/utils/util-linux/>
> +        pagerenv['MORE'] = 'FRX'
> +    return pagerenv
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org <mailto:Mercurial-devel@mercurial-scm.org>
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel <https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel>
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - April 22, 2017, 1:19 a.m.
Excerpts from Augie Fackler's message of 2017-04-21 21:02:31 -0400:
> The vision I had for this was that packages would specify a sane pager for
> their platform in /etc/mercurial/pager.rc or something. I'm opposed to
> this patch, because it's putting the OS-specific preferences in Mercurial
> instead of in the packaging where it belongs.

The problem is, do you think "HGRCPATH=/dev/null hg log" needs to have a
sane behavior or not?

If not, we can make setup.py put some platform dependent files in default.d
to provide a good default if package maintainers forget. default.d configs
will be overridden by system config.
Yuya Nishihara - April 22, 2017, 3:45 a.m.
On Fri, 21 Apr 2017 17:30:50 -0700, Jun Wu wrote:
> Excerpts from Gregory Szorc's message of 2017-04-21 17:19:37 -0700:
> > I think this patch is acceptable. However, at the point we're sniffing
> > platforms and assuming that `more` is `less`, wouldn't we be better off
> > switching the default pager to `less` on these platforms? After all,
> > `MORE=FRX more` will cause actual/not-less `more` to error with "unknown
> > option -FRX." So if we know we're using `less,` let's just use it and avoid
> > less's "more compatibility mode" altogether.
> 
> I think both are nice to have. This patch also solves issues if people
> specify "pager.pager=more" explicitly.

-0. This might show cryptic error if he installed "more" of util-linux.

> For changing "fallbackpager" from "more" to "less", I'm +0.5 but I may be
> biased. Others should comment.

+1 for this. Perhaps that is the default of git?
Augie Fackler - April 22, 2017, 2:30 p.m.
> On Apr 21, 2017, at 23:45, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> For changing "fallbackpager" from "more" to "less", I'm +0.5 but I may be
>> biased. Others should comment.
> 
> +1 for this. Perhaps that is the default of git?

`less` is a better default if and only if it's available - in my (limited) research, `more` was the only pager we can be sure exists. The way git handles this is that the default-default is `less`, but you're one compile-time config knob from changing it to something else. Same for the default-default LESS environment variable.

We can probably get away with `less` as the default-default, but we should still strongly encourage packagers to do the right thing with a config rather than patching the code (since they don't need to.)
Yuya Nishihara - April 22, 2017, 4:14 p.m.
On Sat, 22 Apr 2017 10:30:37 -0400, Augie Fackler wrote:
> > On Apr 21, 2017, at 23:45, Yuya Nishihara <yuya@tcha.org> wrote:
> >> For changing "fallbackpager" from "more" to "less", I'm +0.5 but I may be
> >> biased. Others should comment.
> > 
> > +1 for this. Perhaps that is the default of git?
> 
> `less` is a better default if and only if it's available - in my (limited) research, `more` was the only pager we can be sure exists. The way git handles this is that the default-default is `less`, but you're one compile-time config knob from changing it to something else. Same for the default-default LESS environment variable.

I know "less" isn't available on some Unices (e.g. ancient SunOS), but our
current default-default sounds really bad on OSX. If we stick to "more" as
the only default, we should disable the color as well and tell packagers to
set better defaults.

> We can probably get away with `less` as the default-default, but we should still strongly encourage packagers to do the right thing with a config rather than patching the code (since they don't need to.)

+1 for encouraging packagers to set config anyway.
Augie Fackler - April 22, 2017, 4:20 p.m.
> On Apr 22, 2017, at 12:14 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Sat, 22 Apr 2017 10:30:37 -0400, Augie Fackler wrote:
>>> On Apr 21, 2017, at 23:45, Yuya Nishihara <yuya@tcha.org> wrote:
>>>> For changing "fallbackpager" from "more" to "less", I'm +0.5 but I may be
>>>> biased. Others should comment.
>>> 
>>> +1 for this. Perhaps that is the default of git?
>> 
>> `less` is a better default if and only if it's available - in my (limited) research, `more` was the only pager we can be sure exists. The way git handles this is that the default-default is `less`, but you're one compile-time config knob from changing it to something else. Same for the default-default LESS environment variable.
> 
> I know "less" isn't available on some Unices (e.g. ancient SunOS), but our
> current default-default sounds really bad on OSX. If we stick to "more" as
> the only default, we should disable the color as well and tell packagers to
> set better defaults.

Yep. Given the distribution of our users, setting `less` as the default-default seems reasonable.

>> We can probably get away with `less` as the default-default, but we should still strongly encourage packagers to do the right thing with a config rather than patching the code (since they don't need to.)
>> 
> +1 for encouraging packagers to set config anyway.

https://www.mercurial-scm.org/wiki/Packaging is updated to note that, but I think I want to clean up that page so that the most important things about package contents are at the top.

Patch

diff --git a/mercurial/rcutil.py b/mercurial/rcutil.py
--- a/mercurial/rcutil.py
+++ b/mercurial/rcutil.py
@@ -9,4 +9,5 @@  from __future__ import absolute_import
 
 import os
+import re
 
 from . import (
@@ -96,3 +97,11 @@  def defaultpagerenv():
     intended to be set before starting a pager.
     '''
-    return {'LESS': 'FRX', 'LV': '-c'}
+    pagerenv = {'LESS': 'FRX', 'LV': '-c'}
+    if re.search('freebsd|darwin', pycompat.sysplatform):
+        # On OS X and FreeBSD, "more" is "less" [1], and needs "R" to not lose
+        # color. On Linux, "more" is a different binary [2] that supports
+        # colors by default and will crash if it sees those unrecognised flags.
+        # [1]: http://www.greenwoodsoftware.com/less/
+        # [2]: https://www.kernel.org/pub/linux/utils/util-linux/
+        pagerenv['MORE'] = 'FRX'
+    return pagerenv