Patchwork [4,of,5,V2] ui: echo prompt choice only if formatted output is acceptable

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 10, 2014, 3:09 p.m.
Message ID <6dcb36c1781c7afbdcee.1412953760@mimosa>
Download mbox | patch
Permalink /patch/6177/
State Changes Requested
Headers show

Comments

Yuya Nishihara - Oct. 10, 2014, 3:09 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1412950877 -32400
#      Fri Oct 10 23:21:17 2014 +0900
# Node ID 6dcb36c1781c7afbdcee20c793be20e90854e391
# Parent  80977d0b038a9a42a9b6819e77b80d343c1f711e
ui: echo prompt choice only if formatted output is acceptable

9ab18a912c44 is nice for test output, but it also affects command-server
clients or scripts that have to parse the output.  So enables it only if
formatted output is acceptable.

This should cover most of command-server clients but chg.
Mads Kiilerich - Oct. 10, 2014, 5:17 p.m.
On 10/10/2014 05:09 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1412950877 -32400
> #      Fri Oct 10 23:21:17 2014 +0900
> # Node ID 6dcb36c1781c7afbdcee20c793be20e90854e391
> # Parent  80977d0b038a9a42a9b6819e77b80d343c1f711e
> ui: echo prompt choice only if formatted output is acceptable
>
> 9ab18a912c44 is nice for test output, but it also affects command-server
> clients or scripts that have to parse the output.  So enables it only if
> formatted output is acceptable.
>
> This should cover most of command-server clients but chg.
>
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -684,7 +684,8 @@ class ui(object):
>                   r = default
>               # sometimes self.interactive disagrees with isatty,
>               # show response provided on stdin when simulating
> -            if not self._isatty(self.fin):
> +            # but not for commandserver and other scripting
> +            if not self._isatty(self.fin) and self.formatted():

This change is apparently on top of your change from self.isatty to 
self._isatty which isn't on selenic yet? I thought the change to _isatty 
already addressed the command server issue? Why is additional changes on 
top of that needed?

I liked the idea of automagically showing the selected option in the 
right cases. I do not so much like to piggy-back on ui.formatted and 
then explicitly set it all the places where we needed. Then it would be 
better to add a ui.shownonttypromptchoice configuration option ;-)

Other tools scripting Mercurial might want to see the chosen output, 
just like the test suite. I agree that other tools doing the same thing 
(such as command server) don't want it. It would perhaps not be so bad 
to have a separate option for controlling this behaviour ... but I 
really like to have it this way in the test suite.

/Mads


>                   self.write(r, "\n")
>               return r
>           except EOFError:
> diff --git a/tests/test-diff-color.t b/tests/test-diff-color.t
> --- a/tests/test-diff-color.t
> +++ b/tests/test-diff-color.t
> @@ -69,6 +69,7 @@ diffstat
>     $ cat <<EOF >> $HGRCPATH
>     > record =
>     > [ui]
> +  > formatted = true
>     > interactive = true
>     > [diff]
>     > git = True
> diff --git a/tests/test-issue3084.t b/tests/test-issue3084.t
> --- a/tests/test-issue3084.t
> +++ b/tests/test-issue3084.t
> @@ -2,6 +2,8 @@
>     $ cat <<EOF >> $HGRCPATH
>     > [extensions]
>     > largefiles =
> +  > [ui]
> +  > formatted = true
>     > EOF
>
Yuya Nishihara - Oct. 11, 2014, 5:53 a.m.
On Fri, 10 Oct 2014 19:17:41 +0200, Mads Kiilerich wrote:
> On 10/10/2014 05:09 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1412950877 -32400
> > #      Fri Oct 10 23:21:17 2014 +0900
> > # Node ID 6dcb36c1781c7afbdcee20c793be20e90854e391
> > # Parent  80977d0b038a9a42a9b6819e77b80d343c1f711e
> > ui: echo prompt choice only if formatted output is acceptable
> >
> > 9ab18a912c44 is nice for test output, but it also affects command-server
> > clients or scripts that have to parse the output.  So enables it only if
> > formatted output is acceptable.
> >
> > This should cover most of command-server clients but chg.
> >
> > diff --git a/mercurial/ui.py b/mercurial/ui.py
> > --- a/mercurial/ui.py
> > +++ b/mercurial/ui.py
> > @@ -684,7 +684,8 @@ class ui(object):
> >                   r = default
> >               # sometimes self.interactive disagrees with isatty,
> >               # show response provided on stdin when simulating
> > -            if not self._isatty(self.fin):
> > +            # but not for commandserver and other scripting
> > +            if not self._isatty(self.fin) and self.formatted():
> 
> This change is apparently on top of your change from self.isatty to 
> self._isatty which isn't on selenic yet? I thought the change to _isatty 
> already addressed the command server issue? Why is additional changes on 
> top of that needed?

self._isatty() does nothing different than util.isatty() in this case.
I wrote that patch just because self.interactive() calls self._isatty().

But, it seems the original patch was already ninja queued?

> I liked the idea of automagically showing the selected option in the 
> right cases. I do not so much like to piggy-back on ui.formatted and 
> then explicitly set it all the places where we needed. Then it would be 
> better to add a ui.shownonttypromptchoice configuration option ;-)
> 
> Other tools scripting Mercurial might want to see the chosen output, 
> just like the test suite. I agree that other tools doing the same thing 
> (such as command server) don't want it. It would perhaps not be so bad 
> to have a separate option for controlling this behaviour ... but I 
> really like to have it this way in the test suite.

Right. Some tools will want to see the choice + '\n',

    $ yes | hg xxx > output.log

but others won't

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

Regards,
Pierre-Yves David - Oct. 18, 2014, 9:59 p.m.
On 10/10/2014 10:53 PM, Yuya Nishihara wrote:
> On Fri, 10 Oct 2014 19:17:41 +0200, Mads Kiilerich wrote:
>> On 10/10/2014 05:09 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1412950877 -32400
>>> #      Fri Oct 10 23:21:17 2014 +0900
>>> # Node ID 6dcb36c1781c7afbdcee20c793be20e90854e391
>>> # Parent  80977d0b038a9a42a9b6819e77b80d343c1f711e
>>> ui: echo prompt choice only if formatted output is acceptable
>>>
>>> 9ab18a912c44 is nice for test output, but it also affects command-server
>>> clients or scripts that have to parse the output.  So enables it only if
>>> formatted output is acceptable.
>>>
>>> This should cover most of command-server clients but chg.
>>>
>>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>>> --- a/mercurial/ui.py
>>> +++ b/mercurial/ui.py
>>> @@ -684,7 +684,8 @@ class ui(object):
>>>                    r = default
>>>                # sometimes self.interactive disagrees with isatty,
>>>                # show response provided on stdin when simulating
>>> -            if not self._isatty(self.fin):
>>> +            # but not for commandserver and other scripting
>>> +            if not self._isatty(self.fin) and self.formatted():
>>
>> This change is apparently on top of your change from self.isatty to
>> self._isatty which isn't on selenic yet? I thought the change to _isatty
>> already addressed the command server issue? Why is additional changes on
>> top of that needed?
>
> self._isatty() does nothing different than util.isatty() in this case.
> I wrote that patch just because self.interactive() calls self._isatty().
>
> But, it seems the original patch was already ninja queued?
>
>> I liked the idea of automagically showing the selected option in the
>> right cases. I do not so much like to piggy-back on ui.formatted and
>> then explicitly set it all the places where we needed. Then it would be
>> better to add a ui.shownonttypromptchoice configuration option ;-)
>>
>> Other tools scripting Mercurial might want to see the chosen output,
>> just like the test suite. I agree that other tools doing the same thing
>> (such as command server) don't want it. It would perhaps not be so bad
>> to have a separate option for controlling this behaviour ... but I
>> really like to have it this way in the test suite.
>
> Right. Some tools will want to see the choice + '\n',
>
>      $ yes | hg xxx > output.log
>
> but others won't
>
>      p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
>      do_some_interactive_thing(p)
>      ...

+1 for a separated option. we need to find a way to explicitly turn it 
on for test (but only weakly so that commandserver test does not break)
Yuya Nishihara - Oct. 19, 2014, 3:33 a.m.
On Sat, 18 Oct 2014 14:59:45 -0700, Pierre-Yves David wrote:
> On 10/10/2014 10:53 PM, Yuya Nishihara wrote:
> > On Fri, 10 Oct 2014 19:17:41 +0200, Mads Kiilerich wrote:
> >> On 10/10/2014 05:09 PM, Yuya Nishihara wrote:
> >>> # HG changeset patch
> >>> # User Yuya Nishihara <yuya@tcha.org>
> >>> # Date 1412950877 -32400
> >>> #      Fri Oct 10 23:21:17 2014 +0900
> >>> # Node ID 6dcb36c1781c7afbdcee20c793be20e90854e391
> >>> # Parent  80977d0b038a9a42a9b6819e77b80d343c1f711e
> >>> ui: echo prompt choice only if formatted output is acceptable
> >>>
> >>> 9ab18a912c44 is nice for test output, but it also affects command-server
> >>> clients or scripts that have to parse the output.  So enables it only if
> >>> formatted output is acceptable.
> >>>
> >>> This should cover most of command-server clients but chg.
> >>>
> >>> diff --git a/mercurial/ui.py b/mercurial/ui.py
> >>> --- a/mercurial/ui.py
> >>> +++ b/mercurial/ui.py
> >>> @@ -684,7 +684,8 @@ class ui(object):
> >>>                    r = default
> >>>                # sometimes self.interactive disagrees with isatty,
> >>>                # show response provided on stdin when simulating
> >>> -            if not self._isatty(self.fin):
> >>> +            # but not for commandserver and other scripting
> >>> +            if not self._isatty(self.fin) and self.formatted():
> >>
> >> This change is apparently on top of your change from self.isatty to
> >> self._isatty which isn't on selenic yet? I thought the change to _isatty
> >> already addressed the command server issue? Why is additional changes on
> >> top of that needed?
> >
> > self._isatty() does nothing different than util.isatty() in this case.
> > I wrote that patch just because self.interactive() calls self._isatty().
> >
> > But, it seems the original patch was already ninja queued?
> >
> >> I liked the idea of automagically showing the selected option in the
> >> right cases. I do not so much like to piggy-back on ui.formatted and
> >> then explicitly set it all the places where we needed. Then it would be
> >> better to add a ui.shownonttypromptchoice configuration option ;-)
> >>
> >> Other tools scripting Mercurial might want to see the chosen output,
> >> just like the test suite. I agree that other tools doing the same thing
> >> (such as command server) don't want it. It would perhaps not be so bad
> >> to have a separate option for controlling this behaviour ... but I
> >> really like to have it this way in the test suite.
> >
> > Right. Some tools will want to see the choice + '\n',
> >
> >      $ yes | hg xxx > output.log
> >
> > but others won't
> >
> >      p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
> >      do_some_interactive_thing(p)
> >      ...
> 
> +1 for a separated option. we need to find a way to explicitly turn it 
> on for test (but only weakly so that commandserver test does not break)

You mean?

 1. add ui.shownonttypromptchoice which is _off_ by default
 2. enable it in our tests globally, but not for test-commandserver.t

Should I send the patch for stable?

The current tip might break scripts that do some interaction programmatically
without using commandserver.

    p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
    do_some_interactive_thing(p)  # unwanted echo-back message might break
                                  # output parsing

Regards,
Pierre-Yves David - Oct. 20, 2014, 2:45 a.m.
On 10/18/2014 08:33 PM, Yuya Nishihara wrote:
> On Sat, 18 Oct 2014 14:59:45 -0700, Pierre-Yves David wrote:
>> On 10/10/2014 10:53 PM, Yuya Nishihara wrote:
>>> On Fri, 10 Oct 2014 19:17:41 +0200, Mads Kiilerich wrote:
[…]
>>>> I liked the idea of automagically showing the selected option in the
>>>> right cases. I do not so much like to piggy-back on ui.formatted and
>>>> then explicitly set it all the places where we needed. Then it would be
>>>> better to add a ui.shownonttypromptchoice configuration option ;-)
>>>>
>>>> Other tools scripting Mercurial might want to see the chosen output,
>>>> just like the test suite. I agree that other tools doing the same thing
>>>> (such as command server) don't want it. It would perhaps not be so bad
>>>> to have a separate option for controlling this behaviour ... but I
>>>> really like to have it this way in the test suite.
>>>
>>> Right. Some tools will want to see the choice + '\n',
>>>
>>>       $ yes | hg xxx > output.log
>>>
>>> but others won't
>>>
>>>       p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
>>>       do_some_interactive_thing(p)
>>>       ...
>>
>> +1 for a separated option. we need to find a way to explicitly turn it
>> on for test (but only weakly so that commandserver test does not break)
>
> You mean?
>
>   1. add ui.shownonttypromptchoice which is _off_ by default
>   2. enable it in our tests globally, but not for test-commandserver.t
>
> Should I send the patch for stable?

