implement symlinks in unixfs, first draft #1627
Conversation
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
| @@ -84,10 +84,25 @@ func (f *serialFile) NextFile() (File, error) { | |||
| // open the next file | |||
| fileName := fp.Join(f.name, stat.Name()) | |||
| filePath := fp.Join(f.path, stat.Name()) | |||
| st, err := os.Lstat(filePath) | |||
There was a problem hiding this comment.
Must this (and ModeSymlink check, and if they are handled identically) happen before every os.Open() across the code?
There was a problem hiding this comment.
I think that we only care to handle symlinks like that here, where the primary use case is ipfs add -r.
There was a problem hiding this comment.
There was a problem hiding this comment.
In that case, it wouldnt make sense not to follow the symlink. What are you going to do with a single symlink inside ipfs? A symlink out of context is fairly useless, and potentially dangerous.
There was a problem hiding this comment.
The option should not be a hard-coded "only create symlink if '-r'", but rather another flag to dereference the symlinks, regardless it is a single symlink or '-r' symlink.
If it is potentially dangerous to create a single symlink, then there should be a warning message instead, not hard-coded.
There was a problem hiding this comment.
(regardless of the decision, the default is no dereference, so this PR can be landed without the flag)
There was a problem hiding this comment.
If this PR is back to the default today (i.e. not breaking expectations) i'm good to land this.
There was a problem hiding this comment.
@jbenet alright, so 'ipfs add thisIsASymlink' should do what?
There was a problem hiding this comment.
(from a discussion on irc, symlinks should be added by default. i.e. "in git we trust")
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
whyrusleeping
force-pushed
the
feat/symlinks
branch
from
173d84b
to
9bbd9b0
Compare
| mfr.currentFile = s | ||
|
|
||
| // TODO(why): this is a hack. pick a real contentType | ||
| contentType = "symlink" |
There was a problem hiding this comment.
maybe application/symlink ? not sure. is there a symlink mime type?
There was a problem hiding this comment.
i'd be fine with application/symlink. Googling around doesnt really give anything official for symlinks.
There was a problem hiding this comment.
(decoupled from this PR) Should the 'ipfs add' api be simplified where files are always archived (and optionally gzipped)?
This is already what ipfs get does.
Will this simplify api writing (e.g. then multipart handling will become archive.Extract() in each lang)?
There was a problem hiding this comment.
There was a problem hiding this comment.
+1 for symlink as a regular file with ModeSymlink.
Though I think it is possible to write a more lightweight version of that PR (and part of this PR) without the fileinfo.go
+1 vote for sending an archive to the api pipe (tar for now, later dag).
This will decouple the current api writing/porting into api rest parsing & porting the dag archive extractor library (tar for now).
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
whyrusleeping
force-pushed
the
feat/symlinks
branch
from
ee0b99f
to
56f01e4
Compare
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
|
@whyrusleeping ok LGTM. Looks like all commits pass. 👏 👏 👏 👏 👏 long time coming |
implement symlinks in unixfs, first draft
|
BuildDagFromFile and coreunix.AddR still doesn't make symlink by default. |
|
Fixed in #1629 |
|
|
|
@rht 😄 |
Ah, yeah. Should probably fix those too. |
|
Can be closed: #979 |
|
Can be closed: #616 |
|
(looks like an issue's urgency is measured by the amount of duplicates it has) |
@rht amen to that. |
Implemented basic symlink support. Most commands dont follow them yet, need to figure out semantics for things like
ipfs cat QmSymlink. But they are supported in the fuse mounts.License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com