coreapi unixfs: change Wrap logic to make more sense #6019

magik6k · 2019-02-25T16:48:45Z

Currently coreunix.Adder wrap logic is weird, which causes coreapi.Unixfs().Add to also be weird:

data	wrap	hidden	hash
(file)	❌	❌	(QmFile)
(file)	✔️	❌	(QmWrapper)/QmFile
(dir)/(file)	❌	❌	(QmFile)
(dir)/(file)	✔️	❌	(QmDir)/filename
(dir)/(.file)	✔️	❌	(QmDir)/.filename
(dir)/(.file)	✔️	✔️	(QmDir)/.filename

With this PR it does this:

data	wrap	hidden	hash
(file)	❌	❌	(QmFile)
(file)	✔️	❌	(QmWrapper)/QmFile
(dir)/(file)	❌	❌	(QmDir)/filename
(dir)/(file)	✔️	❌	(QmWrapper)/QmDir/filename
(dir)/(.file)	✔️	❌	(QmWrapper)/QmEmptyDir/
(dir)/(.file)	✔️	✔️	(QmWrapper)/(QmDir)/.filename

New behaviour IMO makes much more sense from API consumer perspective.

TODO:

Cleanup Adder code
Fix regressions
Merge Unixfs.Wrap Fixes interface-go-ipfs-core#10
Merge Handle stdin name in cli/parse go-ipfs-cmds#157

Reviews welcome

Kubuxu · 2019-02-25T16:59:44Z

As far as I know, --warp main use case is/was preserving the name of a file when a file is added.

I can see how it is a bit weird that it doesn't happen in case of directories but I have to highlight that it is quite a big breaking change and I would expect would break existing workflows and scripts.

magik6k · 2019-02-25T17:05:35Z

ipfs add --wrap won't be affected by this change, I just didn't fix it yet.

core/coreapi/unixfs.go

core/coreunix/add.go

core/commands/add.go

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Stebalien

I'm having trouble understanding this.

What's this "top level" thing and can we get rid of it? It seems like we're mixing two code paths with some shared logic.
Why do I care if a file is a directory or not when adding it?

Stebalien · 2019-03-22T20:19:19Z

core/coreunix/add.go

+
+	// if adding a file without wrapping, swap the root to it (when adding a
+	// directory, mfs root is the directory)
+	_, dir := file.(files.Directory)


I'm having trouble understanding this comment. Why do we care if the file we're adding is a directory?

It made sense before I rewrote this code.

Stebalien · 2019-03-22T21:55:27Z

Here's my attempt to simplify this: #6121. It may be totally wrong but it'll at least help me better understand how this all works.

magik6k · 2019-03-22T21:56:19Z

Why do I care if a file is a directory or not when adding it?

When adding a directory, we add its entries to mfs (so mfs root is the directory). When adding a file, we also add it to mfs with its hash as the name so that wrap works.