Maybe. but this sound a bit clumsy. What are the use case for this echo 
thing? test readability and log?

> The current tip might break scripts that do some interaction programmatically
> without using commandserver.
>
>      p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
>      do_some_interactive_thing(p)  # unwanted echo-back message might break
>                                    # output parsing

This sounds like a regression. And we should probably fill an urgent 
ticket to not loose track of this before the final release. I'm adding 
mads kiilerich (responsible for initial patches) and Matt Mackall (great 
priest of the backward compatibility) to this thread.
Yuya Nishihara - Oct. 20, 2014, 2:48 p.m.
On Sun, 19 Oct 2014 19:45:06 -0700, Pierre-Yves David wrote:
> On 10/18/2014 08:33 PM, Yuya Nishihara wrote:
> > On Sat, 18 Oct 2014 14:59:45 -0700, Pierre-Yves David wrote:
> >> On 10/10/2014 10:53 PM, Yuya Nishihara wrote:
> >>> On Fri, 10 Oct 2014 19:17:41 +0200, Mads Kiilerich wrote:
> […]
> >>>> I liked the idea of automagically showing the selected option in the
> >>>> right cases. I do not so much like to piggy-back on ui.formatted and
> >>>> then explicitly set it all the places where we needed. Then it would be
> >>>> better to add a ui.shownonttypromptchoice configuration option ;-)
> >>>>
> >>>> Other tools scripting Mercurial might want to see the chosen output,
> >>>> just like the test suite. I agree that other tools doing the same thing
> >>>> (such as command server) don't want it. It would perhaps not be so bad
> >>>> to have a separate option for controlling this behaviour ... but I
> >>>> really like to have it this way in the test suite.
> >>>
> >>> Right. Some tools will want to see the choice + '\n',
> >>>
> >>>       $ yes | hg xxx > output.log
> >>>
> >>> but others won't
> >>>
> >>>       p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
> >>>       do_some_interactive_thing(p)
> >>>       ...
> >>
> >> +1 for a separated option. we need to find a way to explicitly turn it
> >> on for test (but only weakly so that commandserver test does not break)
> >
> > You mean?
> >
> >   1. add ui.shownonttypromptchoice which is _off_ by default
> >   2. enable it in our tests globally, but not for test-commandserver.t
> >
> > Should I send the patch for stable?
> 
> Maybe. but this sound a bit clumsy. What are the use case for this echo 
> thing? test readability and log?
> 
> > The current tip might break scripts that do some interaction programmatically
> > without using commandserver.
> >
> >      p = Popen('hg xxx', stdin=PIPE, stdout=PIPE)
> >      do_some_interactive_thing(p)  # unwanted echo-back message might break
> >                                    # output parsing
> 
> This sounds like a regression. And we should probably fill an urgent 
> ticket to not loose track of this before the final release. I'm adding 
> mads kiilerich (responsible for initial patches) and Matt Mackall (great 
> priest of the backward compatibility) to this thread.

Filed as
http://bz.selenic.com/show_bug.cgi?id=4417

I'll send patches soon.

Regards,

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -684,7 +684,8 @@  class ui(object):
                 r = default
             # sometimes self.interactive disagrees with isatty,
             # show response provided on stdin when simulating
-            if not self._isatty(self.fin):
+            # but not for commandserver and other scripting
+            if not self._isatty(self.fin) and self.formatted():
                 self.write(r, "\n")
             return r
         except EOFError:
diff --git a/tests/test-diff-color.t b/tests/test-diff-color.t
--- a/tests/test-diff-color.t
+++ b/tests/test-diff-color.t
@@ -69,6 +69,7 @@  diffstat
   $ cat <<EOF >> $HGRCPATH
   > record =
   > [ui]
+  > formatted = true
   > interactive = true
   > [diff]
   > git = True
diff --git a/tests/test-issue3084.t b/tests/test-issue3084.t
--- a/tests/test-issue3084.t
+++ b/tests/test-issue3084.t
@@ -2,6 +2,8 @@ 
   $ cat <<EOF >> $HGRCPATH
   > [extensions]
   > largefiles =
