Patchwork [2,of,2,STABLE] ui: enable echo back of prompt response only in tests (issue4417)

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 20, 2014, 3:15 p.m.
Message ID <c5b0405c9f23896ad444.1413818148@mimosa>
Download mbox | patch
Permalink /patch/6430/
State Accepted
Headers show

Comments

Yuya Nishihara - Oct. 20, 2014, 3:15 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1413694842 -32400
#      Sun Oct 19 14:00:42 2014 +0900
# Branch stable
# Node ID c5b0405c9f23896ad4440899836f13a18e9d9390
# Parent  7ce8b804b319af65322c0ab345f21d289f92c63f
ui: enable echo back of prompt response only in tests (issue4417)

9ab18a912c44 isn't always good.  Some tools will want to see the prompt
choice:

    $ yes | hg xxx > output.log

but others won't:

    p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
    do_some_interactive_thing(p)

Since it could break output parsing of existing scripts, ui.simulatettyecho
is changed to be off by default.
Mads Kiilerich - Oct. 20, 2014, 3:32 p.m.
On 10/20/2014 05:15 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1413694842 -32400
> #      Sun Oct 19 14:00:42 2014 +0900
> # Branch stable
> # Node ID c5b0405c9f23896ad4440899836f13a18e9d9390
> # Parent  7ce8b804b319af65322c0ab345f21d289f92c63f
> ui: enable echo back of prompt response only in tests (issue4417)
>
> 9ab18a912c44 isn't always good.  Some tools will want to see the prompt
> choice:
>
>      $ yes | hg xxx > output.log
>
> but others won't:
>
>      p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
>      do_some_interactive_thing(p)
>
> Since it could break output parsing of existing scripts, ui.simulatettyecho
> is changed to be off by default.
>
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -196,9 +196,6 @@ class server(object):
>           for ui in uis:
>               # any kind of interaction must use server channels
>               ui.setconfig('ui', 'nontty', 'true', 'commandserver')
> -            # echo-back message shouldn't be written because a client
> -            # typically have to parse the output
> -            ui.setconfig('ui', 'simulatettyecho', 'false', 'commandserver')
>   
>           req = dispatch.request(args[:], copiedui, self.repo, self.cin,
>                                  self.cout, self.cerr)
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -714,7 +714,7 @@ class ui(object):
>               # sometimes self.interactive disagrees with isatty,
>               # show response provided on stdin when simulating
>               if (not util.isatty(self.fin)
> -                and self.configbool('ui', 'simulatettyecho', True)):
> +                and self.configbool('ui', 'simulatettyecho')):

It is a config setting with one specific purpose. People who ask for it 
should get it. I would just trust it, without considering isatty.

I would prefer to name the config setting something that is more to the 
point about what it actually does, not so much how it could be 
perceived. It _is_ actually doing something, not just simulating it ;-)

The new config setting should also have some brief documentation.

It could be argued that this setting also should control the display of 
default choices when not interactive. That would perhaps be more 
consistent ... but not big deal.

Finally, I would fold these two patches into one.

/Mads
Yuya Nishihara - Oct. 20, 2014, 4:09 p.m.
On Mon, 20 Oct 2014 17:32:14 +0200, Mads Kiilerich wrote:
> On 10/20/2014 05:15 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1413694842 -32400
> > #      Sun Oct 19 14:00:42 2014 +0900
> > # Branch stable
> > # Node ID c5b0405c9f23896ad4440899836f13a18e9d9390
> > # Parent  7ce8b804b319af65322c0ab345f21d289f92c63f
> > ui: enable echo back of prompt response only in tests (issue4417)
> >
> > 9ab18a912c44 isn't always good.  Some tools will want to see the prompt
> > choice:
> >
> >      $ yes | hg xxx > output.log
> >
> > but others won't:
> >
> >      p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
> >      do_some_interactive_thing(p)
> >
> > Since it could break output parsing of existing scripts, ui.simulatettyecho
> > is changed to be off by default.
> >
> > diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> > --- a/mercurial/commandserver.py
> > +++ b/mercurial/commandserver.py
> > @@ -196,9 +196,6 @@ class server(object):
> >           for ui in uis:
> >               # any kind of interaction must use server channels
> >               ui.setconfig('ui', 'nontty', 'true', 'commandserver')
> > -            # echo-back message shouldn't be written because a client
> > -            # typically have to parse the output
> > -            ui.setconfig('ui', 'simulatettyecho', 'false', 'commandserver')
> >   
> >           req = dispatch.request(args[:], copiedui, self.repo, self.cin,
> >                                  self.cout, self.cerr)
> > diff --git a/mercurial/ui.py b/mercurial/ui.py
> > --- a/mercurial/ui.py
> > +++ b/mercurial/ui.py
> > @@ -714,7 +714,7 @@ class ui(object):
> >               # sometimes self.interactive disagrees with isatty,
> >               # show response provided on stdin when simulating
> >               if (not util.isatty(self.fin)
> > -                and self.configbool('ui', 'simulatettyecho', True)):
> > +                and self.configbool('ui', 'simulatettyecho')):
> 
> It is a config setting with one specific purpose. People who ask for it 
> should get it. I would just trust it, without considering isatty.
> 
> I would prefer to name the config setting something that is more to the 
> point about what it actually does, not so much how it could be 
> perceived. It _is_ actually doing something, not just simulating it ;-)

