Fix os.copy and os.move directories to root

[kiendang]

Fix an error where os.copy(createFolders = true, mergeFolders = true) a directory to root throw a PathError.AbsolutePathOutsideRoot due to calling /up on root.

The same issue (calling /up on root) is seen in os.move, os.copy.matching and os.move.matching so this fixes those as well. In case of os.move and os.move.matching moving a directory to root should throw an error anyway but at least now it throws the correct underlying error instead of AbsolutePathOutsideRoot.

Fix #167

Pull request: #267

[lefou]

Looks reasonable. Thank you!

You can ignore the failing test runner for macosx. It's independent of this issue and should be fixed once I merge #266.

[lefou]

You could also use an intercept here, to declare the concrete expected exception.

https://github.com/com-lihaoyi/utest?tab=readme-ov-file#intercept

[lihaoyi]

Looks good to me. @kiendang if you could update the test to use intercept I can merge this and close the bounty

[lefou]

LGTM. We should cut a new release once this is merged.

[kiendang]

@lefou @lihaoyi I did use intercept earlier but there were a few issues with using intercept in the os.move.matching case. It throws a java.lang.IllegalArgumentException (rather than a FileAlreadyExistsException) which AbsolutePathOutsideRoot extends, so the test wouldn't fail without the fix. Plus there seems to be compilation error with using os.move.matching pattern matching syntax inside a intercept block. And also the purpose is to test that the os.move and os.move.matching cases don't throw AbsolutePathOutsideRoot rather that they throws a specific exception so went with the current approach instead.

[kiendang]

More on the error with using pattern matching syntax inside a intercept block, this would result in an error

intercept[...] {
    os.list(os.root("/", fileSystem)).collect(os.move.matching { case p / "test" => p })
}

while this wouldn't

intercept[...] {
    List(os.root("/", fileSystem)).collect(os.move.matching { case p => p })
}

[lefou]

More on the error with using pattern matching syntax inside a intercept block

Which error do you mean specifically? The FileAlreadyExistsException?

This test works for me with Scala 2.13:

      test("moveMatchingToRootDirectoryShouldFail") {
        withCustomFs { fileSystem =>
          intercept[FileAlreadyExistsException] {
            os.list(os.root("/", fileSystem)).collect(os.move.matching { case p / "test" => p })
          }
        }
      }

But with Scala 2.12.17, I see the same compile error as the CI:

1 targets failed
os.jvm[2.12.17].test.compile scala.reflect.internal.FatalError: 
  unexpected UnApply os.`package`./.unapply({
  val $temp$macro$62 = <unapply-selector>;
  $log$macro$59(utest.TestValue("<unapply-selector>", "os.Path", $temp$macro$62));
  $temp$macro$62
}) <unapply> ((p @ _), "test")
     while compiling: /home/lefou/work/opensource/os-lib/os/test/src-jvm/PathTestsCustomFilesystem.scala
        during phase: globalPhase=typer, enteringPhase=namer
     library version: version 2.12.17
    compiler version: version 2.12.17
  reconstructed args: -bootclasspath /home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.17/scala-library-2.12.17.jar -classpath /home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/utest_2.12/0.8.3/utest_2.12-0.8.3.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/sourcecode_2.12/0.4.0/sourcecode_2.12-0.4.0.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/geny_2.12/1.1.0/geny_2.12-1.1.0.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-sbt/test-interface/1.0/test-interface-1.0.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/portable-scala/portable-scala-reflect_2.12/1.1.2/portable-scala-reflect_2.12-1.1.2.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-reflect/2.12.17/scala-reflect-2.12.17.jar:/home/lefou/work/opensource/os-lib/os/compile-resources:/home/lefou/work/opensource/os-lib/out/os/jvm/2.12.17/compile.dest/classes:/home/lefou/work/opensource/os-lib/os/test/compile-resources:/home/lefou/work/opensource/os-lib/out/os/jvm/2.12.17/test/compile.dest/classes -Xplugin:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/acyclic_2.12.17/0.3.12/acyclic_2.12.17-0.3.12.jar -P:acyclic:force

  last tree to typer: UnApply
       tree position: line 213 of /home/lefou/work/opensource/os-lib/os/test/src-jvm/PathTestsCustomFilesystem.scala
              symbol: null
           call site: method applyOrElse in package os

== Source file context for tree position ==

   210         withCustomFs { fileSystem =>
   211           // This should fail. Just test that it doesn't throw PathError.AbsolutePathOutsideRoot.
   212           intercept[FileAlreadyExistsException] {
   213             os.list(os.root("/", fileSystem)).collect(os.move.matching { case p / "test" => p })
   214           }
   215         }
   216       }
    scala.reflect.internal.Reporting.abort(Reporting.scala:69)
    scala.reflect.internal.Reporting.abort$(Reporting.scala:65)
    scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:28)
    scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5745)
    scala.tools.nsc.typechecker.Typers$Typer.typed(Typers.scala:5785)

[kiendang]

I tested on Scala 3.3.1 and jdk 17. For me it also doesn't throw FileAlreadyExistsException but IllegalArgumentException / is inside /.

[kiendang]

Also yes I got a similar compilation error.

[lihaoyi]

Looks good to me. Thanks @kiendang! Can send me an email at haoyi.sg@gmail.com with your bank details for the bounty