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.

Sign up for GitHub

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

Jump to bottom

add --name new flag when ipfs adding from stdin #5399

Merged
merged 9 commits into from
Sep 13, 2018
Merged

add --name new flag when ipfs adding from stdin #5399

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
514891d
add --name new flag when ipfs adding from stdin
kjzz Aug 23, 2018
19e3c12
drop wrap check and modify parameter name
kjzz Sep 3, 2018
b61dae3
use file fullpath to judge file name
kjzz Sep 5, 2018
0f4e84d
use fileinfo.abs to judge the file stdin
kjzz Sep 5, 2018
ad27614
add sharness tests for --name option
kjzz Sep 6, 2018
76e8190
drop --name test file and update test case
kjzz Sep 6, 2018
9898ba9
update test and do not overwrite in sharness test
kjzz Sep 6, 2018
7de83f2
add type assertion checked
kjzz Sep 11, 2018
b7ea4bf
modify command name and update the test
kjzz Sep 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions core/commands/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const (
progressOptionName = "progress"
trickleOptionName = "trickle"
wrapOptionName = "wrap-with-directory"
pathName = "name"
hiddenOptionName = "hidden"
onlyHashOptionName = "only-hash"
chunkerOptionName = "chunker"
Expand Down Expand Up @@ -116,6 +117,7 @@ You can now check what blocks have been created by:
cmdkit.BoolOption(trickleOptionName, "t", "Use trickle-dag format for dag generation."),
cmdkit.BoolOption(onlyHashOptionName, "n", "Only chunk and hash - do not write to disk."),
cmdkit.BoolOption(wrapOptionName, "w", "Wrap files with a directory object."),
cmdkit.StringOption(pathName, "Assign path name when use wrap-with-directory option"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change this to reflect that the option (with the new check) works only for stdin, e.g., "Assign a name if the file source is stdin" (or something like that, feel free to reword it).

cmdkit.BoolOption(hiddenOptionName, "H", "Include files that are hidden. Only takes effect on recursive add."),
cmdkit.StringOption(chunkerOptionName, "s", "Chunking algorithm, size-[bytes] or rabin-[min]-[avg]-[max]").WithDefault("size-262144"),
cmdkit.BoolOption(pinOptionName, "Pin this object when adding.").WithDefault(true),
Expand Down Expand Up @@ -181,6 +183,7 @@ You can now check what blocks have been created by:
hashFunStr, _ := req.Options[hashOptionName].(string)
inline, _ := req.Options[inlineOptionName].(bool)
inlineLimit, _ := req.Options[inlineLimitOptionName].(int)
pathName, _ := req.Options[pathName].(string)

// The arguments are subject to the following constraints.
//
Expand Down Expand Up @@ -287,6 +290,7 @@ You can now check what blocks have been created by:
fileAdder.Silent = silent
fileAdder.RawLeaves = rawblks
fileAdder.NoCopy = nocopy
fileAdder.Name = pathName
fileAdder.CidBuilder = prefix

if inline {
Expand Down
8 changes: 7 additions & 1 deletion core/coreunix/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ type Adder struct {
RawLeaves bool
Silent bool
Wrap bool
Name string
NoCopy bool
Chunker string
root ipld.Node
Expand Down Expand Up @@ -470,8 +471,13 @@ func (adder *Adder) addFile(file files.File) error {
return err
}

addFileName := file.FileName()
if addFileName == "" && adder.Name != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keks WDYT about checking for an empty filename? I think this covers the stdin case in CLI, I'm not sure what effect this may have in the HTTP case, that's why we're resetting the adder.Name option to rename only one file (in the case HTTP may send many unnamed files).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are several occasions where we use an empty string as file name. I just looked into this and it looks like when using stdin, te FullName() should be os.Stdin.Name(), i.e. /dev/stdin. IMHO that would be a better check, and should also possible on the http server side.

Copy link
Contributor Author

@kjzz kjzz Sep 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello @keks ,i want to fix this bug.And i do not understand theFullname().can u explain more for me? my pr is aim to solve issue #5399 . I have explain to @schomatis why I do for this.You can see at this . Thx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjzz Sure! The file variable is a "files".File, an interface described here: https://godoc.org/github.com/ipfs/go-ipfs-cmdkit/files#File. It has the method FileName(), which is currently used in the check. But it also has the method FullName(), which should return /dev/stdin if the file is stdin. If you check file.FullName() == "/dev/stdin" or file.FullName() == os.Stdin.Name(), that should be more reliable, because there are other reasons why file.FileName() may be the empty string. I only grepped through the code in go-ipfs-cmds and saw a lot of files with empty string as name. I'm not sure this will be a problem in practice, but I can't rule out that it won't. I think checking file.FullName() should be much more reliable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @keks! That sounds much more robust, @kjzz, so the check would be

if file.FullName() == os.Stdin.Name() && adder.Name != "" {

addFileName = adder.Name
adder.Name = ""
}
// patch it into the root
return adder.addNode(dagnode, file.FileName())
return adder.addNode(dagnode, addFileName)
}

func (adder *Adder) addDir(dir files.File) error {
Expand Down