Patchwork [8,of,9,pager,v2] annotate: migrate to modern pager API

login
register
mail settings
Submitter Augie Fackler
Date Feb. 16, 2017, 4:59 p.m.
Message ID <4c2f3f1b67a71401faff.1487264357@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/18555/
State Accepted
Headers show

Comments

Augie Fackler - Feb. 16, 2017, 4:59 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1486439567 18000
#      Mon Feb 06 22:52:47 2017 -0500
# Node ID 4c2f3f1b67a71401faff082dbca79a3f212b5499
# Parent  935dccd057eb61b980901b5c7a806fac9fb55d0f
annotate: migrate to modern pager API
Yuya Nishihara - Feb. 18, 2017, 11:18 a.m.
On Thu, 16 Feb 2017 11:59:17 -0500, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1486439567 18000
> #      Mon Feb 06 22:52:47 2017 -0500
> # Node ID 4c2f3f1b67a71401faff082dbca79a3f212b5499
> # Parent  935dccd057eb61b980901b5c7a806fac9fb55d0f
> annotate: migrate to modern pager API
> 
> diff --git a/hgext/pager.py b/hgext/pager.py
> --- a/hgext/pager.py
> +++ b/hgext/pager.py
> @@ -110,4 +110,4 @@ def uisetup(ui):
>          extensions.wrapfunction(dispatch, '_runcommand', pagecmd)
>      extensions.afterloaded('color', afterloaded)
>  
> -attended = ['annotate', 'cat', 'diff', 'export', 'glog', 'log', 'qdiff']
> +attended = ['cat', 'diff', 'export', 'glog', 'log', 'qdiff']
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -361,6 +361,7 @@ def annotate(ui, repo, *pats, **opts):
>  
>      Returns 0 on success.
>      """
> +    ui.pager('annotate')
>      if not pats:
>          raise error.Abort(_('at least one filename or pattern is required'))

Just to make sure. Do we plan to delay ui.pager() call so short error messages
(and password prompt, etc.) won't be paged?
Martin von Zweigbergk - Feb. 18, 2017, 8:11 p.m.
On Feb 18, 2017 03:18, "Yuya Nishihara" <yuya@tcha.org> wrote:

On Thu, 16 Feb 2017 11:59:17 -0500, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1486439567 18000
> #      Mon Feb 06 22:52:47 2017 -0500
> # Node ID 4c2f3f1b67a71401faff082dbca79a3f212b5499
> # Parent  935dccd057eb61b980901b5c7a806fac9fb55d0f
> annotate: migrate to modern pager API
>
> diff --git a/hgext/pager.py b/hgext/pager.py
> --- a/hgext/pager.py
> +++ b/hgext/pager.py
> @@ -110,4 +110,4 @@ def uisetup(ui):
>          extensions.wrapfunction(dispatch, '_runcommand', pagecmd)
>      extensions.afterloaded('color', afterloaded)
>
> -attended = ['annotate', 'cat', 'diff', 'export', 'glog', 'log', 'qdiff']
> +attended = ['cat', 'diff', 'export', 'glog', 'log', 'qdiff']
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -361,6 +361,7 @@ def annotate(ui, repo, *pats, **opts):
>
>      Returns 0 on success.
>      """
> +    ui.pager('annotate')
>      if not pats:
>          raise error.Abort(_('at least one filename or pattern is
required'))

Just to make sure. Do we plan to delay ui.pager() call so short error
messages
(and password prompt, etc.) won't be paged?


I was wondering the same, but was hoping the pager would be configured to
exit if the full text would fit. I have no idea if that's true for most
systems out there, though. I don't believe I've manually configured mine
that way, so I suspect it's that way be default on our custom Ubuntu. I
guess pager on by default will be pretty annoying if that is not a common
default configuration, so I really hope it is.
Augie Fackler - Feb. 18, 2017, 8:39 p.m.
> On Feb 18, 2017, at 3:11 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
>> > --- a/mercurial/commands.py
>> > +++ b/mercurial/commands.py
>> > @@ -361,6 +361,7 @@ def annotate(ui, repo, *pats, **opts):
>> >
>> >      Returns 0 on success.
>> >      """
>> > +    ui.pager('annotate')
>> >      if not pats:
>> >          raise error.Abort(_('at least one filename or pattern is required'))
>> 
>> Just to make sure. Do we plan to delay ui.pager() call so short error messages
>> (and password prompt, etc.) won't be paged?
>> 
> I was wondering the same, but was hoping the pager would be configured to exit if the full text would fit. I have no idea if that's true for most systems out there, though. I don't believe I've manually configured mine that way, so I suspect it's that way be default on our custom Ubuntu. I guess pager on by default will be pretty annoying if that is not a common default configuration, so I really hope it is.

I don’t think that’s a safe assumption, sadly. I’ll do a follow-up to try and make sure that things are sane at least at a high level.

AF
Yuya Nishihara - Feb. 19, 2017, 2:04 a.m.
On Sat, 18 Feb 2017 15:39:08 -0500, Augie Fackler wrote:
> 
> > On Feb 18, 2017, at 3:11 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> > 
> >> > --- a/mercurial/commands.py
> >> > +++ b/mercurial/commands.py
> >> > @@ -361,6 +361,7 @@ def annotate(ui, repo, *pats, **opts):
> >> >
> >> >      Returns 0 on success.
> >> >      """
> >> > +    ui.pager('annotate')
> >> >      if not pats:
> >> >          raise error.Abort(_('at least one filename or pattern is required'))
> >> 
> >> Just to make sure. Do we plan to delay ui.pager() call so short error messages
> >> (and password prompt, etc.) won't be paged?
> >> 
> > I was wondering the same, but was hoping the pager would be configured to exit if the full text would fit. I have no idea if that's true for most systems out there, though. I don't believe I've manually configured mine that way, so I suspect it's that way be default on our custom Ubuntu. I guess pager on by default will be pretty annoying if that is not a common default configuration, so I really hope it is.
> 
> I don’t think that’s a safe assumption, sadly. I’ll do a follow-up to try and make sure that things are sane at least at a high level.

IIRC, "more" exits automatically, but "less" doesn't by default. Also, "less"
clears the paged contents on exit.
Jun Wu - Feb. 19, 2017, 2:32 a.m.
Excerpts from Yuya Nishihara's message of 2017-02-19 11:04:19 +0900:
> On Sat, 18 Feb 2017 15:39:08 -0500, Augie Fackler wrote:
> > 
> > > On Feb 18, 2017, at 3:11 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> > > 
> > >> > --- a/mercurial/commands.py
> > >> > +++ b/mercurial/commands.py
> > >> > @@ -361,6 +361,7 @@ def annotate(ui, repo, *pats, **opts):
> > >> >
> > >> >      Returns 0 on success.
> > >> >      """
> > >> > +    ui.pager('annotate')
> > >> >      if not pats:
> > >> >          raise error.Abort(_('at least one filename or pattern is required'))
> > >> 
> > >> Just to make sure. Do we plan to delay ui.pager() call so short error messages
> > >> (and password prompt, etc.) won't be paged?
> > >> 
> > > I was wondering the same, but was hoping the pager would be configured to exit if the full text would fit. I have no idea if that's true for most systems out there, though. I don't believe I've manually configured mine that way, so I suspect it's that way be default on our custom Ubuntu. I guess pager on by default will be pretty annoying if that is not a common default configuration, so I really hope it is.
> > 
> > I don’t think that’s a safe assumption, sadly. I’ll do a follow-up to try and make sure that things are sane at least at a high level.
> 
> IIRC, "more" exits automatically, but "less" doesn't by default. Also, "less"
> clears the paged contents on exit.

less can be configured to not clear the screen (-X), and exit automatically
(-F). And another common flag is -R, which will allow colors.

git used to just hard-code LESS=FRX as the default in pager.c [1]. That got
moved to an override-able build variable by [2].

I guess we could do the same, provide "LESS=FRX" as the default to be more
user-friendly.

[1]: https://github.com/git/git/blob/c3b1e8d85133e2a19d372b7c166d5b49fcbbfef2/pager.c#L70
[2]: https://github.com/git/git/commit/995bc22d7f8c611e342095a211065f8585a08e65
Augie Fackler - Feb. 19, 2017, 8:03 p.m.
> On Feb 18, 2017, at 9:32 PM, Jun Wu <quark@fb.com> wrote:
> 
> Excerpts from Yuya Nishihara's message of 2017-02-19 11:04:19 +0900:
>> On Sat, 18 Feb 2017 15:39:08 -0500, Augie Fackler wrote:
>>> 
>>>> On Feb 18, 2017, at 3:11 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
>>>> 
>>>>>> --- a/mercurial/commands.py
>>>>>> +++ b/mercurial/commands.py
>>>>>> @@ -361,6 +361,7 @@ def annotate(ui, repo, *pats, **opts):
>>>>>> 
>>>>>>     Returns 0 on success.
>>>>>>     """
>>>>>> +    ui.pager('annotate')
>>>>>>     if not pats:
>>>>>>         raise error.Abort(_('at least one filename or pattern is required'))
>>>>> 
>>>>> Just to make sure. Do we plan to delay ui.pager() call so short error messages
>>>>> (and password prompt, etc.) won't be paged?
>>>>> 
>>>> I was wondering the same, but was hoping the pager would be configured to exit if the full text would fit. I have no idea if that's true for most systems out there, though. I don't believe I've manually configured mine that way, so I suspect it's that way be default on our custom Ubuntu. I guess pager on by default will be pretty annoying if that is not a common default configuration, so I really hope it is.
>>> 
>>> I don’t think that’s a safe assumption, sadly. I’ll do a follow-up to try and make sure that things are sane at least at a high level.
>> 
>> IIRC, "more" exits automatically, but "less" doesn't by default. Also, "less"
>> clears the paged contents on exit.
> 
> less can be configured to not clear the screen (-X), and exit automatically
> (-F). And another common flag is -R, which will allow colors.
> 
> git used to just hard-code LESS=FRX as the default in pager.c [1]. That got
> moved to an override-able build variable by [2].
> 
> I guess we could do the same, provide "LESS=FRX" as the default to be more
> user-friendly.

This is a great idea for built packages, but in the sources the default should be more(1) because that’s the only pager we can be sure to find.

(I expect our packages on debian would default to sensible-pager, packages on OS X would default to less with FRX set, etc)

> 
> [1]: https://github.com/git/git/blob/c3b1e8d85133e2a19d372b7c166d5b49fcbbfef2/pager.c#L70
> [2]: https://github.com/git/git/commit/995bc22d7f8c611e342095a211065f8585a08e65

Patch

diff --git a/hgext/pager.py b/hgext/pager.py
--- a/hgext/pager.py
+++ b/hgext/pager.py
@@ -110,4 +110,4 @@  def uisetup(ui):
         extensions.wrapfunction(dispatch, '_runcommand', pagecmd)
     extensions.afterloaded('color', afterloaded)
 
-attended = ['annotate', 'cat', 'diff', 'export', 'glog', 'log', 'qdiff']
+attended = ['cat', 'diff', 'export', 'glog', 'log', 'qdiff']
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -361,6 +361,7 @@  def annotate(ui, repo, *pats, **opts):
 
     Returns 0 on success.
     """
+    ui.pager('annotate')
     if not pats:
         raise error.Abort(_('at least one filename or pattern is required'))
 
diff --git a/tests/test-pager.t b/tests/test-pager.t
--- a/tests/test-pager.t
+++ b/tests/test-pager.t
@@ -164,3 +164,43 @@  Pager should not override the exit code 
   $ hg fortytwo --pager=on
   paged! '42\n'
   [42]
+
+A command that asks for paging using ui.pager() directly works:
+  $ hg blame a
+  paged! ' 0: a\n'
+  paged! ' 1: a 1\n'
+  paged! ' 2: a 2\n'
+  paged! ' 3: a 3\n'
+  paged! ' 4: a 4\n'
+  paged! ' 5: a 5\n'
+  paged! ' 6: a 6\n'
+  paged! ' 7: a 7\n'
+  paged! ' 8: a 8\n'
+  paged! ' 9: a 9\n'
+  paged! '10: a 10\n'
+but not with HGPLAIN
+  $ HGPLAIN=1 hg blame a
+   0: a
+   1: a 1
+   2: a 2
+   3: a 3
+   4: a 4
+   5: a 5
+   6: a 6
+   7: a 7
+   8: a 8
+   9: a 9
+  10: a 10
+explicit flags work too:
+  $ hg blame --pager=no a
+   0: a
+   1: a 1
+   2: a 2
+   3: a 3
+   4: a 4
+   5: a 5
+   6: a 6
+   7: a 7
+   8: a 8
+   9: a 9
+  10: a 10