I added value constructor TermConfig to type constructor
PromptConfig. Then I modified promptChar so that values
made from TermConfig take input from tty.

Then in SelectChanges.hs I modified promptUser so that
it uses TermConfig if the command is apply --interactive.

It passed all tests. However I could not figure out on how
to write a test for this patch.

Attachments

• patch

I'm a bit concerned that this leads to inconsistent behaviour between 
different darcs commands, leading to potential user surprise.

The main alternative would be to introduce a flag - this probably needs 
a bit of discussion on the mailing list to confirm that it's worth 
complicating the UI. There's a standard template on the wiki for 
"opposing" such features to make sure that they are really needed: 
http://wiki.darcs.net/Ideas#proposal-gauntlet - if you're happy with the 
idea of introducing a flag, would you mind starting that discussion?

Sorry to keep pushing back on your first patch!

I also have a few technical comments on the patch:

 - There's a stray putStrLn "TermConfig"
 - There's a new warning in Darcs.Util caused by the TermConfig case not 
being covered. I guess this could lead to an actual runtime error so 
should definitely be fixed.

On Fri, Apr 1, 2011 at 7:15 PM, Ganesh Sittampalam <bugs@darcs.net> wrote:

>
> Ganesh Sittampalam <ganesh@earth.li> added the comment:
>
> I'm a bit concerned that this leads to inconsistent behaviour between
> different darcs commands, leading to potential user surprise.
>
>
As I understand it, the inconsistency is that --interactive takes input
from tty instead of stdin in the case of apply.(for all other commands,
it is stdin)


> The main alternative would be to introduce a flag - this probably needs
> a bit of discussion on the mailing list to confirm that it's worth
> complicating the UI. There's a standard template on the wiki for
> "opposing" such features to make sure that they are really needed:
> http://wiki.darcs.net/Ideas#proposal-gauntlet - if you're happy with the
> idea of introducing a flag, would you mind starting that discussion?
>
> So, the flag should be something like --ttyinteractive, which takes input
from tty for all commands?

Sorry to keep pushing back on your first patch!
>
> I also have a few technical comments on the patch:
>
>  - There's a stray putStrLn "TermConfig"
>  - There's a new warning in Darcs.Util caused by the TermConfig case not
> being covered. I guess this could lead to an actual runtime error so
> should definitely be fixed.
>
>
I got rid of these two.(Note that there possibly can't be a runtime error,
unless you nested TermConfig.)

> ----------
> assignedto: ganesh -> bsrkaditya
> status: needs-review -> followup-requested
>
>

The crux of the problem, is that darcs apply --interactive has two
inputs. One for the patch, and one for interactive. The patch could
be given by specifying the file name, or by stdin. interactive can take
input only from stdin(until now). This is causing a breakage if
you try to take patch input from
stdin(issue1648<http://bugs.darcs.net/issue1648>
).


Another facet of the problem, is tests. There are many tests for
--interactive in darcs repo, which take input from stdin. None of them
are for apply.
 (But there could be a hypothetical future test, where apply --interactive
should be tested. The tester give the file name of the patch in the command,
and interactive choices from stdin. You can't do this if my patch is
applied,
unless you somehow 'forbid' tty input in the test)

Sorry for the long delay in the follow up. I am having an insane semester
back at college,(april is the last month).
In any case, april 29 is my last exam. So I will have a quicker response
time
after that.

__________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch567>
> __________________________________
>



-- 
BSRK Aditya

Attachments

• resolve-issue1648_-darcs-now-uses-tty-instead-of-stdin-in-darcs-apply-__interactive.dpatch
• unnamed

>
> Ganesh Sittampalam <ganesh@earth.li> added the comment:
>
> I'm a bit concerned that this leads to inconsistent behaviour between
> different darcs commands, leading to potential user surprise.
>
>
As I understand it, the inconsistency is that --interactive takes input
from tty instead of stdin in the case of apply.(for all other commands,
it is stdin)


> The main alternative would be to introduce a flag - this probably 
needs
> a bit of discussion on the mailing list to confirm that it's worth
> complicating the UI. There's a standard template on the wiki for
> "opposing" such features to make sure that they are really needed:
> http://wiki.darcs.net/Ideas#proposal-gauntlet - if you're happy with 
the
> idea of introducing a flag, would you mind starting that discussion?
>

 So, the flag should be something like --ttyinteractive, which takes 
input from tty for all commands?

>Sorry to keep pushing back on your first patch!
>
> I also have a few technical comments on the patch:
>
>  - There's a stray putStrLn "TermConfig"
>  - There's a new warning in Darcs.Util caused by the TermConfig case 
not
> being covered. I guess this could lead to an actual runtime error so
> should definitely be fixed.
>
>
I got rid of these two.(Note that there possibly can't be a runtime 
error,
unless you nested TermConfig.)

> ----------
> assignedto: ganesh -> bsrkaditya
> status: needs-review -> followup-requested
>
>

The crux of the problem, is that darcs apply --interactive has two
inputs. One for the patch, and one for interactive. The patch could
be given by specifying the file name, or by stdin. interactive can take
input only from stdin(until now). This is causing a breakage if
you try to take patch input from
stdin(issue1648<http://bugs.darcs.net/issue1648>
).


Another facet of the problem, is tests. There are many tests for
--interactive in darcs repo, which take input from stdin. None of them
are for apply.
 (But there could be a hypothetical future test, where apply --
interactive
should be tested. The tester give the file name of the patch in the 
command,
and interactive choices from stdin. You can't do this if my patch is
applied,
unless you somehow 'forbid' tty input in the test)

Sorry for the long delay in the follow up. I am having an insane 
semester
back at college,(april is the last month).
In any case, april 29 is my last exam. So I will have a quicker response
time
after that.

Hi,

Sorry for the long delay in replying to this - been really busy myself.

On 19/04/2011 16:56, BSRK Aditya wrote:
> 
> BSRK Aditya <bsrkaditya@gmail.com> added the comment:
> 
>>
>> Ganesh Sittampalam <ganesh@earth.li> added the comment:
>>
>> I'm a bit concerned that this leads to inconsistent behaviour between
>> different darcs commands, leading to potential user surprise.
>>
>>
> As I understand it, the inconsistency is that --interactive takes input
> from tty instead of stdin in the case of apply.(for all other commands,
> it is stdin)

Yep.

>> The main alternative would be to introduce a flag - this probably 
> needs
>> a bit of discussion on the mailing list to confirm that it's worth
>> complicating the UI. There's a standard template on the wiki for
>> "opposing" such features to make sure that they are really needed:
>> http://wiki.darcs.net/Ideas#proposal-gauntlet - if you're happy with 
> the
>> idea of introducing a flag, would you mind starting that discussion?
>>
> 
>  So, the flag should be something like --ttyinteractive, which takes 
> input from tty for all commands?

Yes, I think so. I'm not quite sure what it should be called, but going
through the new feature process should help with nailing that down.

> The crux of the problem, is that darcs apply --interactive has two
> inputs. One for the patch, and one for interactive. The patch could
> be given by specifying the file name, or by stdin. interactive can take
> input only from stdin(until now). This is causing a breakage if
> you try to take patch input from
> stdin(issue1648<http://bugs.darcs.net/issue1648>
> ).

Yeah. I do think this is a legitimate problem, albeit one that probably
comes up only for a few users; so we should try to find a solution that
works but isn't too disruptive to darcs as a whole.

> Another facet of the problem, is tests. There are many tests for
> --interactive in darcs repo, which take input from stdin. None of them
> are for apply.
>  (But there could be a hypothetical future test, where apply --
> interactive should be tested. The tester give the file name of the patch
> in the command, and interactive choices from stdin. You can't do this
> if my patch is applied, unless you somehow 'forbid' tty input in the test)

Yeah - I hadn't realised that we don't have any tests of apply
--interactive, but I guess really we should. Eric Kow pointed out in the
comments on patch561 that we could probably use "expect" or something
similar to test --ttyinteractive if we add it.

Cheers,

Ganesh

Giving up on this