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
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 >
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
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 >
> 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
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.
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?
> 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.)
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.
> 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