Okay, I'll change it to "ui.promptecho" option that prints response no matter
if it is a tty.

> The new config setting should also have some brief documentation.

Should it?
I think it's internal flag like ui.nontty.

> It could be argued that this setting also should control the display of
> default choices when not interactive. That would perhaps be more 
> consistent ... but not big deal.

It would be, but shouldn't be in stable branch.

> Finally, I would fold these two patches into one.

Agreed.

Regards,
Pierre-Yves David - Oct. 26, 2014, 12:08 p.m.
On 10/20/2014 06:09 PM, Yuya Nishihara wrote:
> On Mon, 20 Oct 2014 17:32:14 +0200, Mads Kiilerich wrote:
>> On 10/20/2014 05:15 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1413694842 -32400
>>> #      Sun Oct 19 14:00:42 2014 +0900
>>> # Branch stable
>>> # Node ID c5b0405c9f23896ad4440899836f13a18e9d9390
>>> # Parent  7ce8b804b319af65322c0ab345f21d289f92c63f
>>> ui: enable echo back of prompt response only in tests (issue4417)

This has apparently been pushed as 5ba11ab48fcf315943225b22601dded5cc1ad8b7

Dropping the series on patchwork.

Patch

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -196,9 +196,6 @@  class server(object):
         for ui in uis:
             # any kind of interaction must use server channels
             ui.setconfig('ui', 'nontty', 'true', 'commandserver')
-            # echo-back message shouldn't be written because a client
-            # typically have to parse the output
-            ui.setconfig('ui', 'simulatettyecho', 'false', 'commandserver')
 
         req = dispatch.request(args[:], copiedui, self.repo, self.cin,
                                self.cout, self.cerr)
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -714,7 +714,7 @@  class ui(object):
             # sometimes self.interactive disagrees with isatty,
             # show response provided on stdin when simulating
             if (not util.isatty(self.fin)
-                and self.configbool('ui', 'simulatettyecho', True)):
+                and self.configbool('ui', 'simulatettyecho')):
                 self.write(r, "\n")
             return r
         except EOFError:
diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -681,6 +681,7 @@  class Test(unittest.TestCase):
         hgrc.write('slash = True\n')
         hgrc.write('interactive = False\n')
         hgrc.write('mergemarkers = detailed\n')
+        hgrc.write('simulatettyecho = True\n')
         hgrc.write('[defaults]\n')
         hgrc.write('backout = -d "0 0"\n')
         hgrc.write('commit = -d "0 0"\n')
diff --git a/tests/test-basic.t b/tests/test-basic.t
--- a/tests/test-basic.t
+++ b/tests/test-basic.t
@@ -8,6 +8,7 @@  Create a repository:
   ui.slash=True
   ui.interactive=False
   ui.mergemarkers=detailed
+  ui.simulatettyecho=True
   $ hg init t
   $ cd t
 
diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
--- a/tests/test-commandserver.t
+++ b/tests/test-commandserver.t
@@ -5,6 +5,12 @@ 
 #endif
   $ export PYTHONPATH
 
+typical client does not want echo-back messages, so test without
+ui.simulatettyecho:
+
+  $ grep -v '^simulatettyecho ' < $HGRCPATH >> $HGRCPATH.new
+  $ mv $HGRCPATH.new $HGRCPATH
+
   $ hg init repo
   $ cd repo
 
@@ -178,7 +184,6 @@  check that local configs for the cached 
   ui.mergemarkers=detailed
   ui.foo=bar
   ui.nontty=true
-  ui.simulatettyecho=false
   *** runcommand init foo
   *** runcommand -R foo showconfig ui defaults
   defaults.backout=-d "0 0"
@@ -189,7 +194,6 @@  check that local configs for the cached 
   ui.interactive=False
   ui.mergemarkers=detailed
   ui.nontty=true
-  ui.simulatettyecho=false
 
   $ rm -R foo