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.

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

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

Merged
merged 14 commits into from
Mar 23, 2019

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Feb 25, 2019

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:

Reviews welcome

@ghost ghost assigned magik6k Feb 25, 2019
@ghost ghost added the status/in-progress In progress label Feb 25, 2019
@Kubuxu
Copy link
Member

Kubuxu commented Feb 25, 2019

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
Copy link
Member Author

magik6k commented Feb 25, 2019

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

core/coreapi/unixfs.go Outdated Show resolved Hide resolved
core/coreunix/add.go Outdated Show resolved Hide resolved
core/commands/add.go Outdated Show resolved Hide resolved
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k magik6k marked this pull request as ready for review March 22, 2019 00:11
@magik6k magik6k requested a review from Kubuxu as a code owner March 22, 2019 00:11
@magik6k 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 magik6k requested a review from Stebalien March 22, 2019 16:21
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

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?


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It made sense before I rewrote this code.

@Stebalien
Copy link
Member

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
Copy link
Member Author

magik6k commented Mar 22, 2019

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
Copy link
Member

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.

go.mod Outdated
@@ -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
Copy link

Choose a reason for hiding this comment

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

Needs v0.0.4 to cover #6118

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

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

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

@Stebalien Stebalien merged commit 8c96e3b into master Mar 23, 2019
@ghost ghost removed the status/in-progress In progress label Mar 23, 2019
@Stebalien Stebalien deleted the fix/unixfs-add-wrap branch March 23, 2019 04:52
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
coreapi unixfs: change Wrap logic to make more sense 

This commit was moved from ipfs/kubo@8c96e3b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants