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