+  > [ui]
+  > formatted = true
   > EOF
 
 Create the repository outside $HOME since largefiles write to
diff --git a/tests/test-keyword.t b/tests/test-keyword.t
--- a/tests/test-keyword.t
+++ b/tests/test-keyword.t
@@ -6,6 +6,7 @@ 
   > record =
   > transplant =
   > [ui]
+  > formatted = true
   > interactive = true
   > EOF
 
diff --git a/tests/test-largefiles-update.t b/tests/test-largefiles-update.t
--- a/tests/test-largefiles-update.t
+++ b/tests/test-largefiles-update.t
@@ -3,6 +3,7 @@  directory (and ".hg/largefiles/dirstate"
 
   $ cat >> $HGRCPATH <<EOF
   > [ui]
+  > formatted = true
   > merge = internal:fail
   > [extensions]
   > largefiles =
diff --git a/tests/test-merge-prompt.t b/tests/test-merge-prompt.t
--- a/tests/test-merge-prompt.t
+++ b/tests/test-merge-prompt.t
@@ -65,6 +65,7 @@  Interactive merge:
 
   $ cat <<EOF >> $HGRCPATH
   > [ui]
+  > formatted = true
   > interactive = true
   > EOF
 
diff --git a/tests/test-mq-qrefresh-interactive.t b/tests/test-mq-qrefresh-interactive.t
--- a/tests/test-mq-qrefresh-interactive.t
+++ b/tests/test-mq-qrefresh-interactive.t
@@ -2,6 +2,7 @@  Create configuration
 
   $ cat <<EOF >> $HGRCPATH
   > [ui]
+  > formatted = true
   > interactive = true
   > EOF
 
diff --git a/tests/test-mq-subrepo.t b/tests/test-mq-subrepo.t
--- a/tests/test-mq-subrepo.t
+++ b/tests/test-mq-subrepo.t
@@ -284,6 +284,7 @@  handle subrepos safely on qrecord
   $ mkrepo repo-2499-qrecord
   $ cat <<EOF >> .hg/hgrc
   > [ui]
+  > formatted = true
   > interactive = true
   > EOF
   $ testadd qrecord -m0 0.diff <<EOF
diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
--- a/tests/test-patchbomb.t
+++ b/tests/test-patchbomb.t
@@ -67,7 +67,8 @@  Mercurial-patchbomb/.* -> Mercurial-patc
   +a
   
 
-  $ hg --config ui.interactive=1 email --confirm -n -f quux -t foo -c bar -r tip<<EOF
+  $ hg --config ui.formatted=1 --config ui.interactive=1 \
+  >   email --confirm -n -f quux -t foo -c bar -r tip <<EOF
   > n
   > EOF
   this patch series consists of 1 patches.
diff --git a/tests/test-qrecord.t b/tests/test-qrecord.t
--- a/tests/test-qrecord.t
+++ b/tests/test-qrecord.t
@@ -2,6 +2,7 @@  Create configuration
 
   $ cat <<EOF >> $HGRCPATH
   > [ui]
+  > formatted = true
   > interactive = true
   > EOF
 
diff --git a/tests/test-record.t b/tests/test-record.t
--- a/tests/test-record.t
+++ b/tests/test-record.t
@@ -2,6 +2,7 @@  Set up a repo
 
   $ cat <<EOF >> $HGRCPATH
   > [ui]
+  > formatted = true
   > interactive = true
   > [extensions]
   > record =
diff --git a/tests/test-transplant.t b/tests/test-transplant.t
--- a/tests/test-transplant.t
+++ b/tests/test-transplant.t
@@ -499,7 +499,8 @@  test interactive transplant
   |/
   o  0:17ab29e464c6
   
-  $ hg transplant -q --config ui.interactive=true -s ../t <<EOF
+  $ hg transplant -q --config ui.formatted=true --config ui.interactive=true \
+  >   -s ../t <<EOF
   > p
   > y
   > n
@@ -531,7 +532,8 @@  test interactive transplant
   |/
   o  17ab29e464c6
   
-  $ hg transplant -q --config ui.interactive=true -s ../t <<EOF
+  $ hg transplant -q --config ui.formatted=true --config ui.interactive=true \
+  >   -s ../t <<EOF
   > x
   > ?
   > y