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

Change the default to not follow symlinks? #12

Closed
harendra-kumar opened this Issue Jul 6, 2016 · 15 comments

Comments

Projects
None yet
2 participants
@harendra-kumar
Contributor

harendra-kumar commented Jul 6, 2016

The listing and copying functions in this package follow symlinks by default. The usual unix utils (ls, find, cp) convention is to not follow symlinks by default and use an explicit option (usually -L) to follow symlinks. It will be nice if we are consistent with this convention, though it will require API behavior change for already published APIs.

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented Jul 6, 2016

The functions have directory semantics and I would like to avoid changing them because there are libraries that depend on existing API (and semantics) now.

@harendra-kumar

This comment has been minimized.

Contributor

harendra-kumar commented Jul 6, 2016

Its perhaps not a good default because it does not preserve symlinks while copying or listing. Copying will turn symlinks into duplicated directories or files rather than copying them as symlinks. Similarly when listing we cannot distinguish whether what we are seeing is a symlink or real dir/file.

Yes, there may be pain in fixing if something depends on those semantics. But, it may be easier now that the package is not too old. It may be harder to change later and will remain so forever. Not sure though if that's a real problem. I guess first time users might assume it to be otherwise and face surprises. If we don't fix we should at least emphasize well in the documentation that these APIs FOLLOW symlinks.

@mrkkrp mrkkrp added the question label Sep 10, 2016

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented Feb 23, 2017

Closing this. Docs we provide are the same as directory has and it should be assumed that semantics of the functions are the same as in directory. I propose you open an issue against directory and when they update their docs, ping me and I'll pick up these changes.

@mrkkrp mrkkrp closed this Feb 23, 2017

@harendra-kumar

This comment has been minimized.

Contributor

harendra-kumar commented Feb 23, 2017

If we don't fix we should at least emphasize well in the documentation that these APIs FOLLOW symlinks.

This will help the users to at least know the current semantics.

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented Feb 23, 2017

Not against that, but it should be stated in directorys docs first (more people use direcotory with vanilla FilePaths anyway).

@harendra-kumar

This comment has been minimized.

Contributor

harendra-kumar commented Mar 27, 2017

I tried to look at all the directory package APIs with the intention of raising an issue there related to this behavior. But I could not find a single one which goes through directories recursively and follows symlinks. The only recursive functions are for removal and they have explicit documentation that they do not follow symlinks.

It seems path-io has this inconsistent behavior only in the recursive listing, walking and copying APIs, these APIs are exclusive to path-io.

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented Mar 27, 2017

Would you like to open a PR that would clarify this in the path-io documentation?

@harendra-kumar

This comment has been minimized.

Contributor

harendra-kumar commented Mar 27, 2017

I would favor changing the default. Though we have taken care of the looping problem via walkDir, we still have the problem of duplication in copying APIs and inconsistency with the rm APIs. No-follow-symlink is a saner default and is followed by all unix/gnu utils. But then the question is do you want to provide APIs with a follow option, how?

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented Apr 2, 2017

inconsistency with the rm APIs

Consistency with Unix commands is not the aim of the package. Consistency with directory is. We will mirror functions from directory including their semantics. If you want different semantics, raise an issue on directory's issue tracker, then when directory gets functions with the desired behavior, we can wrap them in path-io.

Better for wider Haskell community too (people who use directory but not path-io).

@harendra-kumar

This comment has been minimized.

Contributor

harendra-kumar commented Apr 2, 2017

There seems to be a communication gap here.

Consistency with Unix commands is not the aim of the package.

I meant "inconsistency with the rm APIs" of this package itself, not with the unix commands. For example removeDirRecur documentation says "symbolic links are removed without affecting their targets". But copyDirRecur follows and copies the targets of the symbolic links.

If you want different semantics, raise an issue on directory's issue tracker, then when directory
gets functions with the desired behavior, we can wrap them in path-io.

I was talking about only those functions that do not exist in directory package but exist exclusively in path-io for example copyDirRecur. Which means the problem exists exclusively in path-io and there is absolutely nothing to be done in the directory package.

I am not at all advocating consistency with unix commands, however, I do argue that the choice of default taken by unix commands/gnu utils is a good one, and is time tested. Therefore, if you are free to choose a default without other constraints it is a good one. As a trivial example consider a copyDirRecur on a directory that has a symlink to /, it will end copying the whole file system, which is not desirable by default (IMO).

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented Apr 2, 2017

OK, that clarificates the situation. Perhaps we could switch to the new default and release a new major version.

@mrkkrp mrkkrp reopened this May 23, 2017

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented May 23, 2017

I'm re-opening this. Feel free to open a PR changing the default behavior to not following symlinks.

I'm in the process of performing some maintenance for all my active OSS projects. For path-io it means that there will be a new release soon. If you are busy I could look into this issues myself, although it would be much appreciated if you could open a PR for this change.

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented May 23, 2017

I just did a git blame and it looks like the directory walking functions were added by you. Is that correct?

@harendra-kumar

This comment has been minimized.

Contributor

harendra-kumar commented May 23, 2017

Yes, that's correct. They were made consistent with the existing convention in this library and I raised this issue later to discuss and correct the default convention. I am a bit busy but I can try to squeeze in some time for one PR either this one or #19.

@mrkkrp

This comment has been minimized.

Owner

mrkkrp commented May 23, 2017

As I understand #19 adds some new functionality, so I'm in favor of "fixing" what we have already instead. New functionality can be added in a future (minor) version at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment