Simplify file wrapping logic #6121

Stebalien · 2019-03-22T21:53:50Z

I'm not sure if this is entirely correct but basically:

The adder now always adds files under a "root" file/directory.
All wrap logic is handled outside the file adder.

Thoughts? This would also require removing all wrap logic from the core interface.

Depends on ipfs/interface-go-ipfs-core#21.

Stebalien · 2019-03-22T22:02:04Z

core/coreunix/add.go

@@ -480,6 +403,9 @@ func outputDagnode(out chan<- interface{}, name string, dn ipld.Node) error {
 		return err
 	}

+	name = strings.TrimPrefix(name, rootFileName)
+	name = strings.TrimLeft(name, "/")


Moderately icky...

Actually, this whole "rootFileName" thing is kind of broken (for now at least). Your point about not using MFS for a single file is probably the right way to go.

Otherwise, we'll need to correctly strip the root filename everywhere we send out an event.

Stebalien · 2019-03-22T22:02:12Z

core/coreunix/add.go

-		err := adder.addFileNode(fpath, it.Node(), false)
+		node := it.Node()
+		err := adder.addFileNode(fpath, node)
+		node.Close()


I assume we need to close this here?

Yup (though I'd check errors here)

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

core/commands/add.go

 				output.Name = path.Join(name, output.Name)
 			}

