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.

Sign up for GitHub

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

Jump to bottom

Create a raw node instead of a file node when there is no content. #4693

Merged
merged 2 commits into from
Jun 8, 2018
Merged

Create a raw node instead of a file node when there is no content. #4693

merged 2 commits into from
Jun 8, 2018

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Feb 13, 2018 •
edited
Loading

This fixes things so when raw-leaves are enabled a zero size file creates a zero size raw leaf. When raw-leaves are not enabled the hash created changes from QmbFMke1KXqnYyBBWxB74N4c5SBnJMVAiMNRcGu6x1AwQH to Qmdsf68UUYTSSx3i4GtDJfxzpAEZt7Mp23m3qa36LYMSiW, since the type field
changed from TFile to TRaw.

Closes #4688

@ghost ghost assigned kevina Feb 13, 2018
@ghost ghost added the status/in-progress In progress label Feb 13, 2018
hsanjuan
hsanjuan reviewed Feb 13, 2018
View reviewed changes
importer/helpers/dagbuilder.go Outdated
@@ -122,6 +122,34 @@ func (db *DagBuilderHelper) NewUnixfsNode() *UnixfsNode {
return n
}

// NewUnixfsLeaf creates a lead node filled with date
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to cause a golint warning. (and date should be data)

@kevina
Copy link
Contributor Author

kevina commented Feb 14, 2018

Jenkins is complianing about this:

Following Go code is not formatted.
-----------------------------------
./fuse/ipns/ipns_test.go
-----------------------------------
Run 'go fmt ./...' in your source directory
mk/golang.mk:48: recipe for target 'test_go_fmt' failed

Except (1) I did not touch that file (2) that file is already go formatted in that commit as gofmt -d ./fuse/ipns/ipns_test.go returns nothing and (3) when I ran bin/test-go-fmt it doesn't report anything. The script itself is very simple so I am not sure what the problem is.

@magik6k
Copy link
Member

magik6k commented Feb 14, 2018

This is caused by 682ca11, which got merged to master in #4695

@magik6k magik6k mentioned this pull request Feb 14, 2018
Closed
@hsanjuan
Copy link
Contributor

hmm, my bad, I will PR that. it seems it's the order of the imports.

@hsanjuan
Copy link
Contributor

#4702

@kevina kevina mentioned this pull request Feb 15, 2018
Closed
@Kubuxu
Copy link
Member

Kubuxu commented Mar 9, 2018

@whyrusleeping are you ok with changing hash of empty file?

@Stebalien
Copy link
Member

@whyrusleeping are you ok with changing hash of empty file?

Doesn't this only apply when raw leaves (experimental) are enabled?

@magik6k
Copy link
Member

magik6k commented Mar 9, 2018

Just tried, for me it changes to zb2rhmy65F3REf8SZp7De11gxtECBGgUKaLdiDj7MCGCHxbDW with raw-leaves and to Qmdsf68UUYTSSx3i4GtDJfxzpAEZt7Mp23m3qa36LYMSiW without (it's QmbFMke1KXqnYyBBWxB74N4c5SBnJMVAiMNRcGu6x1AwQH in both cases on master now)

Kubuxu
Kubuxu requested changes Mar 15, 2018
View reviewed changes
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

Blocking for now.
I would like to get explicit "good to go" from everyone.

@Kubuxu Kubuxu added need/review Needs a review status/blocked Unable to be worked further until needs are met and removed status/in-progress In progress labels Mar 15, 2018
@kevina
Create a raw node instead of a file node when there is no content.
485a5e5 
This fixes things so when raw-leaves are enabled a zero size file creates
a zero size raw leaf.  When raw-leaves are not enabled the hash created
changes from QmbFMke1KXqnYyBBWxB74N4c5SBnJMVAiMNRcGu6x1AwQH to
Qmdsf68UUYTSSx3i4GtDJfxzpAEZt7Mp23m3qa36LYMSiW, since the type field
changed from TFile to TRaw.

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@kevina
Avoid changing hash for empty non-raw leaves.
6ab1e71 
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@ghost ghost added the status/in-progress In progress label May 31, 2018
@kevina
Copy link
Contributor Author

kevina commented May 31, 2018

@Kubuxu I added a special case to avoid changing the hash, I am removing the blocked label.

@kevina kevina removed the status/blocked Unable to be worked further until needs are met label May 31, 2018
@kevina
Copy link
Contributor Author

kevina commented Jun 4, 2018 •
edited
Loading

@whyrusleeping @Kubuxu @Stebalien I think this should be good to go now. I believe I addressed the concern that was causing @Kubuxu to block it.

@schomatis
Copy link
Contributor

Should the trickle builder also be taken into consideration here?

@kevina
Copy link
Contributor Author

kevina commented Jun 4, 2018

Should the trickle builder also be taken into consideration here?

I don't think so. The trickle dag format never returns a leaf node's hash, so it will make sense that an empty node is simply a node with no children, rather than an empty leaf node.

whyrusleeping
whyrusleeping approved these changes Jun 8, 2018
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This LGTM. Thanks for the test making sure it stays the same

@whyrusleeping whyrusleeping merged commit 65b8d70 into master Jun 8, 2018
@ghost ghost removed the status/in-progress In progress label Jun 8, 2018
@whyrusleeping whyrusleeping deleted the kevina/fix-zero-raw-leaf branch June 8, 2018 08:41
@kevina kevina mentioned this pull request Jun 18, 2018
Closed
@momack2 momack2 added this to Needs Review in ipfs/go-ipfs May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu requested changes

@whyrusleeping whyrusleeping whyrusleeping approved these changes

@Stebalien Stebalien Awaiting requested review from Stebalien

@hsanjuan hsanjuan Awaiting requested review from hsanjuan

Assignees

@kevina kevina

Labels
need/review Needs a review
Projects
No open projects
ipfs/go-ipfs
Needs Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ipfs add --raw-leaves return protobuf instead or raw on zero length file
7 participants
@kevina @magik6k @hsanjuan @Kubuxu @Stebalien @schomatis @whyrusleeping