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 Defaults to ipfs add #2657

Merged
merged 2 commits into from
May 29, 2016
Merged

Add Defaults to ipfs add #2657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f796dc
Add Defaults to `ipfs add`
RichardLitt May 10, 2016
03dd669
Refactor progress and showProgressBar
RichardLitt May 21, 2016
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
55 changes: 21 additions & 34 deletions core/commands/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ const (

var AddCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "Add a file or directory to ipfs.",
Tagline: "Add a file to ipfs.",
Copy link
Contributor

Choose a reason for hiding this comment

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

@RichardLitt Out of curiosity, is there a reason for this change. The original text seamed more accurate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because with -r or -w you're adding more than a file. Seemed to me like that would be pretty common, and that it should be clear that this command also works for dirs.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is worse than it was before. ipfs add is for adding both files and directories, and the command summary line should say so. The description can be more clear, that's separate.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should just make sure that there is a note in the description. I feel that it may have been better before, now that I'm a few months away from where I was when I started this PR.

ShortDescription: `
Adds contents of <path> to ipfs. Use -r to add directories (recursively).
Adds contents of <path> to ipfs. Use -r to add directories.
Note that directories are added recursively, to form the ipfs
MerkleDAG.
Copy link
Member

Choose a reason for hiding this comment

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

  • I think this is also a regression. I dont think this language change adds, it only adds more words, while the meaning is still the same.
  • Also "to form the ipfs MerkleDAG", "the" is off. "to form an ipfs MerkleDAG" sounds better.
  • And, files are also formed as merkledags, so is everything you add to ipfs. I think this clause is more confusing than helpful. I do agree that explaining what's going on is a good idea. This is something for the Long Description.

I would vote for either:

Adds contents of <path> to ipfs. Use -r to add directories, recursively.

or

Adds contents of <path> to ipfs. Use -r to add directories, recursively.

Files and directories will be chunked and imported into ipfs. File chunks
and directory entries will be linked with cryptographic hashes, forming
an ipfs merkledag. For more information about how file importing works
and the merkledag data structures, see <link>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where do you think we should point to in the ?

`,
LongDescription: `
Adds contents of <path> to ipfs. Use -r to add directories.
Expand Down Expand Up @@ -61,29 +63,21 @@ You can now refer to the added file in a gateway, like so:
},
Options: []cmds.Option{
cmds.OptionRecursivePath, // a builtin option that allows recursive paths (-r, --recursive)
cmds.BoolOption(quietOptionName, "q", "Write minimal output."),
cmds.BoolOption(silentOptionName, "Write no output."),
cmds.BoolOption(progressOptionName, "p", "Stream progress data."),
cmds.BoolOption(trickleOptionName, "t", "Use trickle-dag format for dag generation."),
cmds.BoolOption(onlyHashOptionName, "n", "Only chunk and hash - do not write to disk."),
cmds.BoolOption(wrapOptionName, "w", "Wrap files with a directory object."),
cmds.BoolOption(hiddenOptionName, "H", "Include files that are hidden. Only takes effect on recursive add."),
cmds.BoolOption(quietOptionName, "q", "Write minimal output.").Default(false),
cmds.BoolOption(silentOptionName, "Write no output.").Default(false),
cmds.BoolOption(progressOptionName, "p", "Stream progress data.").Default(true),
cmds.BoolOption(trickleOptionName, "t", "Use trickle-dag format for dag generation.").Default(false),
cmds.BoolOption(onlyHashOptionName, "n", "Only chunk and hash - do not write to disk.").Default(false),
cmds.BoolOption(wrapOptionName, "w", "Wrap files with a directory object.").Default(false),
cmds.BoolOption(hiddenOptionName, "H", "Include files that are hidden. Only takes effect on recursive add.").Default(false),
Copy link
Member Author

Choose a reason for hiding this comment

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

These should mostly be removed, too. No need for Default(false).

cmds.StringOption(chunkerOptionName, "s", "Chunking algorithm to use."),
cmds.BoolOption(pinOptionName, "Pin this object when adding. Default: true."),
cmds.BoolOption(pinOptionName, "Pin this object when adding.").Default(true),
},
PreRun: func(req cmds.Request) error {
if quiet, _, _ := req.Option(quietOptionName).Bool(); quiet {
return nil
}

// ipfs cli progress bar defaults to true
progress, found, _ := req.Option(progressOptionName).Bool()
if !found {
progress = true
}

req.SetOption(progressOptionName, progress)

sizeFile, ok := req.Files().(files.SizeFile)
if !ok {
// we don't need to error, the progress bar just won't know how big the files are
Expand Down Expand Up @@ -129,11 +123,7 @@ You can now refer to the added file in a gateway, like so:
hidden, _, _ := req.Option(hiddenOptionName).Bool()
silent, _, _ := req.Option(silentOptionName).Bool()
chunker, _, _ := req.Option(chunkerOptionName).String()
dopin, pin_found, _ := req.Option(pinOptionName).Bool()

if !pin_found { // default
dopin = true
}
dopin, _, _ := req.Option(pinOptionName).Bool()

if hash {
nilnode, err := core.NewNode(n.Context(), &core.BuildCfg{
Expand Down Expand Up @@ -220,7 +210,7 @@ You can now refer to the added file in a gateway, like so:
return
}

progress, prgFound, err := req.Option(progressOptionName).Bool()
progress, _, err := req.Option(progressOptionName).Bool()
if err != nil {
res.SetError(u.ErrCast(), cmds.ErrNormal)
return
Expand All @@ -232,16 +222,13 @@ You can now refer to the added file in a gateway, like so:
return
}

var showProgressBar bool
if prgFound {
showProgressBar = progress
} else if !quiet && !silent {
showProgressBar = true
if !quiet && !silent {
progress = true
Copy link
Member

Choose a reason for hiding this comment

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

I think this change changes the actual semantics of the code. Because now i can't shut it off if i don't also say quiet or silent. For example, this:

ipfs add file --progress=false

no longer works, i think.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah looks like #2657 (comment) confirmed

}

var bar *pb.ProgressBar
var terminalWidth int
if showProgressBar {
if progress {
bar = pb.New64(0).SetUnits(pb.U_BYTES)
bar.ManualUpdate = true
bar.Start()
Expand Down Expand Up @@ -276,7 +263,7 @@ You can now refer to the added file in a gateway, like so:
}
output := out.(*coreunix.AddedObject)
if len(output.Hash) > 0 {
if showProgressBar {
if progress {
// clear progress bar line before we print "added x" output
fmt.Fprintf(res.Stderr(), "\033[2K\r")
}
Expand All @@ -289,7 +276,7 @@ You can now refer to the added file in a gateway, like so:
} else {
log.Debugf("add progress: %v %v\n", output.Name, output.Bytes)

if !showProgressBar {
if !progress {
continue
}

Expand All @@ -305,11 +292,11 @@ You can now refer to the added file in a gateway, like so:
totalProgress = bar.Add64(delta)
}

if showProgressBar {
if progress {
bar.Update()
}
case size := <-sizeChan:
if showProgressBar {
if progress {
bar.Total = size
bar.ShowPercent = true
bar.ShowBar = true
Expand Down
8 changes: 5 additions & 3 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 @@ -11,9 +11,11 @@ test_description="Test add and cat commands"
client_err_add() {
printf "$@\n\n"
echo 'USAGE
ipfs add <path>... - Add a file or directory to ipfs.
ipfs add <path>... - Add a file to ipfs.
Copy link
Member

Choose a reason for hiding this comment

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

Add a file or directory to ipfs


Adds contents of <path> to ipfs. Use -r to add directories (recursively).
Adds contents of <path> to ipfs. Use -r to add directories.
Note that directories are added recursively, to form the ipfs
MerkleDAG.

Use '"'"'ipfs add --help'"'"' for more information about this command.
'
Expand Down Expand Up @@ -360,7 +362,7 @@ test_add_cat_5MB

test_add_cat_expensive

test_add_named_pipe " Post http://$API_ADDR/api/v0/add?encoding=json&progress=true&r=true&stream-channels=true:"
test_add_named_pipe " Post http://$API_ADDR/api/v0/add?encoding=json&r=true&stream-channels=true:"

test_kill_ipfs_daemon

Expand Down