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

add: hash-only: set the prefix for MFS root #4755

Merged
merged 1 commit into from
Mar 23, 2018
Merged

add: hash-only: set the prefix for MFS root #4755

merged 1 commit into from
Mar 23, 2018

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Mar 2, 2018

In the case that the only-hash option (-n) is set for ipfs add, use the same prefix from the fileAdder for the MFS root so it is processed using the same CID version and hash function.

Fixes #4652.

This is a work in progress, some tests should be added. Also, the comment is not very illustrative, it should clarify why is it that the prefix needs two be specified twice (for the file adder and the MFS root).

@djdv
Copy link
Contributor

djdv commented Mar 2, 2018

This works on my node. 👍

A simple test case you can add for this one, is to capture the hashes generated by any add parameter set and compare them with the same set + -n, if they do not match the test should fail, otherwise it should pass.

e.g.
The hashes produced by add -w --cid-version=1 "testdata" must equal the output hashes produced by add -n -w --cid-version=1 "testdata".
The hashes produced by add -w --hash=Blake2b-256 "testdata" must equal the output hashes produced by add -n -w --hash=Blake2b-256 "testdata".

Fixes #4652.

License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
@schomatis
Copy link
Contributor Author

@kevina I based this test on your ipfs add -w -r (dir) (adding the -n option), as the issue was occurring for CIDv1, WDYT?

@schomatis
Copy link
Contributor Author

@Stebalien In this PR the prefix used for the fileAdder is also applied to the MFS root, to ensure consistency across the file hashes. Is this correct?

The MFS root is the only node that isn't added by the fileAdder (because it's already there before the addition of the user files), does that make sense? I want to add an explanation (in the form of a comment) as to why the prefix has to be referenced twice.

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.

Tanks for including tests.

@Kubuxu Kubuxu changed the title [WIP] add: hash-only: set the prefix for MFS root add: hash-only: set the prefix for MFS root Mar 9, 2018
@Kubuxu Kubuxu added the RFM label Mar 9, 2018
@Kubuxu Kubuxu added this to the go-ipfs 0.4.15 milestone Mar 9, 2018
@whyrusleeping whyrusleeping merged commit 59ad0f8 into ipfs:master Mar 23, 2018
@whyrusleeping
Copy link
Member

Another one, thanks @schomatis!

@schomatis schomatis deleted the fix/add/hash-only branch March 23, 2018 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[add] Only-hash gives wrong result with wrap-with-directory
4 participants