Patchwork [V3] pager: fix the invocation of `more` on Windows

login
register
mail settings
Submitter Matt Harbison
Date March 24, 2017, 1:57 a.m.
Message ID <d0c2db2d9f13dca534c5.1490320670@Envy>
Download mbox | patch
Permalink /patch/19615/
State Accepted
Headers show

Comments

Matt Harbison - March 24, 2017, 1:57 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1489983573 14400
#      Mon Mar 20 00:19:33 2017 -0400
# Node ID d0c2db2d9f13dca534c598de050eb1919ef79059
# Parent  df82f375fa005b9c71b463182e6b54aa47fa5999
pager: fix the invocation of `more` on Windows

After 9335dc6b2a9c, with 'shell' being (mostly) set to False, invoking `more` no
longer worked.  Instead, a warning was printed and the pager was disabled.
Invoking `more.com` works.  Since a user may have configured 'pager.pager=more',
do this substitution at the end.  Surprisingly, `more` does allow for arguments,
so those are preserved.  This also allows `more` to work in MSYS.

Setting 'shell=False' runs the executable via CreateProcess(), which has rather
wonky rules for resolving an executable without an extension [1].  Resolving to
*.com is not among them.  Since 'shell=True' yields a cryptic error for a bad
$PAGER, and a *.exe program will work without specifying the extension, sticking
with current 'shell=False' seems like the right thing to do.  I don't think
there are any other *.com pagers out there, so this one special case seems OK.

If somebody wants to do something crazy that requires cmd.exe, I was able to get
normal paged output with 'pager.pager="cmd.exe /c more"'.  I assume you can
replace `more` with *.bat, *.vbs or various other creatures listed in $PATHEXT.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx
Augie Fackler - March 24, 2017, 2:28 a.m.
> On Mar 23, 2017, at 9:57 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1489983573 14400
> #      Mon Mar 20 00:19:33 2017 -0400
> # Node ID d0c2db2d9f13dca534c598de050eb1919ef79059
> # Parent  df82f375fa005b9c71b463182e6b54aa47fa5999
> pager: fix the invocation of `more` on Windows

Queued, thanks much for your work on Windows support.

> 
> After 9335dc6b2a9c, with 'shell' being (mostly) set to False, invoking `more` no
> longer worked.  Instead, a warning was printed and the pager was disabled.
> Invoking `more.com` works.  Since a user may have configured 'pager.pager=more',
> do this substitution at the end.  Surprisingly, `more` does allow for arguments,
> so those are preserved.  This also allows `more` to work in MSYS.
> 
> Setting 'shell=False' runs the executable via CreateProcess(), which has rather
> wonky rules for resolving an executable without an extension [1].  Resolving to
> *.com is not among them.  Since 'shell=True' yields a cryptic error for a bad
> $PAGER, and a *.exe program will work without specifying the extension, sticking
> with current 'shell=False' seems like the right thing to do.  I don't think
> there are any other *.com pagers out there, so this one special case seems OK.
> 
> If somebody wants to do something crazy that requires cmd.exe, I was able to get
> normal paged output with 'pager.pager="cmd.exe /c more"'.  I assume you can
> replace `more` with *.bat, *.vbs or various other creatures listed in $PATHEXT.
> 
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -844,6 +844,15 @@
>         if not pagercmd:
>             return
> 
> +        if pycompat.osname == 'nt':
> +            # `more` cannot be invoked with shell=False, but `more.com` can.
> +            # Hide this implementation detail from the user, so we can also get
> +            # sane bad PAGER behavior.  If args are also given, the space in the
> +            # command line forces shell=True, so that case doesn't need to be
> +            # handled here.
> +            if pagercmd == 'more':
> +                pagercmd = 'more.com'
> +
>         self.debug('starting pager for command %r\n' % command)
>         self.flush()
>         self.pageractive = True
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - March 24, 2017, 2:17 p.m.
On Thu, 23 Mar 2017 21:57:50 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1489983573 14400
> #      Mon Mar 20 00:19:33 2017 -0400
> # Node ID d0c2db2d9f13dca534c598de050eb1919ef79059
> # Parent  df82f375fa005b9c71b463182e6b54aa47fa5999
> pager: fix the invocation of `more` on Windows

> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -844,6 +844,15 @@
>          if not pagercmd:
>              return
>  
> +        if pycompat.osname == 'nt':
> +            # `more` cannot be invoked with shell=False, but `more.com` can.
> +            # Hide this implementation detail from the user, so we can also get
> +            # sane bad PAGER behavior.  If args are also given, the space in the
> +            # command line forces shell=True, so that case doesn't need to be
> +            # handled here.
> +            if pagercmd == 'more':
> +                pagercmd = 'more.com'

Given MSYS nor MSYS2 doesn't provide more.exe by default, this seems fine.
Can we document this hack in help just in case someone installed more.exe?
Matt Harbison - March 25, 2017, 12:16 a.m.
On Fri, 24 Mar 2017 10:17:46 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Thu, 23 Mar 2017 21:57:50 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1489983573 14400
>> #      Mon Mar 20 00:19:33 2017 -0400
>> # Node ID d0c2db2d9f13dca534c598de050eb1919ef79059
>> # Parent  df82f375fa005b9c71b463182e6b54aa47fa5999
>> pager: fix the invocation of `more` on Windows
>
>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>> --- a/mercurial/ui.py
>> +++ b/mercurial/ui.py
>> @@ -844,6 +844,15 @@
>>          if not pagercmd:
>>              return
>>
>> +        if pycompat.osname == 'nt':
>> +            # `more` cannot be invoked with shell=False, but  
>> `more.com` can.
>> +            # Hide this implementation detail from the user, so we can  
>> also get
>> +            # sane bad PAGER behavior.  If args are also given, the  
>> space in the
>> +            # command line forces shell=True, so that case doesn't  
>> need to be
>> +            # handled here.
>> +            if pagercmd == 'more':
>> +                pagercmd = 'more.com'
>
> Given MSYS nor MSYS2 doesn't provide more.exe by default, this seems  
> fine.
> Can we document this hack in help just in case someone installed  
> more.exe?

I know this is an edge case, but instead of documenting how a user can't  
just say 'more' and spawn 'more.exe', what do you think about calling  
util.findexe() if pagercmd == 'more'?  That finds 'more.com' too, so  
things should just work as if they were in cmd.exe.
Yuya Nishihara - March 25, 2017, 1:44 a.m.
On Fri, 24 Mar 2017 20:16:30 -0400, Matt Harbison wrote:
> On Fri, 24 Mar 2017 10:17:46 -0400, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Thu, 23 Mar 2017 21:57:50 -0400, Matt Harbison wrote:
> >> +            # handled here.
> >> +            if pagercmd == 'more':
> >> +                pagercmd = 'more.com'
> >
> > Given MSYS nor MSYS2 doesn't provide more.exe by default, this seems  
> > fine.
> > Can we document this hack in help just in case someone installed  
> > more.exe?
> 
> I know this is an edge case, but instead of documenting how a user can't
> just say 'more' and spawn 'more.exe', what do you think about calling
> util.findexe() if pagercmd == 'more'?  That finds 'more.com' too, so
> things should just work as if they were in cmd.exe.

Yeah, that sounds much better.
We can simply do 'if nt and not shell: findexe()'.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -844,6 +844,15 @@ 
         if not pagercmd:
             return
 
+        if pycompat.osname == 'nt':
+            # `more` cannot be invoked with shell=False, but `more.com` can.
+            # Hide this implementation detail from the user, so we can also get
+            # sane bad PAGER behavior.  If args are also given, the space in the
+            # command line forces shell=True, so that case doesn't need to be
+            # handled here.
+            if pagercmd == 'more':
+                pagercmd = 'more.com'
+
         self.debug('starting pager for command %r\n' % command)
         self.flush()
         self.pageractive = True