Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smoother behaviour when composing with other-language programs #178

Closed
infinity0 opened this issue Sep 16, 2016 · 8 comments
Closed

Smoother behaviour when composing with other-language programs #178

infinity0 opened this issue Sep 16, 2016 · 8 comments

Comments

@infinity0
Copy link

infinity0 commented Sep 16, 2016

Small example, run this inside a git repo (with unstaged/uncommited changes):

$ cat ./git-add-p.hs 
#!/usr/bin/runhaskell
{-# LANGUAGE OverloadedStrings #-}
import Turtle
import Data.Text
import System.Environment
main = do
  argv <- fmap (fmap pack) getArgs
  stdin & proc "git" (["add", "-p"] ++ argv) & view
  1. It would be nicer if I didn't have to say stdin explicitly, it should be implicit if not given.
    • Perhaps, add a proc' or something like that.
  2. Even if I give stdin explicitly, it doesn't work well if i run e.g. ./git-add-p.hs nonexistentfile - git will output "No changes" and exit, but Turtle only exits if I ctrl-D explicitly. Or, I press [enter] instead, then it exits with an ugly git-add-p.hs: fd:12: hClose: resource vanished (Broken pipe).
    • Combined with (1), this would remove the need for empty everywhere when using proc.
  3. Remove the need for an explicit pack.
    • Perhaps, add a getArgs to Turtle that applies fmap (fmap pack).
  4. (Harder) git add -p flushes the buffer on non-line breaks (when prompting for user input), so when you run ./git-add-p.hs the behaviour is not the best. You can see similar issues when replacing add with log and it triggers the pager. But I can understand if supporting this is low priority; I wanted mainly to demonstrate the other points.

I'm still a haskell noob so apologies if any of this is redundant / doable with other utils.

@Gabriella439
Copy link
Owner

The root of the problem is that you're not supposed to use stdin here because git add does not read any input from standard input. That's why you get the broken pipe exception when you try to feed it input by typing an empty line. Replace stdin with empty and I believe it will do what you want.

Also, your use of view is not doing what you want, either. It's harmless, but it has no effect on the program. This is because:

view (liftIO io) = io

... and proc is defined in terms of liftIO and therefore emits nothing for view to print.

Your final line should basically be this:

proc "git" (["add", "-p"] ++ argv) empty

@infinity0
Copy link
Author

The point of the example is that sometimes the inner program reads from stdin, and sometimes it doesn't. In the specific case of git add -p this is predictable in advance, but in general this isn't the case - for example if I'm writing a wrapper program where I want the behaviour "use stdin if my caller is passing in stdin, or not if they're not". If you're aiming to make it easy for people to write shell-script like things, then this sort of flexible "use stdin or not, automatically" semantics is quite important.

Thanks for the point about view, I think it was left over from an earlier iteration of the example where it was needed.

@infinity0
Copy link
Author

infinity0 commented Sep 16, 2016

Actually, forgot even about the wrapper program I mentioned, the original example already demonstrates the issue: git add -p does read from stdin in most cases - except in the case where there are no changes in your index work tree, then it doesn't read from stdin. The point is, I shouldn't have to write extra logic to detect this and pass in either empty or stdin - I don't have to do that in a normal shell script!

@Gabriella439
Copy link
Owner

Before I continue, let me address your original points 3 and 4 which I forgot to respond to.

For point 3, turtle provides Turtle.Prelude.arguments, which is the Text analog of System.Environment.getArgs. Also, for something even fancier, check out Turtle.Options or optparse-generic.

For point 4, I'm working on ByteString variants of turtle utilities that will take chunked ByteString input as it becomes available and not pausing for line-boundaries. The module-in-progress for this is Turtle.Bytes, but it's still currently missing theByteString analogs of the subprocess runners. Under the hood these would use Data.ByteString.hGetSome (Link) which will not wait for a full buffer to return input.

So part of this discussion is whether or not there should be a proc', and the second part is what exactly proc' should do.

I'd prefer not to add a proc' mainly because:

  • I'd have to duplicate every other sub-process runner to add this new variation on behavior
  • I believe we can get your desired behavior for the existing proc function

I agree with you that if you provide stdin as input and the process does not demand any input that the program should not prompt the user for an input line and should also fail gracefully if the downstream process closes the pipe. In other words, you would just always provide stdin and it would do the right thing for both commands that need standard input and commands that don't.

If that's okay with you, that's what I will fix.

@infinity0
Copy link
Author

Sounds good, and I can see that proc' was not a good solution.

Another idea to avoid passing in stdin everywhere is: have Shell incorporate a Reader-like environment that by default references stdin. Then have an operator like sh's < that does the equivalent of local to set the new stdin. That would involve much more work though, likely including API changes. But if you think it's a reasonable approach, I could try it myself some time.

@Gabriella439
Copy link
Owner

Alright, this should be fixed now. stdin should now be a reasonable default for all subprocess runners: if they don't use stdin they won't block on it to complete but if they do everything will proceed as normal.

@infinity0
Copy link
Author

Great, thanks!

@Gabriella439
Copy link
Owner

You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants