cli: Fix stdin parsing

[atomgardner]

@Kubuxu requested to nuke EnableStdin on StringArg, but I reckon it's (slightly) better to just fix the parsing & avoid having to wonder, is this a valid pipeline or do I need to use xargs?

EDIT(Kubuxu, for tracking): Resolves #2877, Resolves #2870

[chriscool]

The comment above does not match anymore what is done below.

[Kubuxu]

I don't like this solution much as we are again introducing trap of depending on type of input (named pipe in this case) to work. Are you sure that it will work the same with for example unnamed pipes or under docker?

About whether you should use xargs or just pipe in: after nuking StringArgs stdin handling it would be easy: If given command wants a file, you can pipe, if it works on argument strings, you want to xargs it.

Other thing is I don't know any command that work like ipfs now.

[whyrusleeping]

@Kubuxu @thomas-gardner how about instead of having the arguments be strings here, we make them some kind of interface. that way, we can push through the parsing here without having to read from stdin, and then when we actually get to the command we can read from stdin as we need it.

[Kubuxu]

As I talked with @whyrusleeping it would be very hard to parse parameters on demand as for the fact that parameters are transported over HTTP.

@lgierth could you weigh in?

[whyrusleeping]

the biggest issue for me right now is that ipfs block get --help hangs.

[whyrusleeping]

I don't think we can successfully get away with not checking what type of thing stdin is.

[kevina]

I don't like this solution much as we are again introducing trap of depending on type of input (named pipe in this case) to work. Are you sure that it will work the same with for example unnamed pipes or under docker?

I agree with @Kubuxu on this. If we allow stdin input, it should have to be specified on the command line. I do not like the idea at all of a command behaving differently if I pipe something into it, vs if I just run it from the terminal. Sometimes I just run command that take input from stdin and just manually enter the data, useful in debugging and exploring how a command works.

[Kubuxu]

My solution for that is to make ipfs behave as every other command and nuke stdin handling for string based arguments in favour of xargs. This is why xargs was created so every command doesn't have to do it on its own and so they all work in similar manner.

[whyrusleeping]

@Kubuxu alright, lets go ahead with that. Explicit selecting stdin in all cases.

[atomgardner]

I now agree that this is really aberrant behaviour for a UNIX command
(GNU ls doesn't accept pipes, for example.). A git grep "StringA.*E"
shows that, with two exceptions, all the StringArgs with EnableStdin are
expecting a hash of some sort. So it is actually very easy to know when
to use xargs: whenever you're piping a hash.

@Kubuxu , a couple questions:

1. What should ipfs do if a StringArg is being piped in? Ignore it and
   error?

2. Using add wlog, what should happen on all the following cases (my
   expectations are in comments.)

   $ ipfs add

   error

   $ echo "foo" |ipfs add

   same as go-ipfs/master

   $ ipfs add -

   same as go-ipfs/master

   $ echo "foo" |ipfs add -

   echo "foo" |ipfs && ipfs add -

   $ ipfs add $filename

   same as go-ipfs/master

   $ ipfs add $filename -

   ipfs add $filename && ipfs add -

   $ echo "foo" |ipfs add $filename

   echo "foo" |ipfs add && ipfs add $filename

   $ echo "foo" |ipfs add $filename -

   echo "foo" |ipfs add && ipfs add $filename && ipfs add -

and what should happen if a $filename doesn't exist?

===STALE===
Hmm, I should probably have clarified what this patch does.

0: FileArgs are unchanged, so slick lines like

$ echo "Hello, Pluto?" |ipfs add 

will work as before.

1: Commands will not (should not?) run differently when strings are
piped in; it's just a way to construct the cmd line.

$ hello="QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG"
$ ./ipfs-31e65d7 ls "$hello"
QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V 1688 about
...
$ echo "$hello" |./ipfs-31e65d7 ls >cool_hashes
QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V 1688 about
...

2: Manual input can be requested with a hyphen,

$ ./ipfs-31e65d7 ls -
# reads string arg until EOF

but a pipe will take precedence:

$ cat cool_hashes |cut -f1 -d' '|./ipfs-31e65d7 cat  - |head -2

               IPFS -- Inter-Planetary File system

===STALE===

[Kubuxu]

Answers:
0. When data is being piped in into command that is not prepared for it, the command doesn't care

1. ipfs add command should work similarly to the cat command from Unix.

    $ ipfs add
    # error
    > read from stdin (display info that stdin is read over stderr)

    $ echo "foo" |ipfs add
    # same as go-ipfs/master
    > add text foo to ipfs

    $ ipfs add -
    # same as go-ipfs/master
    > read from stdin (maybe display info that stdin is read over stderr)

    $ echo "foo" |ipfs add -
    # echo "foo" |ipfs && ipfs add -
    > same as `echo "foo" | ipfs add`

    $ ipfs add $filename
    # same as go-ipfs/master
    > add $filename to ipfs

    $ ipfs add $filename -
    # ipfs add $filename && ipfs add -
    > add filename and read and add from stdin/console, can finish with Ctrl-D

    $ echo "foo" |ipfs add $filename 
    # echo "foo" |ipfs add && ipfs add $filename
    > ignore stdin, add just $filename

    $ echo "foo" |ipfs add $filename -
    # echo "foo" |ipfs add && ipfs add $filename && ipfs add -
    > add both $filename and string "foo"

Non existing filename should depend on the command or wherever it is handled now.

My proposition would change:

cat $hashesfile | ipfs ls into cat $hashes | xargs ipfs ls - same as you would do with normal ls command
cat $hashesfile | ipfs cat into cat $hashes | xargs ipfs cat - same as you would do with normal cat command (xargs cat < $filestocatfile)
cat $nodesfile | ipfs bootstrap add into cat $nodesfile | xargs ipfs bootstrap add - same way you would use the command from the command line (you place the input as arugments, no magic conversion in the ipfs command itself).

My idea is to model ipfs command after something familiar, and no command I know parses stdin and places it as its arguments.

[atomgardner]

The close is unintentional.

@Kubuxu, how does this look?

[Kubuxu]

@thomas-gardner as you removed the branch you will have to create new PR.

It looks generally good. I would also add go test to check that there are no StringArgs with EnableStdin called to root out the noise from code. The test can be written similarly to: #2648

We will also need sharness regression tests so this behaviour (hanging on --help) doesn't repeat.

[Kubuxu]

Also as the message is only cosmetic we can make is so it is only displayed if stdin is character input device.

Thank you for work you are putting into it.