If we could drop wrap from adder/coreapi it would make this much simpler (we wouldn't use mfs at all when adding single files)

Stebalien · 2019-03-22T22:40:57Z

If we could drop wrap from adder/coreapi it would make this much simpler (we wouldn't use mfs at all when adding single files)

Unless you can think of a reason not to, let's just do that. It's really trivial to wrap a file anyways.

jmank88 · 2019-03-23T01:27:22Z

go.mod

@@ -33,12 +33,12 @@ require (
 	github.com/ipfs/go-ipfs-blocksutil v0.0.1
 	github.com/ipfs/go-ipfs-chunker v0.0.1
 	github.com/ipfs/go-ipfs-cmdkit v0.0.1
-	github.com/ipfs/go-ipfs-cmds v0.0.2
+	github.com/ipfs/go-ipfs-cmds v0.0.3


Needs v0.0.4 to cover #6118

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien

(ok, I'm not going to block this on my now massive PR)

coreapi unixfs: change Wrap logic to make more sense This commit was moved from ipfs/kubo@8c96e3b

ghost assigned magik6k Feb 25, 2019

ghost added the status/in-progress In progress label Feb 25, 2019

magik6k mentioned this pull request Feb 25, 2019

Unixfs.Wrap Fixes ipfs/interface-go-ipfs-core#10

Merged

magik6k mentioned this pull request Feb 26, 2019

Release 0.4.19 #6026

Merged

magik6k force-pushed the fix/unixfs-add-wrap branch from 0f22695 to b80801f Compare March 4, 2019 13:01

Stebalien reviewed Mar 7, 2019

View reviewed changes

core/coreapi/unixfs.go Outdated Show resolved Hide resolved

core/coreunix/add.go Outdated Show resolved Hide resolved

magik6k force-pushed the fix/unixfs-add-wrap branch 3 times, most recently from c5b6009 to 9e2b237 Compare March 11, 2019 12:36

magik6k mentioned this pull request Mar 11, 2019

Remove hidden file handling ipfs/interface-go-ipfs-core#17

Closed

alanshaw mentioned this pull request Mar 11, 2019

Re-evaluate hidden file handling still has interop with go-ipfs ipfs/js-ipfs#1917

Closed

Stebalien reviewed Mar 11, 2019

View reviewed changes

core/commands/add.go Outdated Show resolved Hide resolved

magik6k added 9 commits March 21, 2019 19:17

coreapi unixfs: change Wrap logic to make more sense

332d4ae

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi add wrap: fix gotests

2e8bb8c

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

WIP almost working ipfs add

b8bc26a

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: remove hidden file handling in add

c5b81e9

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

gomod: use interface-go-ipfs-core@8da6e5f68c0

6a36a44

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

add: gofmt

150fa15

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

add: fix sharness tests (WIP)

782b2d6

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

unixfs add: remove StdinName

964ef0b

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

adder: pin the right node

152829d

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the fix/unixfs-add-wrap branch from 2b0ea9f to 152829d Compare March 21, 2019 18:20

magik6k added 2 commits March 22, 2019 00:45

add: move stdin-name handling to cmds

9919528

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Update go-ipfs-cmds

cfa4862

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the fix/unixfs-add-wrap branch from 36afdea to cfa4862 Compare March 21, 2019 23:50

add: gofmt

a94d3b7

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k mentioned this pull request Mar 22, 2019

Handle stdin name in cli/parse ipfs/go-ipfs-cmds#157

Merged

magik6k marked this pull request as ready for review March 22, 2019 00:11

magik6k requested a review from Kubuxu as a code owner March 22, 2019 00:11

magik6k changed the title ~~[RFC] coreapi unixfs: change Wrap logic to make more sense~~ coreapi unixfs: change Wrap logic to make more sense Mar 22, 2019

magik6k requested a review from Stebalien March 22, 2019 16:21

Update go-ipfs-cmds and core-iface

01ef512

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the fix/unixfs-add-wrap branch from 3b75b06 to 01ef512 Compare March 22, 2019 16:59

Stebalien mentioned this pull request Mar 22, 2019

support add --nocopy <url> #6118

Closed

ghost assigned Stebalien Mar 22, 2019

Stebalien reviewed Mar 22, 2019

View reviewed changes

jmank88 reviewed Mar 23, 2019

View reviewed changes

dep: update files, cmds, interfaces

2d96b4d

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the fix/unixfs-add-wrap branch from becec73 to 2d96b4d Compare March 23, 2019 04:45

Stebalien approved these changes Mar 23, 2019

View reviewed changes

Stebalien merged commit 8c96e3b into master Mar 23, 2019

ghost removed the status/in-progress In progress label Mar 23, 2019

Stebalien deleted the fix/unixfs-add-wrap branch March 23, 2019 04:52

jmank88 mentioned this pull request Mar 23, 2019

Support for ipfs add <url> #6065

Closed

magik6k mentioned this pull request Apr 25, 2019

ipfs add with two files only adds one of them in 0.4.20 #6254

Closed

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#6019 from ipfs/fix/unixfs-add-wrap

0a211b6

coreapi unixfs: change Wrap logic to make more sense This commit was moved from ipfs/kubo@8c96e3b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coreapi unixfs: change Wrap logic to make more sense #6019

coreapi unixfs: change Wrap logic to make more sense #6019

magik6k commented Feb 25, 2019 •

edited by Stebalien

Loading

Kubuxu commented Feb 25, 2019

magik6k commented Feb 25, 2019

Stebalien left a comment

Stebalien Mar 22, 2019

magik6k Mar 22, 2019

Stebalien commented Mar 22, 2019

magik6k commented Mar 22, 2019

Stebalien commented Mar 22, 2019

jmank88 Mar 23, 2019

Stebalien left a comment

coreapi unixfs: change Wrap logic to make more sense #6019

coreapi unixfs: change Wrap logic to make more sense #6019

Conversation

magik6k commented Feb 25, 2019 • edited by Stebalien Loading

Kubuxu commented Feb 25, 2019

magik6k commented Feb 25, 2019

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Mar 22, 2019

Choose a reason for hiding this comment

magik6k Mar 22, 2019

Choose a reason for hiding this comment

Stebalien commented Mar 22, 2019

magik6k commented Mar 22, 2019

Stebalien commented Mar 22, 2019

jmank88 Mar 23, 2019

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

magik6k commented Feb 25, 2019 •

edited by Stebalien

Loading