Add full support for CidV1 in Files API and Dag Modifier #4026

kevina · 2017-07-01T08:49:49Z

kevina · 2017-07-01T08:51:24Z

Tests are failing. Working on fixing.

Also plan to add a "--raw-leaves" option to the files api.

kevina · 2017-08-09T08:20:36Z

Support for raw leaves is done.

I am going to tack on full CifV1 support and also support for setting a alternative hash function since they are very closely related. If necessary I can separate out CidV1 support into a new commit.

kevina · 2017-08-13T18:29:14Z

Full support for CidV1 (including using an alternative hash function is now done) using the following strategy (slightly modified from #3921 (comment))

When a file or directory is modified the new hash and any new interior nodes in the dag will use the same prefix (which will preserve the Cid version and hash function).
When a new file or directory is created it will use the same prefix as the directory it is added to, by default. The --cid-version and --hash-fun option can be used to specify a different prefix instead.
If the Cid version is 0 legacy leaf nodes will be used, otherwise raw leaves will be used. This behavior can be changed by using the --raw-leaves option when modifying a file.
In order to be able to change the MFS root I added a update command to the files api. When called it will update the prefix (cid version and hash function) of a directory.

If any part of this is controversial or needs more though it can easily be factored out of this P.R. and into a new one.

kevina · 2017-09-02T19:34:43Z

@whyrusleeping this has been ready for review for awhile now

The only outstanding issue if the files update command (ref #4171) which is needed to update the root to something other than CIDv0. This part doesn't have to go in, but many of the tests currently rely on it and will have to be redone.

I wrote a new utility cid-fmt (see ipfs/go-cid#31) which can greatly help with these tests.

whyrusleeping · 2017-09-06T23:57:54Z

core/commands/files/files.go

+			root.Prefix = prefix
+		}
+
+		err = mfs.Mkdir(root, dirtomake, dashp, flush)


Hrm.. this is getting complicated. Its starting to feel like we need some sort of MkdirOpts here that hold the dashp, flush, and prefix options for the operation.

That would work. Are you thinking:

mfs.Mkdir(root, dirtomake, mfs.MkdirOpts{Dashp: dashp, Flush: flush, Prefix: prefix})

whyrusleeping · 2017-09-06T23:58:35Z

core/commands/files/files.go

@@ -756,6 +794,69 @@ are run with the '--flush=false'.
 	},
 }

+var FilesUpdateCmd = &cmds.Command{


not yet sold on update as the command name, but its an okay placeholder for now.

whyrusleeping · 2017-09-06T23:59:12Z

core/commands/files/files.go

@@ -756,6 +794,69 @@ are run with the '--flush=false'.
 	},
 }

+var FilesUpdateCmd = &cmds.Command{
+	Helptext: cmds.HelpText{
+		Tagline: "Change the cid version of hash function of the root node of a given path.",


s/of/or/

Also, given this description, maybe we should call the command re-cid ?

How about chcid?

whyrusleeping · 2017-09-07T00:06:26Z

core/commands/files/files.go

+	Helptext: cmds.HelpText{
+		Tagline: "Change the cid version of hash function of the root node of a given path.",
+		ShortDescription: `
+Flush a given path to disk. This is only useful when other commands


ShortDescription needs fixing

Oops, will fix.

whyrusleeping · 2017-09-07T00:06:34Z

core/commands/files/files.go

+`,
+	},
+	Arguments: []cmds.Argument{
+		cmds.StringArg("path", false, false, "Path to flush. Default: '/'."),


update text here

Oops, will fix.

whyrusleeping · 2017-09-07T00:07:56Z

core/commands/files/files.go

-func getFileHandle(r *mfs.Root, path string, create bool) (*mfs.File, error) {
+func getPrefix(req cmds.Request) (*cid.Prefix, error) {
+	cidVer, cidVerSet, _ := req.Option("cid-version").Int()
+	hashFunStr, hashFunSet, _ := req.Option("hash-fun").String()


i'm not sure we should use hash-fun. ipfs add just uses --hash, i'm thinking we should be consistent.

@whyrusleeping That won't work. The --hash option is already used in one of the sub-commands and will create a conflict.

kevina · 2017-09-07T00:24:00Z

core/commands/files/files.go

+		// ^^fixme: can't use just "hash" as the option name as the
+		// conflicts with "--hash" usage by the stat command, this is
+		// unfortunate as it creates an inconsistency with the "add"
+		// that uses "hash"
 	},


@whyrusleeping here is the comment on why I can't use "hash" as the option name

Can't we just have the --hash option for stat be different from the one on the other commands?

We could, if you are willing to change the interface for stat. That is something I didn't want to do.

@whyrusleeping another option is to use --mhash instead of --hash-fun this is more specific (multi-hash) and is a good name for eventually making it a global option.

I still don't think this is blocking anything. Stat is a separate command, it can have a flag that means something different, having those options on the ipfs files command doesnt make sense for a lot of cases too. ipfs add, which will at some point be moved under the ipfs files command also uses --hash. We need to stay consistent here.

Stat is a separate command, it can have a flag that means something different

Not with the current API. Currently the --hash-fun function is a global option for all ipfs files command. The ipfs files stat commandalso has the--hashoption. It creates this error when thestat` command is used:

$ ipfs files stat Error: Option name 'hash' used multiple times

To resolve this conflict I need to either move the --hash option into each of the subcommands, or rename the --stat option in ipfs files stat to something else. If I move the --hash option to each of the subcommands I should likely move the --raw-leaves and --cid-ver option also to be consistent.

Currently the --hash-fun function is a global option for all ipfs files command.

dont do that.

I need to either move the --hash option into each of the subcommands

Yes, do that.

kevina · 2017-09-11T18:28:23Z

@whyrusleeping I pushed some commits to address your feedback, I changed update to chcid.

whyrusleeping · 2017-10-19T11:43:37Z

test/sharness/t0250-files-api.sh

@@ -97,16 +97,17 @@ test_sharding() {
 }

 test_files_api() {
-	local EXTRA ARGS
+	local EXTRA ARGS WRITE_ARGS


did you mean to call this RAW_LEAVES ?

whyrusleeping · 2017-10-19T11:46:34Z

Alright, this looks good to me. Going to need to rebase, and then it would be nice to have a few more of the codeclimate issues resolved before merging.

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina · 2017-10-19T17:59:58Z

@whyrusleeping rebased and cleaned up some of the codeclimate issues

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

whyrusleeping · 2017-10-19T23:29:24Z

Thanks @kevina! we're entering the brave new world of cidV1..

kevina · 2017-10-19T23:35:34Z

Your Welcome. I image there is still more work to do, but this should hopefully make CidV1 practical to use by default. :)

kevina added the status/in-progress In progress label Jul 1, 2017

kevina mentioned this pull request Jul 1, 2017

Improve DagModifier to fully support CIDv1 #3921

Closed

kevina force-pushed the kevina/files-raw-leaves branch 2 times, most recently from b2c7b6e to a740592 Compare August 8, 2017 22:19

kevina changed the title ~~WIP: Add full support for raw leaves in files API and Dag Modifier~~ Add full support for raw leaves in files API and Dag Modifier and more Aug 9, 2017

kevina force-pushed the kevina/files-raw-leaves branch from dcff359 to 321b72d Compare August 13, 2017 09:15

kevina changed the title ~~Add full support for raw leaves in files API and Dag Modifier and more~~ Add full support for CidV1 in Files API and Dag Modifier Aug 13, 2017

kevina added the need/review Needs a review label Aug 13, 2017

kevina requested a review from whyrusleeping August 13, 2017 18:30

kevina force-pushed the kevina/files-raw-leaves branch 5 times, most recently from c9d16c5 to 872c7a7 Compare August 16, 2017 23:18

Kubuxu self-requested a review August 22, 2017 23:37

This was referenced Aug 26, 2017

Provide way to update the CID prefix of DAG Objects #4171

Open

WIP Fix DagModifier for raw-leaves nodes #3901

Closed

whyrusleeping reviewed Sep 6, 2017

View reviewed changes

whyrusleeping reviewed Sep 7, 2017

View reviewed changes

kevina commented Sep 7, 2017

View reviewed changes

kevina force-pushed the kevina/files-raw-leaves branch from 1a70c1d to 28dcccd Compare September 10, 2017 23:46

whyrusleeping reviewed Oct 19, 2017

View reviewed changes

kevina force-pushed the kevina/files-raw-leaves branch from 5e96b61 to 14ff11b Compare October 19, 2017 15:53

kevina added 4 commits October 19, 2017 12:59

Test raw leaves in trickle dag tests.

189cf0c

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Provide support for raw leaves in DAG modifier.

7302c3a

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Add "--raw-leaves" option to "ipfs files"

a3bd2c2

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Enable CidV1 (and other prefixes) in the Dag Modifier.

2e15dcb

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina force-pushed the kevina/files-raw-leaves branch from 30ddb19 to 54ab3d2 Compare October 19, 2017 17:57

kevina force-pushed the kevina/files-raw-leaves branch from 54ab3d2 to 6d1d38c Compare October 19, 2017 18:04

kevina added 11 commits October 19, 2017 14:14

Test for alternative hash function in Dag Modifier.

77e9b8d

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

mfs: inherit CID prefix from from parent directory

f2fbfdf

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Add --cid-version and --hash-fun option to files API

18a6344

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Add "files update" command.

63ddffd

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Enhance sharness tests.

47c198c

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Fix help text for "update" command.

fd7b72f

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Eliminate Prefix field from MFS root, use MkdirOpts.

d29930c

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Rename "files update" to "files chcid".

b5b61f7

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Address C.R. in t0250-files-api.sh.

00a3b1d

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Make --hash and related option local to sub-commands that use them.

798afed

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Documentation.

0182e59

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina force-pushed the kevina/files-raw-leaves branch from 6d1d38c to 0182e59 Compare October 19, 2017 18:15

Fix "files stat" to work on raw nodes.

3003f93

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

whyrusleeping approved these changes Oct 19, 2017

View reviewed changes

whyrusleeping merged commit e1f158b into master Oct 19, 2017

whyrusleeping deleted the kevina/files-raw-leaves branch October 19, 2017 22:56

kevina removed need/review Needs a review status/in-progress In progress labels Oct 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add full support for CidV1 in Files API and Dag Modifier #4026

Add full support for CidV1 in Files API and Dag Modifier #4026

kevina commented Jul 1, 2017 •

edited

Loading

kevina commented Jul 1, 2017

kevina commented Aug 9, 2017

kevina commented Aug 13, 2017

kevina commented Sep 2, 2017 •

edited

Loading

whyrusleeping Sep 6, 2017

kevina Sep 7, 2017

whyrusleeping Sep 6, 2017

whyrusleeping Sep 6, 2017

kevina Sep 7, 2017

whyrusleeping Sep 7, 2017

kevina Sep 7, 2017

whyrusleeping Sep 7, 2017

kevina Sep 7, 2017

whyrusleeping Sep 7, 2017

kevina Sep 7, 2017 •

edited

Loading

kevina Sep 7, 2017

whyrusleeping Sep 11, 2017

kevina Sep 11, 2017

kevina Oct 17, 2017

whyrusleeping Oct 18, 2017

kevina Oct 18, 2017

whyrusleeping Oct 18, 2017

kevina Oct 18, 2017

kevina commented Sep 11, 2017

whyrusleeping Oct 19, 2017

whyrusleeping commented Oct 19, 2017

kevina commented Oct 19, 2017

whyrusleeping commented Oct 19, 2017

kevina commented Oct 19, 2017 •

edited

Loading

Add full support for CidV1 in Files API and Dag Modifier #4026

Add full support for CidV1 in Files API and Dag Modifier #4026

Conversation

kevina commented Jul 1, 2017 • edited Loading

kevina commented Jul 1, 2017

kevina commented Aug 9, 2017

kevina commented Aug 13, 2017

kevina commented Sep 2, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Sep 7, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

kevina commented Sep 11, 2017

Choose a reason for hiding this comment

whyrusleeping commented Oct 19, 2017

kevina commented Oct 19, 2017

whyrusleeping commented Oct 19, 2017

kevina commented Oct 19, 2017 • edited Loading

kevina commented Jul 1, 2017 •

edited

Loading

kevina commented Sep 2, 2017 •

edited

Loading

kevina Sep 7, 2017 •

edited

Loading

kevina commented Oct 19, 2017 •

edited

Loading