Patchwork [stable,v2] progress: disable progress when the terminal resembles cmd.exe (issue4801)

login
register
mail settings
Submitter Augie Fackler
Date March 1, 2016, 7:42 p.m.
Message ID <301d88f9aeeae128fa8a.1456861349@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/13509/
State Deferred
Delegated to: Yuya Nishihara
Headers show

Comments

Augie Fackler - March 1, 2016, 7:42 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1456860899 18000
#      Tue Mar 01 14:34:59 2016 -0500
# Branch stable
# Node ID 301d88f9aeeae128fa8a4e15303f4ac64c7afc69
# Parent  5f95d6a70e9b37bf11487580a5c750d861baa8f5
# EXP-Topic progress-cmd
progress: disable progress when the terminal resembles cmd.exe (issue4801)

It appears that cmd.exe is hopelessly unreliable when it comes to
handling carriage returns, based on my own web searching. The blessed
mechanism for performing cursor positioning appears to be
SetConsoleCursorPosition(), but I don't have easy access to a Windows
machine, and we need a simple fix for stable anyway.

Ideally, someone on Windows can write a fix for this and contribute it.
Simon King - March 1, 2016, 9:19 p.m.
> On 1 Mar 2016, at 19:42, Augie Fackler <raf@durin42.com> wrote:
> 
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1456860899 18000
> #      Tue Mar 01 14:34:59 2016 -0500
> # Branch stable
> # Node ID 301d88f9aeeae128fa8a4e15303f4ac64c7afc69
> # Parent  5f95d6a70e9b37bf11487580a5c750d861baa8f5
> # EXP-Topic progress-cmd
> progress: disable progress when the terminal resembles cmd.exe (issue4801)
> 
> It appears that cmd.exe is hopelessly unreliable when it comes to
> handling carriage returns, based on my own web searching. The blessed
> mechanism for performing cursor positioning appears to be
> SetConsoleCursorPosition(), but I don't have easy access to a Windows
> machine, and we need a simple fix for stable anyway.
> 
> Ideally, someone on Windows can write a fix for this and contribute it.
> 
> diff --git a/mercurial/progress.py b/mercurial/progress.py
> --- a/mercurial/progress.py
> +++ b/mercurial/progress.py
> @@ -7,6 +7,7 @@
> 
> from __future__ import absolute_import
> 
> +import os
> import sys
> import threading
> import time
> @@ -18,8 +19,17 @@ def spacejoin(*args):
>     return ' '.join(s for s in args if s)
> 
> def shouldprint(ui):
> -    return not (ui.quiet or ui.plain()) and (
> -        ui._isatty(sys.stderr) or ui.configbool('progress', 'assume-tty'))
> +    if ui.configbool('progress', 'assume-tty'):
> +        return True

At the moment, either ui.quiet or ui.plain() are enough to disable progress. With this change, progress.assume-tty would override them. Was that intentional?

Simon

> +    if os.name == 'nt' and 'TERM' not in os.environ:
> +        # We think this is cmd.exe, which means we're going to skip
> +        # progress. It appears that carriage returns are unreliable
> +        # for cursor positioning in cmd.exe, and that we should
> +        # instead be using SetConsoleCursorPosition() to move the
> +        # cursor for progress writes. See issue4801 for the report
> +        # that revealed this problem.
> +        return False
> +    return not (ui.quiet or ui.plain()) and ui._isatty(sys.stderr)
> 
> def fmtremaining(seconds):
>     """format a number of remaining seconds in human readable way
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - March 1, 2016, 9:23 p.m.
> On Mar 1, 2016, at 13:19, Simon King <simon@simonking.org.uk> wrote:
> 
>> On 1 Mar 2016, at 19:42, Augie Fackler <raf@durin42.com> wrote:
>> 
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1456860899 18000
>> #      Tue Mar 01 14:34:59 2016 -0500
>> # Branch stable
>> # Node ID 301d88f9aeeae128fa8a4e15303f4ac64c7afc69
>> # Parent  5f95d6a70e9b37bf11487580a5c750d861baa8f5
>> # EXP-Topic progress-cmd
>> progress: disable progress when the terminal resembles cmd.exe (issue4801)
>> 
>> It appears that cmd.exe is hopelessly unreliable when it comes to
>> handling carriage returns, based on my own web searching. The blessed
>> mechanism for performing cursor positioning appears to be
>> SetConsoleCursorPosition(), but I don't have easy access to a Windows
>> machine, and we need a simple fix for stable anyway.
>> 
>> Ideally, someone on Windows can write a fix for this and contribute it.
>> 
>> diff --git a/mercurial/progress.py b/mercurial/progress.py
>> --- a/mercurial/progress.py
>> +++ b/mercurial/progress.py
>> @@ -7,6 +7,7 @@
>> 
>> from __future__ import absolute_import
>> 
>> +import os
>> import sys
>> import threading
>> import time
>> @@ -18,8 +19,17 @@ def spacejoin(*args):
>>    return ' '.join(s for s in args if s)
>> 
>> def shouldprint(ui):
>> -    return not (ui.quiet or ui.plain()) and (
>> -        ui._isatty(sys.stderr) or ui.configbool('progress', 'assume-tty'))
>> +    if ui.configbool('progress', 'assume-tty'):
>> +        return True
> 
> At the moment, either ui.quiet or ui.plain() are enough to disable progress. With this change, progress.assume-tty would override them. Was that intentional?

Partially. The principle use of assume-tty is for testing progress, so I'm not super worried about changing the semantics of that.

Matt, do you feel strongly about that? (Other concerns about this patch aside.)
Matt Mackall - March 1, 2016, 11:41 p.m.
On Tue, 2016-03-01 at 13:23 -0800, Augie Fackler wrote:
> > On Mar 1, 2016, at 13:19, Simon King <simon@simonking.org.uk> wrote:
> > 
> > > On 1 Mar 2016, at 19:42, Augie Fackler <raf@durin42.com> wrote:
> > > 
> > > # HG changeset patch
> > > # User Augie Fackler <augie@google.com>
> > > # Date 1456860899 18000
> > > #      Tue Mar 01 14:34:59 2016 -0500
> > > # Branch stable
> > > # Node ID 301d88f9aeeae128fa8a4e15303f4ac64c7afc69
> > > # Parent  5f95d6a70e9b37bf11487580a5c750d861baa8f5
> > > # EXP-Topic progress-cmd
> > > progress: disable progress when the terminal resembles cmd.exe (issue4801)
> > > 
> > > It appears that cmd.exe is hopelessly unreliable when it comes to
> > > handling carriage returns, based on my own web searching. The blessed
> > > mechanism for performing cursor positioning appears to be
> > > SetConsoleCursorPosition(), but I don't have easy access to a Windows
> > > machine, and we need a simple fix for stable anyway.
> > > 
> > > Ideally, someone on Windows can write a fix for this and contribute it.
> > > 
> > > diff --git a/mercurial/progress.py b/mercurial/progress.py
> > > --- a/mercurial/progress.py
> > > +++ b/mercurial/progress.py
> > > @@ -7,6 +7,7 @@
> > > 
> > > from __future__ import absolute_import
> > > 
> > > +import os
> > > import sys
> > > import threading
> > > import time
> > > @@ -18,8 +19,17 @@ def spacejoin(*args):
> > >    return ' '.join(s for s in args if s)
> > > 
> > > def shouldprint(ui):
> > > -    return not (ui.quiet or ui.plain()) and (
> > > -        ui._isatty(sys.stderr) or ui.configbool('progress', 'assume-
> > > tty'))
> > > +    if ui.configbool('progress', 'assume-tty'):
> > > +        return True
> > 
> > At the moment, either ui.quiet or ui.plain() are enough to disable progress.
> > With this change, progress.assume-tty would override them. Was that
> > intentional?
> 
> Partially. The principle use of assume-tty is for testing progress, so I'm not
> super worried about changing the semantics of that.
> 
> Matt, do you feel strongly about that? (Other concerns about this patch
> aside.)

Timeless and I made a fair amount of progress tracking this down today but timed
out. I think I'm going to leave this issue for the next release.
Augie Fackler - March 1, 2016, 11:42 p.m.
> On Mar 1, 2016, at 15:41, Matt Mackall <mpm@selenic.com> wrote:
> 
> On Tue, 2016-03-01 at 13:23 -0800, Augie Fackler wrote:
>>> On Mar 1, 2016, at 13:19, Simon King <simon@simonking.org.uk> wrote:
>> Partially. The principle use of assume-tty is for testing progress, so I'm not
>> super worried about changing the semantics of that.
>> 
>> Matt, do you feel strongly about that? (Other concerns about this patch
>> aside.)
> 
> Timeless and I made a fair amount of progress tracking this down today but timed
> out. I think I'm going to leave this issue for the next release.

Fair enough. Can I get you to update the bug with whatever you've figured out in case I get motivated again?
Sean Farley - March 1, 2016, 11:49 p.m.
Matt Mackall <mpm@selenic.com> writes:

> On Tue, 2016-03-01 at 13:23 -0800, Augie Fackler wrote:
>> > On Mar 1, 2016, at 13:19, Simon King <simon@simonking.org.uk> wrote:
>> > 
>> > > On 1 Mar 2016, at 19:42, Augie Fackler <raf@durin42.com> wrote:
>> > > 
>> > > # HG changeset patch
>> > > # User Augie Fackler <augie@google.com>
>> > > # Date 1456860899 18000
>> > > #Tue Mar 01 14:34:59 2016 -0500
>> > > # Branch stable
>> > > # Node ID 301d88f9aeeae128fa8a4e15303f4ac64c7afc69
>> > > # Parent5f95d6a70e9b37bf11487580a5c750d861baa8f5
>> > > # EXP-Topic progress-cmd
>> > > progress: disable progress when the terminal resembles cmd.exe (issue4801)
>> > > 
>> > > It appears that cmd.exe is hopelessly unreliable when it comes to
>> > > handling carriage returns, based on my own web searching. The blessed
>> > > mechanism for performing cursor positioning appears to be
>> > > SetConsoleCursorPosition(), but I don't have easy access to a Windows
>> > > machine, and we need a simple fix for stable anyway.
>> > > 
>> > > Ideally, someone on Windows can write a fix for this and contribute it.
>> > > 
>> > > diff --git a/mercurial/progress.py b/mercurial/progress.py
>> > > --- a/mercurial/progress.py
>> > > +++ b/mercurial/progress.py
>> > > @@ -7,6 +7,7 @@
>> > > 
>> > > from __future__ import absolute_import
>> > > 
>> > > +import os
>> > > import sys
>> > > import threading
>> > > import time
>> > > @@ -18,8 +19,17 @@ def spacejoin(*args):
>> > > return ' '.join(s for s in args if s)
>> > > 
>> > > def shouldprint(ui):
>> > > -return not (ui.quiet or ui.plain()) and (
>> > > -ui._isatty(sys.stderr) or ui.configbool('progress', 'assume-
>> > > tty'))
>> > > +if ui.configbool('progress', 'assume-tty'):
>> > > +return True
>> > 
>> > At the moment, either ui.quiet or ui.plain() are enough to disable progress.
>> > With this change, progress.assume-tty would override them. Was that
>> > intentional?
>> 
>> Partially. The principle use of assume-tty is for testing progress, so I'm not
>> super worried about changing the semantics of that.
>> 
>> Matt, do you feel strongly about that? (Other concerns about this patch
>> aside.)
>
> Timeless and I made a fair amount of progress tracking this down today but timed
                                       ^^^^^^^^ best pun of the day

Patch

diff --git a/mercurial/progress.py b/mercurial/progress.py
--- a/mercurial/progress.py
+++ b/mercurial/progress.py
@@ -7,6 +7,7 @@ 
 
 from __future__ import absolute_import
 
+import os
 import sys
 import threading
 import time
@@ -18,8 +19,17 @@  def spacejoin(*args):
     return ' '.join(s for s in args if s)
 
 def shouldprint(ui):
-    return not (ui.quiet or ui.plain()) and (
-        ui._isatty(sys.stderr) or ui.configbool('progress', 'assume-tty'))
+    if ui.configbool('progress', 'assume-tty'):
+        return True
+    if os.name == 'nt' and 'TERM' not in os.environ:
+        # We think this is cmd.exe, which means we're going to skip
+        # progress. It appears that carriage returns are unreliable
+        # for cursor positioning in cmd.exe, and that we should
+        # instead be using SetConsoleCursorPosition() to move the
+        # cursor for progress writes. See issue4801 for the report
+        # that revealed this problem.
+        return False
+    return not (ui.quiet or ui.plain()) and ui._isatty(sys.stderr)
 
 def fmtremaining(seconds):
     """format a number of remaining seconds in human readable way