fix mfs cp/mv bug

[hackergrrl]

Addresses #2654

[whyrusleeping]

lets definitely not panic here. there are other types (symlinks, raw blocks) that might pop up

[hackergrrl]

Agreed.

[Kubuxu]

This broke many sharness tests: http://ci.ipfs.team:8111/viewLog.html?buildId=1732&buildTypeId=GoIpfs_CiTests&tab=buildLog#_focus=11331

[hackergrrl]

Green!

[hackergrrl]

@whyrusleeping anything else needed here?

[whyrusleeping]

i'm pretty sure we don't want to treat these all the same

[hackergrrl]

I believe this is consistent with what the old code did -- what should happen instead?

[whyrusleeping]

no its not, take a look at the code in childNode

[hackergrrl]

Awesome -- cacheNode is exactly what I wanted here.

[whyrusleeping]

symlink is not correct here. We don't currently handle them.

[hackergrrl]

What the old code in AddChild did was check if len(links) == 0, and if so, cached it as a file. it didn't check type (symlink, file, etc) at all like cacheNode does.

This is because ipfs add uses the mfs package to add files, which means in order to handle symlinks in directories (like we do today on ipfs add), AddChild needs to also support symlinks.

[whyrusleeping]

symlink is still not correct here.

[whyrusleeping]

If we pass a dag node that represents a symlink to 'new file' here, its not going to work properly if we try to use it (NewDagModifier will produce undefined behaviour when trying to modify a symlink).

[whyrusleeping]

The reason this currently works (since we use mfs for add) is that we never instantiate a NewFile as a symlink (or when we do, in the current incorrect code, it never gets used).

[whyrusleeping]

the caching here is likely the cause of #2615

[hackergrrl]

Looks like it was introduced in #2531. I think we had best remove the caching here and figure out how to fix #2531 separately; it's much less important than this being broken (and #2615).

[whyrusleeping]

@noffle could I get an update here? This is causing issues for people.

[hackergrrl]

Hey @whyrusleeping. The fix is simple enough: remove the caching piece introduced in #2531. However, this will also cause folders to appear out of order / not at all when using --only-hash. Fixing that piece is going to be more complicated. Either we can re-introduce the smaller regression (--only-hash output) now, or I can try and find some time this sprint to take a look.

[whyrusleeping]

@noffle lets go ahead and temporarily disable the only hash flag when combined with the recursive flag, we can print a notice to the user referencing a tracking bug.

[hackergrrl]

@whyrusleeping good idea -- added

[hackergrrl]

Looks like this approach won't work either. Another approach -- here are why's notes:

10:59:42 whyrusleeping | the easy solution i see, is adding a way to set your own mfs root object on the fileadder
11:00:02 whyrusleeping | creating that with an in memory dagservice (so all of its writes are kept)
11:00:21 whyrusleeping | and giving the FileAdder itself the 'null' dagservice that throws away all the writes

I'll try and get to this next week.

[whyrusleeping]

this has been fixed in #2795

[hackergrrl]

Thank you for crushing this bug into a fine paste.

On 06/08 22:09, Jeromy Johnson wrote:

this has been fixed in #2795

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#2690 (comment)