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