better-files icon indicating copy to clipboard operation
better-files copied to clipboard

linkTo seams to be broken

Open scalway opened this issue 6 years ago • 7 comments

Current implementation of symbolicLinkTo and linkTo seams to be incorect.

//both functions do almost same thing (one is symbolic of course but... yea)
//this one'll return f
link.symbolicLinkTo(f) 
// and this one returns link
f.linkTo(link) 
//but both make link to shows on file f.

Current code:

def symbolicLinkTo(destination: File)(implicit attributes: File.Attributes = File.Attributes.default): destination.type = {
    Files.createSymbolicLink(path, destination.path, attributes: _*)  // <-- here
    destination
  }

  def linkTo(destination: File, symbolic: Boolean = false)(implicit attributes: File.Attributes = File.Attributes.default): destination.type = {
    if (symbolic) {
      symbolicLinkTo(destination)(attributes)
    } else {
      Files.createLink(destination.path, path) // <-- here
      destination
    }
  }

Java API looks like this.

//java.nio.file.Files
//--------------------------
public static Path createSymbolicLink(Path link, Path target, FileAttribute<?>... attrs)
public static Path createLink(Path link, Path existing)

In java path link is always first argument!, and in current implementation it is inconsistant.

scalway avatar Mar 01 '18 13:03 scalway

Isn't this obvious from the types? The return types of both method return the destination.type ...

pathikrit avatar Mar 01 '18 15:03 pathikrit

No... I was not precise enough... sorry.

Problem is when java API is called. symbolicLinkTo and linkTo creates links in totally different manner. symbolicLinkTo assumes to be called on link object and target f needs to be passed as argument linkTo needs to be called on target f file and takes link file as argument!

it means destination is different in both and it'll satisfy compiler contract in both cases.

link.symbolicLinkTo(f) 
// will call Files.createSymbolicLink(link, f)
// and return f

link.linkTo(f) //expected same api like in symbolicLinkTo
/** 
  will call Files.createLink(f, link)... ops
  and throw Error
  java.nio.file.NoSuchFileException: /tmp/491470459606103590ar-152-Swieta_Bozonarodzeniowe.ppt -> 
  /tmp/1058432223189661722
  sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
  sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
  sun.nio.fs.UnixFileSystemProvider.createLink(UnixFileSystemProvider.java:476)
  java.nio.file.Files.createLink(Files.java:1086)
  better.files.File.linkTo(File.scala:674 
*/

f.linkTo(link)  
// will call  Files.createLink(link, f)
//and return link

f.symbolicLinkTo(link)
/** 
java.nio.file.FileAlreadyExistsException: /tmp/491470459606103590ar-152-Swieta_Bozonarodzeniowe.ppt
  sun.nio.fs.UnixException.translateToIOException(UnixException.java:88)
  sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
  sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
  sun.nio.fs.UnixFileSystemProvider.createSymbolicLink(UnixFileSystemProvider.java:457)
  java.nio.file.Files.createSymbolicLink(Files.java:1043)
  better.files.File.symbolicLinkTo(File.scala:668)

*/

To be honest it seams to be messed! Problematic part is that someone could depends on this behavior already :(. linkTo is not so often used and i thing it should be changed

scalway avatar Mar 01 '18 16:03 scalway

I intentionally named these methods with to suffixes to make this clear e.g. I called it symbolicLinkTo instead of symbolicLink and linkTo instead of link. I think the problem is bad design on the Java API which I should have wrapped in a consistent manner in better-files but no way to fix now without breaking existing code.

pathikrit avatar Mar 01 '18 16:03 pathikrit

java api is consistent here: link path is always first param. I propose to deprecate linkTo with appriopriate comment, and create def hardLinkTo().

Dsl.ln is stil problematic part (currently it is defined in terms of linkTo).

scalway avatar Mar 01 '18 18:03 scalway

I still don't understand - can you send a quick PR of what you want?

pathikrit avatar Jun 06 '18 23:06 pathikrit

I got hit by this too. It seems like the intended behavior is to have File(newLinkPath).linkTo(existingFile) in both cases, but its actually swapped depending on the symbolic link flag. Right now its this:

File(existingFilePath).linkTo(newLinkFile, false) File(newLInkPath).linkTo(existingFile, true)

It looks like an unintended argument swap on the Java method call more than anything, as it does seem pretty unexpected to have the source/dest change based on an optional flag. If you agree, let me know the intended order and I'll create a PR to fix it. If this is the intended behavior, I can add something to your docs or wiki that explains the working of the feature.

gblahaerath avatar Feb 03 '19 21:02 gblahaerath

@gblahaerath : Yes intention is to have consistent File(source).linkTo(destination) in both cases. Can you send a PR please?

pathikrit avatar Feb 03 '19 22:02 pathikrit