Add Defaults to `ipfs add` #2657

RichardLitt · 2016-05-10T21:14:24Z

I didn't bother with Chunker, because I think that is a much wider PR. These should all be solid, though. Redid some of the logic to make it smoother.

Part of #2484.

whyrusleeping · 2016-05-10T21:31:23Z

doesnt build

RichardLitt · 2016-05-10T21:55:56Z

What is the command I should use to build?

whyrusleeping · 2016-05-10T22:01:02Z

make build, make install, make test, go test ./... Depending on what youre looking to do. Generally the go test one is the best bet

Kubuxu · 2016-05-12T17:54:13Z

You have to reflect changes in sharness tests.
http://ci.ipfs.team:8111/viewLog.html?buildId=1447&buildTypeId=GoIpfs_CiTests&tab=buildLog#_state=10171&focus=10171

Kubuxu · 2016-05-17T19:15:39Z

Sharness still fails: http://ci.ipfs.team:8111/viewLog.html?buildId=1922&buildTypeId=GoIpfs_CiTests&tab=buildLog#_focus=9123

Kubuxu · 2016-05-19T19:15:47Z

I squashed it as previously commits in a middle were failing.

Kubuxu · 2016-05-20T20:13:42Z

LGTM

whyrusleeping · 2016-05-20T21:23:05Z

core/commands/add.go

-		if prgFound {
-			showProgressBar = progress
-		} else if !quiet && !silent {
+		if !progress && !quiet && !silent {


i'm not sure this one is correct. If the user actually specifies a choice for the progress option, we want to respect it, otherwise, we assume true unless they pass either quiet or silent.

I didn't bother with Chunker, because I think that is a much wider PR. These should all be solid, though. Redid some of the logic to make it smoother. Part of #2484. License: MIT Signed-off-by: Richard Littauer <richard.littauer@gmail.com>

RichardLitt · 2016-05-21T11:39:11Z

I think I fixed that issue.

License: MIT Signed-off-by: Richard Littauer <richard.littauer@gmail.com>

Kubuxu · 2016-05-25T14:39:04Z

LGTM

kevina · 2016-05-29T07:50:58Z

core/commands/add.go

@@ -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.",


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

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.

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.

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.

Following ipfs/kubo#2657

whyrusleeping · 2016-05-31T01:02:02Z

this broke gx, previously the http api wouldnt receive progress information, now it does (since the default gets set differently)

whyrusleeping · 2016-05-31T01:03:13Z

Ah, its actually because now we can't actually specify --progress=false

RichardLitt · 2016-05-31T09:57:49Z

Adding back in 235-239 and showProgressBar should fix this; but that seems inelegant. Is there a better way, @whyrusleeping?

jbenet · 2016-08-26T07:57:58Z

core/commands/add.go

-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.


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>.

Where do you think we should point to in the ?

jbenet · 2016-08-26T08:07:10Z

I think this PR's language changes should be fixed.
And i think the progress bar changes should be either undone or fixed. the changelog looks like it has a bunch of other progress bar fixes. i'll update here if it's fixed.

I don't think it is, because:

> ipfs add /tmp/random
added QmRh7AkukqpqVuGYcH4rSj1rHYUZvQK6hjmbB9Z7NgiNNR random

> ipfs add /tmp/random --progress=false
added QmRh7AkukqpqVuGYcH4rSj1rHYUZvQK6hjmbB9Z7NgiNNR random
 0 B / 95.37 MB [-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------]   0.00%

The first one (no --progress=false) works fine. shows the progress bar moving, then clear is out to write the last line.
The second one had no bar until it was done, then it output this empty progress bar crap.
I think this is not coming from this PR but from another. i will investigate.

RichardLitt · 2016-08-26T14:53:25Z

core/commands/add.go

+		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),


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

This reverts most of the changes in da4a4ac, which were later found to be errorful. See #2657 for details. License: MIT Signed-off-by: Richard Littauer <richard.littauer@gmail.com>

This reverts commit da4a4ac, reversing changes made to 518f7e0. License: MIT Signed-off-by: Richard Littauer <richard.littauer@gmail.com>

In addition to removing the .Default option in the "add" options this also fixes the --progress option so --progress=false work again. This reverts commit da4a4ac, reversing changes made to 518f7e0. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

* commit 'cdd5285f16af665e5fd5d3592f53b2134281e76a': add cmd: clean up default logic of --progress option add cmd: use .Default(true) for pin option. Revert "Merge pull request ipfs#2657 from ipfs/feature/add-defaults-to-add" bitswap: add wantlist fullness to protobuf messages Enable parallel builds on CircleCI Fix PHONY name in Makefile Run coveralls if COVERALLS_TOKEN is set Switch unixfs.Metadata.MimeType to optional Fix bad formatting introduced by e855047 blockstore.AllKeyChan: fix/cleanup error handling blockstore.AllKeyChan: avoid channels by using the new NextSync method ulimit: handle freebsd ulimit code separately from the rest of the unixes Add test for flags. bitswap: increase wantlist resend delay to one minute ds-help: avoid unnecessary allocs when posssible and make use of RawKey fix formatting on error call "block rm": make channel large enough to avoid blocking # Conflicts: # Makefile # core/commands/add.go

RichardLitt added the topic/docs-ipfs Topic docs-ipfs label May 10, 2016

RichardLitt force-pushed the feature/add-defaults-to-add branch from 47bf289 to 42263d8 Compare May 10, 2016 21:18

RichardLitt force-pushed the feature/add-defaults-to-add branch from 42263d8 to 540267d Compare May 10, 2016 22:04

Kubuxu force-pushed the feature/add-defaults-to-add branch from 9b06067 to 87ec9d5 Compare May 17, 2016 19:08

Kubuxu added the need/author-input Needs input from the original author label May 17, 2016

RichardLitt force-pushed the feature/add-defaults-to-add branch from 87ec9d5 to 44fa353 Compare May 18, 2016 14:32

RichardLitt removed the need/author-input Needs input from the original author label May 18, 2016

Kubuxu force-pushed the feature/add-defaults-to-add branch 2 times, most recently from 3d6ead0 to 9183b5e Compare May 19, 2016 19:15

Kubuxu added the RFM label May 20, 2016

whyrusleeping reviewed May 20, 2016
View reviewed changes

whyrusleeping added need/author-input Needs input from the original author and removed RFM labels May 20, 2016

Add Defaults to ipfs add

6f796dc

I didn't bother with Chunker, because I think that is a much wider PR. These should all be solid, though. Redid some of the logic to make it smoother. Part of #2484. License: MIT Signed-off-by: Richard Littauer <richard.littauer@gmail.com>

RichardLitt force-pushed the feature/add-defaults-to-add branch from 9183b5e to 74bbb28 Compare May 21, 2016 11:38

RichardLitt added need/review Needs a review and removed need/author-input Needs input from the original author labels May 21, 2016

RichardLitt force-pushed the feature/add-defaults-to-add branch 2 times, most recently from 8d9820a to 84176a6 Compare May 21, 2016 13:17

Refactor progress and showProgressBar

03dd669

License: MIT Signed-off-by: Richard Littauer <richard.littauer@gmail.com>

RichardLitt force-pushed the feature/add-defaults-to-add branch from 84176a6 to 03dd669 Compare May 21, 2016 13:27

Kubuxu added RFM and removed need/review Needs a review labels May 25, 2016

whyrusleeping merged commit da4a4ac into master May 29, 2016

whyrusleeping deleted the feature/add-defaults-to-add branch May 29, 2016 06:27

kevina reviewed May 29, 2016
View reviewed changes

RichardLitt added a commit to ipfs-inactive/http-api-spec that referenced this pull request May 30, 2016

Add group add defaults, pin option

d8ae984

Following ipfs/kubo#2657

RichardLitt mentioned this pull request May 30, 2016

Add group add defaults, pin option ipfs-inactive/http-api-spec#93

Merged

jbenet reviewed Aug 26, 2016
View reviewed changes

RichardLitt reviewed Aug 26, 2016
View reviewed changes

jbenet mentioned this pull request Aug 28, 2016

Code Review 0.4.2..0.4.3-rc3 #3138

Closed

58 tasks

RichardLitt mentioned this pull request Sep 1, 2016

Revert da4a4ac and add more longDesc #3164

Closed

kevina mentioned this pull request Dec 4, 2016

Revert Add Defaults to ipfs add / Fix default logic of --progress option #3464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Defaults to `ipfs add` #2657

Add Defaults to `ipfs add` #2657

RichardLitt commented May 10, 2016

whyrusleeping commented May 10, 2016

RichardLitt commented May 10, 2016

whyrusleeping commented May 10, 2016

Kubuxu commented May 12, 2016

Kubuxu commented May 17, 2016

Kubuxu commented May 19, 2016

Kubuxu commented May 20, 2016

whyrusleeping May 20, 2016

RichardLitt commented May 21, 2016

Kubuxu commented May 25, 2016

kevina May 29, 2016

RichardLitt May 30, 2016

jbenet Aug 26, 2016

RichardLitt Aug 26, 2016

whyrusleeping commented May 31, 2016

whyrusleeping commented May 31, 2016

RichardLitt commented May 31, 2016

jbenet Aug 26, 2016

RichardLitt Aug 26, 2016

jbenet commented Aug 26, 2016 •

edited

Loading

RichardLitt Aug 26, 2016

Add Defaults to ipfs add #2657

Add Defaults to ipfs add #2657

Conversation

RichardLitt commented May 10, 2016

whyrusleeping commented May 10, 2016

RichardLitt commented May 10, 2016

whyrusleeping commented May 10, 2016

Kubuxu commented May 12, 2016

Kubuxu commented May 17, 2016

Kubuxu commented May 19, 2016

Kubuxu commented May 20, 2016

Choose a reason for hiding this comment

RichardLitt commented May 21, 2016

Kubuxu commented May 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented May 31, 2016

whyrusleeping commented May 31, 2016

RichardLitt commented May 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Aug 26, 2016 • edited Loading

Choose a reason for hiding this comment

Add Defaults to `ipfs add` #2657

Add Defaults to `ipfs add` #2657

jbenet commented Aug 26, 2016 •

edited

Loading