ipfs cat "multihash too short" error when using stdin

[chriscool]

Try for example:

$ echo stuff >file1
$ ipfs add file1 
added QmNgd5cz2jNftnAHBhcRUGdtiaMzb5Rhjqd4etondHHST8 file1
$ cat file1 | while read line; do ipfs cat QmNgd5cz2jNftnAHBhcRUGdtiaMzb5Rhjqd4etondHHST8; done
Error: multihash too short. must be > 3 bytes

As stdin is opened when ipfs cat is run, the following line appends an empty string into the stringArgs variable:

https://github.com/ipfs/go-ipfs/blob/master/commands/cli/parse.go#L201

The empty string is then passed to multihash which fails.

I think parse.go should try to read from stdin only if a special option like "-" or "--stdin" is passed to "ipfs cat".

Maybe this bug happens with other commands than "ipfs cat". I haven't checked.

[chriscool]

Maybe the following test failure is linked to the same problem:

https://github.com/ipfs/go-ipfs/blob/master/test/sharness/t0111-gateway-writable.sh#L36

[chriscool]

As in whyrusleeping/git-ipfs-rehost#5 it's possible to work around the problem using "</dev/null" after the ipfs cat command. For example like:

$ cat file1 | while read line; do ipfs cat QmNgd5cz2jNftnAHBhcRUGdtiaMzb5Rhjqd4etondHHST8 </dev/null; done

[jbenet]

ugh, this is a cmds lib being aggressive problem.

do we know what the desired fix there would look like?

cc @whyrusleeping

[chriscool]

If we agree that for example unless --stdin is passed, commands should not read from stdin, I can have a look at fixing this.

[wking]

On Mon, Apr 27, 2015 at 04:59:59AM -0700, Christian Couder wrote:

If we agree that for example unless --stdin is passed, commands
should not read from stdin, I can have a look at fixing this.

Personally, I think the user interface for this type of thing should
be “explicitly look at stdin in the command implementation iff we
don't have any of the associated arguments”. So:

$ ipfs cat QmBlaBla

wouldn't read from stdin, but:

$ ipfs cat

would. I don't think use cases involving both stdin and argv:

$ echo 'QmBlaBla' | ipfs cat QmFooBar

are useful enough to be worth supporting.

[chriscool]

@wking yeah this is another possibility.

Also if you allow both stdin and args it is not clear which of stdin and args will be processed first.
Currently stdin is processed first.

One potential problem is that we may have commands with many arguments some of them possibly optional and if we decide early on without thinking much about it what the command will accept in its stdin, we may not chose wisely.

If we have to explicitely add --stdin as an allowed flag for a command, then we will do that only when there is a real need and we can decide based on the real need what is the best input for stdin (and add tests at the same time).

Some git commands have special stdin input, see for example:

http://git-scm.com/docs/git-update-ref
http://git-scm.com/docs/git-http-fetch
http://git-scm.com/docs/git-index-pack
http://git-scm.com/docs/git-hash-object

[wking]

On Tue, Apr 28, 2015 at 09:54:55AM -0700, Christian Couder wrote:

Also if you allow both stdin and args it is not clear which of stdin
and args will be processed first.

Not a problem if it's either/or and we just ignore stdin if we have
the associated argv entries. If we do provide a way to use both
stdin and argv (e.g. a --stdin flag), then the order is still not a
problem for things like ‘ipfs add’. But it would be a problem for
‘ipfs cat’, so I'm in favor of IPFS consistently using an either/or
approach.

One potential problem is that we may have commands with many
arguments some of them possibly optional and if we decide early on
without thinking much about it what the command will accept in its
stdin, we may not chose wisely.

I agree that it's imporant to think over when and why we want to
optionally read arguments (or some other content) from stdin, but I
don't agree that requiring a --stdin flag is going to make it
particularly easier to have that discipline ;). Since ‘cat’ (the
POSIX command) has 1:

file
A pathname of an input file. If no file operands are
specified, the standard input shall be used. If a file is '-',
the cat utility shall read from the standard input at that
point in the sequence. The cat utility shall not close and
reopen standard input when it is referenced in this way, but
shall accept multiple occurrences of '-' as a file operand.

I think that's a good pattern to follow for ‘ipfs add’. But I'm also
ok following Git's explicit --stdin and --stdin-paths
(git-hash-object(1)). It's also good to be consistent within IPFS.
If there are other precedents for the other stdin-reading commands (I
haven't checked), than maybe they argue for a different convention.

[jbenet]

@wking

Personally, I think the user interface for this type of thing should
be “explicitly look at stdin in the command implementation iff we
don't have any of the associated arguments”. So:

$ ipfs cat QmBlaBla

wouldn't read from stdin, but:

$ ipfs cat

would.

Agreed with this.

I don't think use cases involving both stdin and argv:

$ echo 'QmBlaBla' | ipfs cat QmFooBar

are useful enough to be worth supporting.

Disagree. Maybe for places where stdin would be used as a file arg, but not categorically for arguments. there's many examples of tools using both stdin and arguments. Example:

ipfs pipe QmcAuALHaH9pxJP9bzo7go8QU9xUraSozBNVynRs81hpqr >ForMe.tar
cat ForYou.tar | ipfs pipe QmcAuALHaH9pxJP9bzo7go8QU9xUraSozBNVynRs81hpqr

cat untrusted | hashpipe Qmaa4Rw81a3a1VEx4LxB7HADUAXvZFhCoRdBzsMZyZmqHD >trusted

---

@chriscool

One potential problem is that we may have commands with many arguments some of them possibly optional and if we decide early on without thinking much about it what the command will accept in its stdin, we may not chose wisely.

Agreed.

If we have to explicitely add --stdin as an allowed flag for a command, then we will do that only when there is a real need and we can decide based on the real need what is the best input for stdin (and add tests at the same time).

Very much agreed.

---

We could use - for stdin, which is pretty standard:

cat file | ipfs add -

And at least we probably should support it.

But I've always preferred when tools are "smart enough" to "do the right thing". So i'd like to work hard towards being understanding of the users, but it should definitely not make things annoying to use. (opposite of intended effect). In general, I think the tools rarely are complicated enough that we can't figure out what to do. We can often bucket the use cases into pretty different cases: either reading froms stdin OR reading args as filenames to read:

cat file | ipfs add    # ok
cat file | ipfs add -  # ok
ipfs add file file2    # ok

cat file | ipfs add file2   # not ok

(I think parsing arguments as filenames to read AND reading from stdin in the same call (e.g. cat file | ipfs add anotherfile) is not good -- or in other words, i havent found a use case where it's necessary to support it and is not confusing to users. Excepting as signaled with - as the cat man page suggests:

cat file | ipfs add - file2  # probably ok

---

I disagree with requiring --stdin everywhere. I think we can do better. I do think it will be needed to disambiguate in some complicated cases, but not always (the common commands).

[wking]

On Tue, Apr 28, 2015 at 12:20:11PM -0700, Juan Batiz-Benet wrote:

I don't think use cases involving both stdin and argv:

$ echo 'QmBlaBla' | ipfs cat QmFooBar

are useful enough to be worth supporting.

Disagree. Maybe for places where stdin would be used as a file arg,
but not categorically for arguments. there's many examples of tools
using both stdin and arguments.

Right, I meant cases like ‘ipfs add’ which use:

cmds.FileArg("path", true, true, "The path to a file to be added to IPFS").EnableRecursive().EnableStdin()

I think that argument should be either argv or stdin and not both.
If you have two separate arguments you can have one on argv and on on
either argv or stdin. For example, I'm fine with:

$ echo hello | ipfs add --quiet

which gets the file content from stdin and the quiet setting from
argv. Or your hashpipe which gets the content from stdin and the
expected multihash from argv. But I wouldn't like a hashpipe that
took content from both stdin and and argv path at the same time.
Which sounds like what you're saying here:

(I think parsing arguments as filenames to read AND reading from
stdin in the same call (e.g. cat file | ipfs add anotherfile) is
not good)

[chriscool]

This should be fixed as PR #1238 as been merged.

[jbenet]

thanks @chriscool -- mind verifying and reopen if not?

[chriscool]

The new tests in t0040 make sure the original issue is fixed.

[jbenet]

ok great -- thanks