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

Make ipfs add -r follow symlinks #620

Closed
wants to merge 2 commits into from

Conversation

mildred
Copy link
Contributor

@mildred mildred commented Jan 22, 2015

Fix issue described in #616, symlinks are now followed by default.

@mappum
Copy link
Contributor

mappum commented Jan 22, 2015

If we're following symlinks, we might need cycle detection (what if you linked to the parent directory?). Also, might be better to only follow symlinks when a CLI flag is used (ipfs add -r -l).

@whyrusleeping
Copy link
Member

Yeah, agreed with @mappum.

@mildred
Copy link
Contributor Author

mildred commented Jan 23, 2015

Ok, so if we want to default to not follow symlinks, we'll have first to implement symlinks in unixfs.

As for cycle detection, I'm sure this could be a good idea, but I don't think any other program out there does it. You shouldn't have cycles on your filesystem, period. Following symlinks is a natural behaviour, and you generally have to be explicit if you suspect there is a symlink and you don't want to follow it. Go is a kind of exception here with its os.Readdir function that does a lstat() instead of the more conventional stat() function.

So I think adding cycle detection would slow things down a lot and add too much complexity to the code to be worth implementing.

@whyrusleeping
Copy link
Member

@mildred most unix programs do actually do a sort of 'cycle detection' in counting how many symlinks theyve seen in the current resolution and failing if that number goes over a certain threshold. It costs pretty much nothing as its done in-line during the resolve.

Implementing symlinks in unixfs is feasible, and should just be as simple as storing a path in the dag node.

@jbenet
Copy link
Member

jbenet commented Jan 23, 2015

@mildred thanks for the PR! agree on both:

  • follow symlinks with a flag (default operation should just add the symlink, like git does)
  • cycle detection (either the symlink count, or could be done with a map of absolute paths)

I'll let @whyrusleeping shepherd this one, as he's the unixfs pro

@jbenet jbenet mentioned this pull request Jan 23, 2015
@jbenet
Copy link
Member

jbenet commented Jan 31, 2015

ping @whyrusleeping

@jbenet jbenet modified the milestone: α Feb 2, 2015
@whyrusleeping
Copy link
Member

If we can add a sharness test or two around this, that would be awesome. Otherwise, LGTM

@whyrusleeping whyrusleeping modified the milestones: Post Alpha, α Feb 4, 2015
@whyrusleeping whyrusleeping added the topic/files Topic files label Mar 28, 2015
@whyrusleeping whyrusleeping removed this from the Post Alpha milestone Mar 29, 2015
@whyrusleeping
Copy link
Member

closing due to inactivity, please reopen as necessary

note: all pull requests older than three weeks may be closed in an effort to keep our open pull requests more focused.

ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
clean up a channel that was dangling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/files Topic files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants