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

On 10/20/2014 04:48 PM, Yuya Nishihara wrote:
> 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.

I agree that something like


probably would be better.

/Mads

On Mon, 2014-10-20 at 17:02 +0200, Mads Kiilerich wrote:
> On 10/20/2014 04:48 PM, Yuya Nishihara wrote:
> > 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.
> 
> I agree that something like
> 
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -713,7 +713,3 @@ class ui(object):
>                   r = default
> -            # sometimes self.interactive disagrees with isatty,
> -            # show response provided on stdin when simulating
> -            # but commandserver
> -            if (not util.isatty(self.fin)
> -                and not self.configbool('ui', 'nontty')):
> +            if self.configbool('ui', 'promptecho'):
>                   self.write(r, "\n")
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -682,2 +682,3 @@ class Test(unittest.TestCase):
>           hgrc.write('interactive = False\n')
> +        hgrc.write('promptecho = On\n')
>           hgrc.write('mergemarkers = detailed\n')
> 
> probably would be better.

Exactly. I'll go ahead and queue this fix.