Refactor UnixFS CoreAPI #5501

magik6k · 2018-09-20T14:55:51Z

This implements most options applying to single files supported by ipfs add.

TODO:

--pin, --only-hash
docstrings
option handling cleanup

To keep this PR manageable, I'm leaving multi-file / filestore / etc. for next PRs

kevina · 2018-09-28T21:53:40Z

This seams like a overcomplicated way of going about it. Was this discussed with any one before?

Stebalien · 2018-09-28T22:12:36Z

@kevina which part?

It's using the functional options pattern (https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis) which we've started using pretty much everywhere. It allows us to easily extend these options.

kevina · 2018-09-28T22:17:59Z

I will withhold judgement until I will have a closer look at the "functional options pattern" link provided. (It just seams to me that it is adding a lot of not necessary useful indirection.)

kevina · 2018-09-29T02:14:49Z

core/coreapi/interface/options/unixfs.go

+// directly into CID instead of being stored and addressed by it's hash
+//
+// Note that while there is no hard limit on the number of bytes here, it should
+// be kept at something reasonably low like 32b (default for 'ipfs add')


The wording here makes it sounds like the value should be 32 bytes or smaller. That is misleading. Values less than 32 bytes should not be encouraged. The default of 32 was chosen so that when the Inline option is enabled it won't increase the size of the hashes, but a limit of 32 bytes is not very usefel. A more reasonable upper limit would be 64 bytes, but if you are not concerned with displaying it than values like 128 or even 256 are also okay and based on my informal testing do not cause problems anywhere in IPFS.

kevina

I don't like the way the Inline option is handled.

core/coreapi/interface/options/unixfs.go

kevina · 2018-09-29T06:42:54Z

core/coreapi/unixfs_test.go

@@ -38,7 +40,6 @@ var hello = "/ipfs/QmQy2Dw4Wk7rdJKjThjYXzfFJNaRKRHhHP5gHHXroJMYxk"
 var helloStr = "hello, world!"

 // `echo -n | ipfs add`
-var emptyFile = "/ipfs/QmbFMke1KXqnYyBBWxB74N4c5SBnJMVAiMNRcGu6x1AwQH"


Is there a reason for removing this?

kevina · 2018-09-29T06:51:39Z

It's using the functional options pattern

Okay. That is actually a pretty good idea and I take back my earlier comments.

My personal preference is to complete the transformation and convert the add command to use the new code. This way we can be sure we don't miss anything, but I can understand if we don't want to do that all at once.

kevina · 2018-10-02T16:15:26Z

core/coreapi/interface/options/unixfs.go

+// Note that while there is no hard limit on the number of bytes, it should
+// be kept at a reasonably low value, like 64 bytes if you intend to display
+// these hashes. Larger values like 256 bytes will work fine, but may affect
+// de-duplication of smaller blocks


Where did you get the de-duplication of smaller blocks from?

Maybe the wording isn't quite clear. The idea is that when you have many duplicated small blocks (100s of bytes), and a large inline limit, you can end up with quite a bit bigger DAG (i.e. the cumulative block size of the dag would be bigger).

If this is wrong / not clear enough, can you suggest better wording here?

I didn't think of that.

My main concern with larger blocks is if they get too large they might cause a problem somewhere even if they work find 98% of the time.

kevina · 2018-10-02T16:15:30Z

core/coreapi/interface/options/unixfs.go

 func (unixfsOpts) InlineLimit(limit int) UnixfsAddOption {
 	return func(settings *UnixfsAddSettings) error {
+		settings.InlineBlocks = true


I'm not sure I would do this, setting the InlineLimit on the command line does not enable inlining.

On the other hand, what are the cases somebody (other than in the command implementation) would set this option and then not want to inline blocks? It feels a bit like saying 'I want to inline blocks up to 123 bytes and I want to inline blocks'

A user might want to change the defaults. That is, I might want to create a set of default options that change the default inline limit without actually enabling inlining (unless requested by the user).

Note: I don't actually care one way or the other about this.

Well, yeah. The options case makes enough sense, I'll go with that

Yeah, I was thinking along the same line as @Stebalien. Also, since I assume the idea is to switch the ipfs add command to use the new API it best to keep the behavior constant.

kevina · 2018-10-02T16:16:25Z

core/coreapi/interface/options/unixfs.go

@@ -124,13 +126,26 @@ func (unixfsOpts) RawLeaves(enable bool) UnixfsAddOption {
 	}
 }

+// InlineBlocks tells the adder to inline small blocks into CIDs
+func (unixfsOpts) InlineBlocks(enable bool) UnixfsAddOption {


I think just Inline will be clear enough. It will also be consistent with the command line.

kevina · 2018-10-02T22:52:06Z

core/coreapi/interface/options/unixfs.go

+// de-duplication of smaller blocks.
+//
+// Setting this value too high may cause various problems, such as render some
+// blocks unfetchable


I think we are saying too much here and at this point will just be confusing the user. I would just simplify this with

Note that while there is no hard limit on the number of bytes, it should be kept at a reasonably low value, such as 64 and no more then 1k. Setting this value too high may cause various problems, such as render some blocks unfetchable.

@Stebalien thoughts?

Changes Addressed.

kevina · 2018-10-02T23:13:31Z

@Stebalien do we want to get this in as is? I personally would rather we wait until all the options are implemented and the ipfs add command is converted. This was we get the benefit of all the sharness tests to make sure here are no bugs or changes in behavior.

Stebalien · 2018-10-02T23:23:04Z

I kind of agree. @magik6k does it look like that would be a large addition to this patch?

magik6k · 2018-10-02T23:38:07Z

I don't really know, it kind of depends on how well go-ipfs-cmdkit/files stuff will fit here, I just didn't fiddle with that yet, so it might be anywhere from a few small commits to a few-thousand-diff-lines monster (I'm quite positive that this will be on the manageable side).

I'm fine with going either way, just want to make sure this doesn't get frozen because it grew too big.

kevina · 2018-10-02T23:52:57Z

@magik6k I would attempt to do the complete conversion. If it becomes unmanageable then I would split out parts into a separate p.r.

I would rather not have lots of duplicate code in go-ipfs if this commit lands and then ipfs add doesn't get converted for some reason.

Stebalien · 2018-10-02T23:53:31Z

I'm fine with going either way, just want to make sure this doesn't get frozen because it grew too big.

We can also do that as a followup PR against this one.

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

kevina

I left one comment in an earlier review about documentation for the InlineLimit option that still needs to be addressed.

Otherwise this mostly LGTM and seams solidly done. I won't be able to go over this with a fine tooth comb but will look it over more closely in a day or so as I don't want this too go to long before merging.

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

kevina · 2018-10-04T18:47:26Z

core/coreapi/interface/options/unixfs.go

-// Setting this value too high may cause various problems, such as render some
-// blocks unfetchable
+// Note that while there is no hard limit on the number of bytes, it should be
+// kept at a reasonably low value, such as 64 and no more than 1k. Setting this


@Stebalien should we say 1k here or use a lower value?

What about "it should be kept at a reasonably low value, such as 64; implementations may choose to reject anything larger". We can always increase this if someone complains.

(Note: I never really intend to reject anything less than 1k but users tend to ignore all but strongly worded warnings).

Okay. I was actually trying to avoid creating too strong a suggestion. But I am okay with.

// Note that while there is no hard limit on the number of bytes, it should be // kept at a reasonably low value, such as 64; implementations may choose to // reject anything larger.

And just leave it at that.

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

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

Stebalien · 2018-10-05T18:18:44Z

😅 merged before we get any new conflicts.

Refactor UnixFS CoreAPI This commit was moved from ipfs/kubo@5ba0976

magik6k added the topic/core-api Topic core-api label Sep 20, 2018

magik6k requested a review from Kubuxu as a code owner September 20, 2018 14:55

ghost assigned magik6k Sep 20, 2018

ghost added the status/in-progress In progress label Sep 20, 2018

magik6k mentioned this pull request Sep 20, 2018

CoreAPI progress tracking issue #4498

Open

51 tasks

magik6k requested a review from Stebalien September 20, 2018 22:03

magik6k changed the title ~~WIP: More options for coreapi.unixfs.Add~~ More options for coreapi.unixfs.Add Sep 25, 2018

magik6k force-pushed the feat/coreapi/unixfs branch from 8647489 to a724bbc Compare September 25, 2018 14:29

Stebalien requested a review from kevina September 28, 2018 21:08

kevina reviewed Sep 29, 2018

View reviewed changes

kevina previously requested changes Sep 29, 2018

View reviewed changes

magik6k force-pushed the feat/coreapi/unixfs branch from fad4462 to 8535fd7 Compare October 2, 2018 07:43

Stebalien requested a review from kevina October 2, 2018 20:09

kevina reviewed Oct 2, 2018

View reviewed changes

magik6k force-pushed the feat/coreapi/unixfs branch from 8535fd7 to 800e2e3 Compare October 2, 2018 22:37

Stebalien requested a review from kevina October 2, 2018 22:42

magik6k force-pushed the feat/coreapi/unixfs branch from 800e2e3 to a8473f3 Compare October 2, 2018 22:42

kevina reviewed Oct 2, 2018

View reviewed changes

Stebalien added RFM status/blocked Unable to be worked further until needs are met labels Oct 2, 2018

magik6k removed RFM status/blocked Unable to be worked further until needs are met labels Oct 3, 2018

magik6k changed the title ~~More options for coreapi.unixfs.Add~~ Refactor UnixFS CoreAPI Oct 3, 2018

coreapi unixfs: multi file support in unixfs coreapi

e6bc923

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

magik6k force-pushed the feat/coreapi/unixfs branch from 6c68539 to e6bc923 Compare October 3, 2018 13:15

coreapi unixfs: unixfs.Get

8dda695

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

magik6k force-pushed the feat/coreapi/unixfs branch from ff051d7 to 8dda695 Compare October 3, 2018 15:24

magik6k added 7 commits October 3, 2018 22:06

coreapi unixfs: test recursive adding

c6f8e7e

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

coreapi unixfs: wrap option

18fec6e

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

coreapi unixfs: hidden opiton

3e754d6

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

coreapi unixfs: stdin-name option

f9cf849

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

coreapi unixfs: progress events

a81ec29

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

coreapi unixfs: filestore opts

d51ce96

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

ipfs add uses CoreAPI

69eb015

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

kevina self-requested a review October 4, 2018 00:02

kevina reviewed Oct 4, 2018

View reviewed changes

kevina self-requested a review October 4, 2018 01:48

magik6k added 2 commits October 4, 2018 04:07

coreapi unixfile: simplify RawNode case

051c330

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

coreapi unixfs: fix inline doc

4b1f733

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

kevina reviewed Oct 4, 2018

View reviewed changes

Stebalien mentioned this pull request Oct 5, 2018

add offline id test #4978 and refactor command code #5562

Merged

update unixfs inline option comment to give us room to change things

35e8d6f

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

ghost assigned Stebalien Oct 5, 2018

Merge remote-tracking branch 'origin/master' into feat/coreapi/unixfs

676403b

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

Stebalien approved these changes Oct 5, 2018

View reviewed changes

Stebalien merged commit 5ba0976 into master Oct 5, 2018

ghost removed the status/in-progress In progress label Oct 5, 2018

Stebalien deleted the feat/coreapi/unixfs branch October 5, 2018 18:18

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

Merge pull request ipfs/kubo#5501 from ipfs/feat/coreapi/unixfs

75f391b

Refactor UnixFS CoreAPI This commit was moved from ipfs/kubo@5ba0976

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor UnixFS CoreAPI #5501

Refactor UnixFS CoreAPI #5501

magik6k commented Sep 20, 2018 •

edited

Loading

kevina commented Sep 28, 2018

Stebalien commented Sep 28, 2018

kevina commented Sep 28, 2018

kevina Sep 29, 2018

kevina left a comment

kevina Sep 29, 2018

kevina commented Sep 29, 2018

kevina Oct 2, 2018

magik6k Oct 2, 2018

kevina Oct 2, 2018

kevina Oct 2, 2018

magik6k Oct 2, 2018

Stebalien Oct 2, 2018

magik6k Oct 2, 2018

kevina Oct 2, 2018

kevina Oct 2, 2018

kevina Oct 2, 2018

kevina commented Oct 2, 2018 •

edited

Loading

Stebalien commented Oct 2, 2018

magik6k commented Oct 2, 2018

kevina commented Oct 2, 2018

Stebalien commented Oct 2, 2018

kevina left a comment

kevina Oct 4, 2018

Stebalien Oct 4, 2018

kevina Oct 5, 2018 •

edited

Loading

Stebalien commented Oct 5, 2018

Refactor UnixFS CoreAPI #5501

Refactor UnixFS CoreAPI #5501

Conversation

magik6k commented Sep 20, 2018 • edited Loading

kevina commented Sep 28, 2018

Stebalien commented Sep 28, 2018

kevina commented Sep 28, 2018

Choose a reason for hiding this comment

kevina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina commented Sep 29, 2018

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 commented Oct 2, 2018 • edited Loading

Stebalien commented Oct 2, 2018

magik6k commented Oct 2, 2018

kevina commented Oct 2, 2018

Stebalien commented Oct 2, 2018

kevina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Stebalien commented Oct 5, 2018

magik6k commented Sep 20, 2018 •

edited

Loading

kevina commented Oct 2, 2018 •

edited

Loading

kevina Oct 5, 2018 •

edited

Loading