commands: remove EnableStdin support for StringArg

With verbose flag:
* remove EnableStdin() flags on all StringArg,

* remove all unneeded parsing code for StringArg, and print an
* informative message if `ipfs` begins reading from a CharDevice,

* remove broken go tests for EnableStdin cli parsing, and add some
* trivial test cases for reading FileArg from stdin,

* add a panic to prevent EnableStdin from being set on
* StringArg in the future.

Resolves: #2877, #2870
License: MIT
Signed-off-by: Thomas Gardner <tmg@fastmail.com>
This commit is contained in:
Thomas Gardner 2016-06-23 22:35:41 +10:00
parent 2a4bbb712f
commit ddc8d0c60c
23 changed files with 82 additions and 143 deletions

View File

@ -42,13 +42,16 @@ func FileArg(name string, required, variadic bool, description string) Argument
// (`FileArg("file", ArgRequired, ArgStdin, ArgRecursive)`)
func (a Argument) EnableStdin() Argument {
if a.Type == ArgString {
panic("Only FileArgs can be read from Stdin")
}
a.SupportsStdin = true
return a
}
func (a Argument) EnableRecursive() Argument {
if a.Type != ArgFile {
panic("Only ArgFile arguments can enable recursive")
panic("Only FileArgs can enable recursive")
}
a.Recursive = true

View File

@ -1,7 +1,6 @@
package cli
import (
"bytes"
"fmt"
"os"
"path"
@ -13,8 +12,11 @@ import (
cmds "github.com/ipfs/go-ipfs/commands"
files "github.com/ipfs/go-ipfs/commands/files"
u "gx/ipfs/QmZNVWh8LLjAavuQ2JXuFmuYH3C11xo988vSgp7UQrTRj1/go-ipfs-util"
logging "gx/ipfs/QmYtB7Qge8cJpXc4irsEp8zRqfnZMBeB7aTrMEkPk67DRv/go-log"
)
var log = logging.Logger("commands/cli")
// Parse parses the input commandline string (cmd, flags, and args).
// returns the corresponding command Request object.
func Parse(input []string, stdin *os.File, root *cmds.Command) (cmds.Request, *cmds.Command, []string, error) {
@ -238,6 +240,8 @@ func parseOpts(args []string, root *cmds.Command) (
return
}
const msgStdinInfo = "ipfs: Reading from %s; send Ctrl-d to stop.\n"
func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursive, hidden bool, root *cmds.Command) ([]string, []files.File, error) {
// ignore stdin on Windows
if runtime.GOOS == "windows" {
@ -286,36 +290,11 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
fillingVariadic := argDefIndex+1 > len(argDefs)
var err error
if argDef.Type == cmds.ArgString {
if len(inputs) > 0 {
// If argument is "-" use stdin
if inputs[0] == "-" && argDef.SupportsStdin {
stringArgs, stdin, err = appendStdinAsString(stringArgs, stdin)
if err != nil {
return nil, nil, err
}
}
// add string values
stringArgs, inputs = appendString(stringArgs, inputs)
} else if !argDef.SupportsStdin {
if len(inputs) == 0 {
// failure case, we have stdin, but our current
// argument doesnt want stdin
break
}
stringArgs, inputs = appendString(stringArgs, inputs)
stringArgs, inputs = append(stringArgs, inputs[0]), inputs[1:]
} else {
if stdin != nil && argDef.Required && !fillingVariadic {
// if we have a stdin, read it in and use the data as a string value
stringArgs, stdin, err = appendStdinAsString(stringArgs, stdin)
if err != nil {
return nil, nil, err
}
} else {
break
}
break
}
} else if argDef.Type == cmds.ArgFile {
if len(inputs) > 0 {
@ -325,7 +304,9 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
var file files.File
var err error
if fpath == "-" {
file = files.NewReaderFile("", "", stdin, nil)
if err = printReadInfo(stdin, msgStdinInfo); err == nil {
file = files.NewReaderFile("", "", stdin, nil)
}
} else {
file, err = appendFile(fpath, argDef, recursive, hidden)
}
@ -337,6 +318,9 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
} else {
if stdin != nil && argDef.SupportsStdin &&
argDef.Required && !fillingVariadic {
if err := printReadInfo(stdin, msgStdinInfo); err != nil {
return nil, nil, err
}
fileArgs[""] = files.NewReaderFile("", "", stdin, nil)
} else {
break
@ -389,22 +373,6 @@ func getArgDef(i int, argDefs []cmds.Argument) *cmds.Argument {
return nil
}
func appendString(args, inputs []string) ([]string, []string) {
return append(args, inputs[0]), inputs[1:]
}
func appendStdinAsString(args []string, stdin *os.File) ([]string, *os.File, error) {
buf := new(bytes.Buffer)
_, err := buf.ReadFrom(stdin)
if err != nil {
return nil, nil, err
}
input := strings.TrimSpace(buf.String())
return append(args, strings.Split(input, "\n")...), nil, nil
}
const notRecursiveFmtStr = "'%s' is a directory, use the '-%s' flag to specify directories"
const dirNotSupportedFmtStr = "Invalid path '%s', argument '%s' does not support directories"
@ -435,3 +403,18 @@ func appendFile(fpath string, argDef *cmds.Argument, recursive, hidden bool) (fi
return files.NewSerialFile(path.Base(fpath), fpath, hidden, stat)
}
// Inform the user if a file is waiting on input
func printReadInfo(f *os.File, msg string) error {
fInfo, err := f.Stat()
if err != nil {
log.Error(err)
return err
}
if (fInfo.Mode() & os.ModeCharDevice) != 0 {
fmt.Fprintf(os.Stderr, msg, f.Name())
}
return nil
}

View File

@ -177,26 +177,14 @@ func TestArgumentParsing(t *testing.T) {
commands.StringArg("b", true, false, "another arg"),
},
},
"stdinenabled": {
"FileArg": {
Arguments: []commands.Argument{
commands.StringArg("a", true, true, "some arg").EnableStdin(),
commands.FileArg("a", false, false, "some arg"),
},
},
"stdinenabled2args": &commands.Command{
"FileArg+Stdin": {
Arguments: []commands.Argument{
commands.StringArg("a", true, false, "some arg"),
commands.StringArg("b", true, true, "another arg").EnableStdin(),
},
},
"stdinenablednotvariadic": &commands.Command{
Arguments: []commands.Argument{
commands.StringArg("a", true, false, "some arg").EnableStdin(),
},
},
"stdinenablednotvariadic2args": &commands.Command{
Arguments: []commands.Argument{
commands.StringArg("a", true, false, "some arg"),
commands.StringArg("b", true, false, "another arg").EnableStdin(),
commands.FileArg("a", true, true, "some arg").EnableStdin(),
},
},
},
@ -259,53 +247,17 @@ func TestArgumentParsing(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer os.Remove(fstdin.Name())
if _, err := io.WriteString(fstdin, content); err != nil {
t.Fatal(err)
}
return fstdin
}
test([]string{"stdinenabled", "value1", "value2"}, nil, []string{"value1", "value2"})
fstdin := fileToSimulateStdin(t, "stdin1")
test([]string{"stdinenabled"}, fstdin, []string{"stdin1"})
test([]string{"stdinenabled", "value1"}, fstdin, []string{"value1"})
test([]string{"stdinenabled", "value1", "value2"}, fstdin, []string{"value1", "value2"})
defer os.Remove(fstdin.Name())
fstdin = fileToSimulateStdin(t, "stdin1\nstdin2")
test([]string{"stdinenabled"}, fstdin, []string{"stdin1", "stdin2"})
fstdin = fileToSimulateStdin(t, "stdin1\nstdin2\nstdin3")
test([]string{"stdinenabled"}, fstdin, []string{"stdin1", "stdin2", "stdin3"})
test([]string{"stdinenabled2args", "value1", "value2"}, nil, []string{"value1", "value2"})
fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"stdinenabled2args", "value1"}, fstdin, []string{"value1", "stdin1"})
test([]string{"stdinenabled2args", "value1", "value2"}, fstdin, []string{"value1", "value2"})
test([]string{"stdinenabled2args", "value1", "value2", "value3"}, fstdin, []string{"value1", "value2", "value3"})
fstdin = fileToSimulateStdin(t, "stdin1\nstdin2")
test([]string{"stdinenabled2args", "value1"}, fstdin, []string{"value1", "stdin1", "stdin2"})
test([]string{"stdinenablednotvariadic", "value1"}, nil, []string{"value1"})
fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"stdinenablednotvariadic"}, fstdin, []string{"stdin1"})
test([]string{"stdinenablednotvariadic", "value1"}, fstdin, []string{"value1"})
test([]string{"stdinenablednotvariadic2args", "value1", "value2"}, nil, []string{"value1", "value2"})
fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"stdinenablednotvariadic2args", "value1"}, fstdin, []string{"value1", "stdin1"})
test([]string{"stdinenablednotvariadic2args", "value1", "value2"}, fstdin, []string{"value1", "value2"})
testFail([]string{"stdinenablednotvariadic2args"}, fstdin, "cant use stdin for non stdin arg")
fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"noarg"}, fstdin, []string{})
fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"optionalsecond", "value1", "value2"}, fstdin, []string{"value1", "value2"})
test([]string{"FileArg", fstdin.Name()}, nil, []string{})
test([]string{"FileArg+Stdin"}, fstdin, []string{})
test([]string{"FileArg+Stdin", "-"}, fstdin, []string{})
}

View File

@ -31,7 +31,7 @@ var unwantCmd = &cmds.Command{
Tagline: "Remove a given block from your wantlist.",
},
Arguments: []cmds.Argument{
cmds.StringArg("key", true, true, "Key(s) to remove from your wantlist.").EnableStdin(),
cmds.StringArg("key", true, true, "Key(s) to remove from your wantlist."),
},
Run: func(req cmds.Request, res cmds.Response) {
nd, err := req.InvocContext().GetNode()

View File

@ -55,7 +55,7 @@ on raw ipfs blocks. It outputs the following to stdout:
},
Arguments: []cmds.Argument{
cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get.").EnableStdin(),
cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get."),
},
Run: func(req cmds.Request, res cmds.Response) {
b, err := getBlockForKey(req, req.Arguments()[0])
@ -88,7 +88,7 @@ It outputs to stdout, and <key> is a base58 encoded multihash.
},
Arguments: []cmds.Argument{
cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get.").EnableStdin(),
cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get."),
},
Run: func(req cmds.Request, res cmds.Response) {
b, err := getBlockForKey(req, req.Arguments()[0])

View File

@ -47,7 +47,7 @@ in the bootstrap list).
},
Arguments: []cmds.Argument{
cmds.StringArg("peer", false, true, peerOptionDesc).EnableStdin(),
cmds.StringArg("peer", false, true, peerOptionDesc),
},
Options: []cmds.Option{
@ -129,7 +129,7 @@ var bootstrapRemoveCmd = &cmds.Command{
},
Arguments: []cmds.Argument{
cmds.StringArg("peer", false, true, peerOptionDesc).EnableStdin(),
cmds.StringArg("peer", false, true, peerOptionDesc),
},
Options: []cmds.Option{
cmds.BoolOption("all", "Remove all bootstrap peers.").Default(false),

View File

@ -20,7 +20,7 @@ var CatCmd = &cmds.Command{
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to be outputted.").EnableStdin(),
cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to be outputted."),
},
Run: func(req cmds.Request, res cmds.Response) {
node, err := req.InvocContext().GetNode()

View File

@ -459,7 +459,7 @@ NOTE: A value may not exceed 2048 bytes.
Arguments: []cmds.Argument{
cmds.StringArg("key", true, false, "The key to store the value at."),
cmds.StringArg("value", true, false, "The value to store.").EnableStdin(),
cmds.StringArg("value", true, false, "The value to store."),
},
Options: []cmds.Option{
cmds.BoolOption("verbose", "v", "Print extra information.").Default(false),

View File

@ -44,7 +44,7 @@ The resolver can recursively resolve:
},
Arguments: []cmds.Argument{
cmds.StringArg("domain-name", true, false, "The domain-name name to resolve.").EnableStdin(),
cmds.StringArg("domain-name", true, false, "The domain-name name to resolve."),
},
Options: []cmds.Option{
cmds.BoolOption("recursive", "r", "Resolve until the result is not a DNS link.").Default(false),

View File

@ -37,7 +37,7 @@ may also specify the level of compression by specifying '-l=<1-9>'.
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, false, "The path to the IPFS object(s) to be outputted.").EnableStdin(),
cmds.StringArg("ipfs-path", true, false, "The path to the IPFS object(s) to be outputted."),
},
Options: []cmds.Option{
cmds.StringOption("output", "o", "The path where the output should be stored."),

View File

@ -58,7 +58,7 @@ EXAMPLE:
`,
},
Arguments: []cmds.Argument{
cmds.StringArg("peerid", false, false, "Peer.ID of node to look up.").EnableStdin(),
cmds.StringArg("peerid", false, false, "Peer.ID of node to look up."),
},
Options: []cmds.Option{
cmds.StringOption("format", "f", "Optional output format."),

View File

@ -46,7 +46,7 @@ Resolve the value of a reference:
},
Arguments: []cmds.Argument{
cmds.StringArg("name", false, false, "The IPNS name to resolve. Defaults to your node's peerID.").EnableStdin(),
cmds.StringArg("name", false, false, "The IPNS name to resolve. Defaults to your node's peerID."),
},
Options: []cmds.Option{
cmds.BoolOption("recursive", "r", "Resolve until the result is not an IPNS name.").Default(false),

View File

@ -41,7 +41,7 @@ format:
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to list links from.").EnableStdin(),
cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to list links from."),
},
Options: []cmds.Option{
cmds.BoolOption("headers", "v", "Print table headers (Hash, Size, Name).").Default(false),

View File

@ -78,7 +78,7 @@ is the raw data of the object.
},
Arguments: []cmds.Argument{
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format.").EnableStdin(),
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format."),
},
Run: func(req cmds.Request, res cmds.Response) {
n, err := req.InvocContext().GetNode()
@ -108,7 +108,7 @@ multihash.
},
Arguments: []cmds.Argument{
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format.").EnableStdin(),
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format."),
},
Options: []cmds.Option{
cmds.BoolOption("headers", "v", "Print table headers (Hash, Size, Name).").Default(false),
@ -179,7 +179,7 @@ This command outputs data in the following encodings:
},
Arguments: []cmds.Argument{
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format.").EnableStdin(),
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format."),
},
Run: func(req cmds.Request, res cmds.Response) {
n, err := req.InvocContext().GetNode()
@ -246,7 +246,7 @@ var ObjectStatCmd = &cmds.Command{
},
Arguments: []cmds.Argument{
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format.").EnableStdin(),
cmds.StringArg("key", true, false, "Key of the object to retrieve, in base58-encoded multihash format."),
},
Run: func(req cmds.Request, res cmds.Response) {
n, err := req.InvocContext().GetNode()

View File

@ -39,7 +39,7 @@ var addPinCmd = &cmds.Command{
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, true, "Path to object(s) to be pinned.").EnableStdin(),
cmds.StringArg("ipfs-path", true, true, "Path to object(s) to be pinned."),
},
Options: []cmds.Option{
cmds.BoolOption("recursive", "r", "Recursively pin the object linked to by the specified object(s).").Default(true),
@ -103,7 +103,7 @@ collected if needed. (By default, recursively. Use -r=false for direct pins)
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, true, "Path to object(s) to be unpinned.").EnableStdin(),
cmds.StringArg("ipfs-path", true, true, "Path to object(s) to be unpinned."),
},
Options: []cmds.Option{
cmds.BoolOption("recursive", "r", "Recursively unpin the object linked to by the specified object(s).").Default(true),

View File

@ -37,7 +37,7 @@ trip latency information.
`,
},
Arguments: []cmds.Argument{
cmds.StringArg("peer ID", true, true, "ID of peer to be pinged.").EnableStdin(),
cmds.StringArg("peer ID", true, true, "ID of peer to be pinged."),
},
Options: []cmds.Option{
cmds.IntOption("count", "n", "Number of ping messages to send.").Default(10),

View File

@ -47,7 +47,7 @@ Publish an <ipfs-path> to another public key (not implemented):
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, false, "IPFS path of the object to be published.").EnableStdin(),
cmds.StringArg("ipfs-path", true, false, "IPFS path of the object to be published."),
},
Options: []cmds.Option{
cmds.BoolOption("resolve", "Resolve given path before publishing.").Default(true),

View File

@ -46,7 +46,7 @@ NOTE: List all references recursively by using the flag '-r'.
"local": RefsLocalCmd,
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, true, "Path to the object(s) to list refs from.").EnableStdin(),
cmds.StringArg("ipfs-path", true, true, "Path to the object(s) to list refs from."),
},
Options: []cmds.Option{
cmds.StringOption("format", "Emit edges with given format. Available tokens: <src> <dst> <linkname>.").Default("<dst>"),

View File

@ -56,7 +56,7 @@ Resolve the value of an IPFS DAG path:
},
Arguments: []cmds.Argument{
cmds.StringArg("name", true, false, "The name to resolve.").EnableStdin(),
cmds.StringArg("name", true, false, "The name to resolve."),
},
Options: []cmds.Option{
cmds.BoolOption("recursive", "r", "Resolve until the result is an IPFS name.").Default(false),

View File

@ -215,7 +215,7 @@ ipfs swarm connect /ip4/104.131.131.82/tcp/4001/ipfs/QmaCpDMGvV2BGHeYERUEnRQAwe3
`,
},
Arguments: []cmds.Argument{
cmds.StringArg("address", true, true, "Address of peer to connect to.").EnableStdin(),
cmds.StringArg("address", true, true, "Address of peer to connect to."),
},
Run: func(req cmds.Request, res cmds.Response) {
ctx := req.Context()
@ -273,7 +273,7 @@ it will reconnect.
`,
},
Arguments: []cmds.Argument{
cmds.StringArg("address", true, true, "Address of peer to disconnect from.").EnableStdin(),
cmds.StringArg("address", true, true, "Address of peer to disconnect from."),
},
Run: func(req cmds.Request, res cmds.Response) {
n, err := req.InvocContext().GetNode()
@ -441,7 +441,7 @@ add your filters to the ipfs config file.
`,
},
Arguments: []cmds.Argument{
cmds.StringArg("address", true, true, "Multiaddr to filter.").EnableStdin(),
cmds.StringArg("address", true, true, "Multiaddr to filter."),
},
Run: func(req cmds.Request, res cmds.Response) {
n, err := req.InvocContext().GetNode()
@ -513,7 +513,7 @@ remove your filters from the ipfs config file.
`,
},
Arguments: []cmds.Argument{
cmds.StringArg("address", true, true, "Multiaddr filter to remove.").EnableStdin(),
cmds.StringArg("address", true, true, "Multiaddr filter to remove."),
},
Run: func(req cmds.Request, res cmds.Response) {
n, err := req.InvocContext().GetNode()

View File

@ -83,7 +83,7 @@ var tarCatCmd = &cmds.Command{
},
Arguments: []cmds.Argument{
cmds.StringArg("path", true, false, "IPFS path of archive to export.").EnableStdin(),
cmds.StringArg("path", true, false, "IPFS path of archive to export."),
},
Run: func(req cmds.Request, res cmds.Response) {
nd, err := req.InvocContext().GetNode()

View File

@ -64,7 +64,7 @@ Example:
},
Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to list links from.").EnableStdin(),
cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to list links from."),
},
Run: func(req cmds.Request, res cmds.Response) {
node, err := req.InvocContext().GetNode()

View File

@ -223,8 +223,8 @@ test_expect_success "ipfs cat output looks good" '
test_cmp expected actual
'
test_expect_success "ipfs cat accept hash from stdin" '
echo "$HASH" | ipfs cat >actual
test_expect_success "ipfs cat accept hash from built input" '
echo "$HASH" | xargs ipfs cat >actual
'
test_expect_success "ipfs cat output looks good" '
@ -279,11 +279,11 @@ test_expect_success "'ipfs add' output looks good" '
test_cmp expected actual
'
test_expect_success "'ipfs cat' with stdin input succeeds" '
echo "$HASH" | ipfs cat >actual
test_expect_success "'ipfs cat' with built input succeeds" '
echo "$HASH" | xargs ipfs cat >actual
'
test_expect_success "ipfs cat with stdin input output looks good" '
test_expect_success "ipfs cat with built input output looks good" '
printf "Hello Neptune!\nHello Pluton!" >expected &&
test_cmp expected actual
'
@ -330,8 +330,8 @@ test_expect_success "'ipfs add -rn' output looks good" '
test_cmp expected actual
'
test_expect_success "ipfs cat accept many hashes from stdin" '
{ echo "$MARS"; echo "$VENUS"; } | ipfs cat >actual
test_expect_success "ipfs cat accept many hashes from built input" '
{ echo "$MARS"; echo "$VENUS"; } | xargs ipfs cat >actual
'
test_expect_success "ipfs cat output looks good" '
@ -347,21 +347,22 @@ test_expect_success "ipfs cat output looks good" '
test_cmp expected actual
'
test_expect_success "ipfs cat with both arg and stdin" '
echo "$MARS" | ipfs cat "$VENUS" >actual
test_expect_success "ipfs cat with both arg and built input" '
echo "$MARS" | xargs ipfs cat "$VENUS" >actual
'
test_expect_success "ipfs cat output looks good" '
cat mountdir/planets/venus.txt >expected &&
cat mountdir/planets/venus.txt mountdir/planets/mars.txt >expected &&
test_cmp expected actual
'
test_expect_success "ipfs cat with two args and stdin" '
echo "$MARS" | ipfs cat "$VENUS" "$VENUS" >actual
test_expect_success "ipfs cat with two args and built input" '
echo "$MARS" | xargs ipfs cat "$VENUS" "$VENUS" >actual
'
test_expect_success "ipfs cat output looks good" '
cat mountdir/planets/venus.txt mountdir/planets/venus.txt >expected &&
cat mountdir/planets/venus.txt mountdir/planets/venus.txt \
mountdir/planets/mars.txt >expected &&
test_cmp expected actual
'