+			if !wrap && output.Name == "" {


I switched this to check the wrap flag instead of "is this a directory". Is that correct?

I think we allowed "" only for wrapping directories, so this seems correct

magik6k

Tests are failing on some event stuff, not sure why, but overall 👍

magik6k · 2019-03-22T22:24:39Z

core/coreunix/add.go

-		err := adder.addFileNode(fpath, it.Node(), false)
+		node := it.Node()
+		err := adder.addFileNode(fpath, node)
+		node.Close()


Yup (though I'd check errors here)

magik6k · 2019-03-22T22:41:40Z

core/commands/add.go

 				output.Name = path.Join(name, output.Name)
 			}

+			if !wrap && output.Name == "" {


I think we allowed "" only for wrapping directories, so this seems correct

Stebalien · 2019-03-23T03:52:54Z

core/coreapi/unixfs.go


-	fileAdder, err := coreunix.NewAdder(ctx, pinning, addblockstore, dserv)
+	fileAdder, err := coreunix.NewAdder(pinning, addblockstore, dserv)


TL;DR:

We can now pass a nil dagservice to "only hash". I needed to move this functionality down into the adder because I removed the SetMfsRoot function.

Instead of setting adder.Pin, we now just pass a nil pinner to disable pinning. Really, I only needed 1 (see below) but I figured I'd make these consistent.

Alternatively, I could have added an OnlyHash option to the file adder.

Background:

I wanted to move away from MFS for adding single files to get away from all the directory wrapping issues. However, the old adder kind of assumed MFS everywhere. It was technically possible to just not use it but I was worried about leaving the adder in an inconsistent state.

The old adder wasn't safely reusable (as far as I can tell).

So I refactored it into:

Adder: A reusable, immutable "adder" to carry all the add options, dag/pin/gc services, etc.

adderJob: job for adding files/symlinks (embeds the Adder).

dirAdderJob: job for adding directories (with mfs support) (embeds adderJob).

How does that relate to the changes above? Well, Adder no longer stores the MFS root (because adder is immutable/reusable). That meant SetMfsRoot had to go.

Really, we probably shouldn't be using MFS for importing files at all. I'm not sure why we are but we can fix that later.

Stebalien · 2019-03-23T03:53:13Z

core/coreunix/add.go

+	return ch
+}
+func (noopDagService) Remove(_ context.Context, _ cid.Cid) error       { return nil }
+func (noopDagService) RemoveMany(_ context.Context, _ []cid.Cid) error { return nil }


Should probably go elsewhere but I don't feel like bubbling go-ipld-format.

Stebalien · 2019-03-23T03:53:48Z

core/coreunix/add.go

+	rnode.SetCidBuilder(job.CidBuilder)
+	ds := job.dagService
+	if ds == nil {
+		ds = mdutils.Mock()


This mock dag should also go in go-ipld-format.

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

This makes it a bit easier to reason about the state of the adder. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

This way, we can entirely avoid MFS when adding single files. This patch also changes how Pin/OnlyHash works. Instead of having a Pin option, just construct the adder with a `nil` pinner to not pin. Likewise, we handle "only hash" by passing a nil dagservice. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

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

Stebalien · 2019-03-23T05:09:18Z

There's still something wrong with this...

Stebalien · 2019-03-25T18:51:26Z

(not worth debugging for now)

Stebalien requested a review from Kubuxu as a code owner March 22, 2019 21:53

Stebalien mentioned this pull request Mar 22, 2019

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

Merged

4 tasks

Stebalien force-pushed the fix/unixfs-add-wrap-simplify branch from d0021bb to 0a2c877 Compare March 22, 2019 22:00

ghost assigned Stebalien Mar 22, 2019

ghost added the status/in-progress In progress label Mar 22, 2019

Stebalien commented Mar 22, 2019

View reviewed changes

Stebalien mentioned this pull request Mar 22, 2019

remove Wrap ipfs/interface-go-ipfs-core#21

Merged

Stebalien force-pushed the fix/unixfs-add-wrap-simplify branch from 0a2c877 to 376d9ff Compare March 22, 2019 22:21

magik6k reviewed Mar 22, 2019

View reviewed changes

Stebalien force-pushed the fix/unixfs-add-wrap-simplify branch from 62e215d to 1fc7096 Compare March 23, 2019 02:56

Stebalien commented Mar 23, 2019

View reviewed changes

Stebalien force-pushed the fix/unixfs-add-wrap-simplify branch from 810544b to 79c12b2 Compare March 23, 2019 04:20

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

Stebalien changed the base branch from fix/unixfs-add-wrap to master March 23, 2019 04:52

Stebalien added 8 commits March 22, 2019 21:54

files: remove toplevel stuff

e076aac

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

wip: update interface-go-ipfs-core

cbfcc4e

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

add: remove wrap logic

409d269

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

add: avoid materializing the super-root

6138712

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

adder: split add options from job

22ed66b

This makes it a bit easier to reason about the state of the adder. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

add: check close errors

3f76a28

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

add: fix symlinks

831b765

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

Stebalien force-pushed the fix/unixfs-add-wrap-simplify branch from cd5d6b4 to 831b765 Compare March 23, 2019 05:09

magik6k mentioned this pull request Mar 25, 2019

coreapi: remove Unixfs.Wrap #6123

Merged

Stebalien closed this Mar 25, 2019

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

hacdias deleted the fix/unixfs-add-wrap-simplify branch May 9, 2023 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify file wrapping logic #6121

Simplify file wrapping logic #6121

Stebalien commented Mar 22, 2019 •

edited

Loading

Stebalien Mar 22, 2019

Stebalien Mar 22, 2019

Stebalien Mar 22, 2019

magik6k Mar 22, 2019

Stebalien Mar 22, 2019

magik6k Mar 22, 2019

magik6k left a comment

magik6k Mar 22, 2019

magik6k Mar 22, 2019

Stebalien Mar 23, 2019

Stebalien Mar 23, 2019

Stebalien Mar 23, 2019

Stebalien commented Mar 23, 2019

Stebalien commented Mar 25, 2019


		fileAdder, err := coreunix.NewAdder(ctx, pinning, addblockstore, dserv)
		fileAdder, err := coreunix.NewAdder(pinning, addblockstore, dserv)

Simplify file wrapping logic #6121

Simplify file wrapping logic #6121

Conversation

Stebalien commented Mar 22, 2019 • 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

magik6k left a 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

Stebalien commented Mar 23, 2019

Stebalien commented Mar 25, 2019

Stebalien commented Mar 22, 2019 •

edited

Loading