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

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 7 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 a name if the file source is stdin."),
Copy link
Member

Choose a reason for hiding this comment

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

Given that this'll only work for STDIN, I'd consider calling this --stdin-name. I believe the original issue was suggesting a more general --name flag (even for regular files) but I actually prefer how this patch currently works.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seams a little two verbose to me. Also, in the future want to allow this to override an existing name and then we will be stuck with both a --name and --stdin-name option.

For now I recommend we keep it as is and instead return an error (or maybe just a warning) when the option won't have any effect.

Copy link
Member

Choose a reason for hiding this comment

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

Also, in the future want to allow this to override an existing name and then we will be stuck with both a --name and --stdin-name option.

Actually, my thought was that this should never be a general-purpose flag. I believe that was the original suggestion but, as was discovered when writing this patch, that's ambiguous with multiple inputs. We also don't want to change the semantics at some point in the future (IMO).

Copy link
Member

Choose a reason for hiding this comment

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

We could also have a "rename" feature but we'll have to carefully consider how we want to do it.

Note, by "multiple inputs" I mean:

> ipfs add -w --name "foo" file1 file2

We can only ever have one stdin so setting the name for stdin is unambiguous. We can easily add multiple files at the same time.

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
9 changes: 8 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,14 @@ func (adder *Adder) addFile(file files.File) error {
return err
}

addFileName := file.FileName()
addFileInfo := file.(files.FileInfo)
Copy link
Member

Choose a reason for hiding this comment

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

@keks are you sure this won't panic in some cases? We should probably use a checked type assertion (or comment on why this must be true).

if addFileInfo.AbsPath() == 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
21 changes: 21 additions & 0 deletions test/sharness/t0040-add-and-cat.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,27 @@ test_add_cat_file() {
test_expect_success "make sure it looks good" '
test_cmp zero-length-file zero-length-file_out
'

test_expect_success "ipfs add --name" '
NAMEHASH="QmdFyxZXsFiP4csgfM5uPu99AvFiKH62CSPDw5TP92nr7w" &&
echo "IPFS" | ipfs add --name file.txt > actual &&
echo "added $NAMEHASH file.txt" > expected &&
test_cmp expected actual
'

test_expect_success "ipfs add --name -w" '
NAMEHASH="QmdFyxZXsFiP4csgfM5uPu99AvFiKH62CSPDw5TP92nr7w" &&
echo "IPFS" | ipfs add -w --name file.txt | head -n1> actual &&
echo "added $NAMEHASH file.txt" > expected &&
test_cmp expected actual
'

test_expect_success "ipfs cat with name" '
NAMEHASH=$(echo "IPFS" | ipfs add -w --name file.txt -Q) &&
ipfs cat /ipfs/$NAMEHASH/file.txt > expected &&
echo "IPFS" > actual &&
test_cmp expected actual
'
}

test_add_cat_5MB() {
Expand Down