fix nim-lang#8225 os.isHidden was buggy on posix (nim-lang#8315)

* fix nim-lang#8225 isHidden was broken on posix * scope rest of tos.nim under blocks to avoid variable scope bugs
krux02 · Oct 15, 2018 · 189971a · 189971a
1 parent 5c1c371
commit 189971a
Show file tree

Hide file tree

Showing 2 changed files with 130 additions and 117 deletions.
diff --git a/lib/pure/os.nim b/lib/pure/os.nim
@@ -305,15 +305,6 @@ proc absolutePath*(path: string, root = getCurrentDir()): string =
       raise newException(ValueError, "The specified root is not absolute: " & root)
     joinPath(root, path)
 
-when isMainModule:
-  doAssertRaises(ValueError): discard absolutePath("a", "b")
-  doAssert absolutePath("a") == getCurrentDir() / "a"
-  doAssert absolutePath("a", "/b") == "/b" / "a"
-  when defined(Posix):
-    doAssert absolutePath("a", "/b/") == "/b" / "a"
-    doAssert absolutePath("a", "/b/c") == "/b/c" / "a"
-    doAssert absolutePath("/a", "b/") == "/a"
-
 proc expandFilename*(filename: string): string {.rtl, extern: "nos$1",
   tags: [ReadDirEffect].} =
   ## Returns the full (`absolute`:idx:) path of an existing file `filename`,
@@ -1724,13 +1715,22 @@ proc getFileInfo*(path: string, followSymlink = true): FileInfo =
     rawToFormalFileInfo(rawInfo, path, result)
 
 proc isHidden*(path: string): bool =
-  ## Determines whether a given path is hidden or not. Returns false if the
-  ## file doesn't exist. The given path must be accessible from the current
-  ## working directory of the program.
+  ## Determines whether ``path`` is hidden or not, using this
+  ## reference https://en.wikipedia.org/wiki/Hidden_file_and_hidden_directory
+  ##
+  ## On Windows: returns true if it exists and its "hidden" attribute is set.
   ##
-  ## On Windows, a file is hidden if the file's 'hidden' attribute is set.
-  ## On Unix-like systems, a file is hidden if it starts with a '.' (period)
-  ## and is not *just* '.' or '..' ' ."
+  ## On posix: returns true if ``lastPathPart(path)`` starts with ``.`` and is
+  ## not ``.`` or ``..``. Note: paths are not normalized to determine `isHidden`.
+  runnableExamples:
+    when defined(posix):
+      doAssert ".foo".isHidden
+      doAssert: not ".foo/bar".isHidden
+      doAssert: not ".".isHidden
+      doAssert: not "..".isHidden
+      doAssert: not "".isHidden
+      doAssert ".foo/".isHidden
+
   when defined(Windows):
     when useWinUnicode:
       wrapUnary(attributes, getFileAttributesW, path)
@@ -1739,14 +1739,8 @@ proc isHidden*(path: string): bool =
     if attributes != -1'i32:
       result = (attributes and FILE_ATTRIBUTE_HIDDEN) != 0'i32
   else:
-    if fileExists(path):
-      let
-        fileName = extractFilename(path)
-        nameLen = len(fileName)
-      if nameLen == 2:
-        result = (fileName[0] == '.') and (fileName[1] != '.')
-      elif nameLen > 2:
-        result = (fileName[0] == '.') and (fileName[3] != '.')
+    let fileName = lastPathPart(path)
+    result = len(fileName) >= 2 and fileName[0] == '.' and fileName != ".."
 
 {.pop.}
 

diff --git a/tests/stdlib/tos.nim b/tests/stdlib/tos.nim
@@ -53,124 +53,125 @@ true
 
 import os, strutils
 
-let files = @["these.txt", "are.x", "testing.r", "files.q"]
-let dirs = @["some", "created", "test", "dirs"]
+block fileOperations:
+  let files = @["these.txt", "are.x", "testing.r", "files.q"]
+  let dirs = @["some", "created", "test", "dirs"]
 
-let dname = "__really_obscure_dir_name"
+  let dname = "__really_obscure_dir_name"
 
-createDir(dname)
-echo dirExists(dname)
-
-# Test creating files and dirs
-for dir in dirs:
-  createDir(dname/dir)
-  echo dirExists(dname/dir)
+  createDir(dname)
+  echo dirExists(dname)
 
-for file in files:
-  let fh = open(dname/file, fmReadWrite)
-  fh.close()
-  echo fileExists(dname/file)
+  # Test creating files and dirs
+  for dir in dirs:
+    createDir(dname/dir)
+    echo dirExists(dname/dir)
 
-echo "All:"
+  for file in files:
+    let fh = open(dname/file, fmReadWrite)
+    fh.close()
+    echo fileExists(dname/file)
 
-template norm(x): untyped =
-  (when defined(windows): x.replace('\\', '/') else: x)
+  echo "All:"
 
-for path in walkPattern(dname/"*"):
-  echo path.norm
+  template norm(x): untyped =
+    (when defined(windows): x.replace('\\', '/') else: x)
 
-echo "Files:"
+  for path in walkPattern(dname/"*"):
+    echo path.norm
 
-for path in walkFiles(dname/"*"):
-  echo path.norm
+  echo "Files:"
 
-echo "Dirs:"
+  for path in walkFiles(dname/"*"):
+    echo path.norm
 
-for path in walkDirs(dname/"*"):
-  echo path.norm
+  echo "Dirs:"
 
-# Test removal of files dirs
-for dir in dirs:
-  removeDir(dname/dir)
-  echo dirExists(dname/dir)
+  for path in walkDirs(dname/"*"):
+    echo path.norm
 
-for file in files:
-  removeFile(dname/file)
-  echo fileExists(dname/file)
+  # Test removal of files dirs
+  for dir in dirs:
+    removeDir(dname/dir)
+    echo dirExists(dname/dir)
 
-removeDir(dname)
-echo dirExists(dname)
+  for file in files:
+    removeFile(dname/file)
+    echo fileExists(dname/file)
 
-# createDir should create recursive directories
-createDir(dirs[0] / dirs[1])
-echo dirExists(dirs[0] / dirs[1]) # true
-removeDir(dirs[0])
+  removeDir(dname)
+  echo dirExists(dname)
 
-# createDir should properly handle trailing separator
-createDir(dname / "")
-echo dirExists(dname) # true
-removeDir(dname)
+  # createDir should create recursive directories
+  createDir(dirs[0] / dirs[1])
+  echo dirExists(dirs[0] / dirs[1]) # true
+  removeDir(dirs[0])
 
-# createDir should raise IOError if the path exists
-# and is not a directory
-open(dname, fmWrite).close
-try:
-  createDir(dname)
-except IOError:
-  echo "Raises"
-removeFile(dname)
+  # createDir should properly handle trailing separator
+  createDir(dname / "")
+  echo dirExists(dname) # true
+  removeDir(dname)
 
-# removeFile should not remove directory
-createDir(dname)
-try:
+  # createDir should raise IOError if the path exists
+  # and is not a directory
+  open(dname, fmWrite).close
+  try:
+    createDir(dname)
+  except IOError:
+    echo "Raises"
   removeFile(dname)
-except OSError:
-  echo "Raises"
-removeDir(dname)
 
-# test copyDir:
-createDir("a/b")
-open("a/b/file.txt", fmWrite).close
-createDir("a/b/c")
-open("a/b/c/fileC.txt", fmWrite).close
-
-copyDir("a", "../dest/a")
-removeDir("a")
+  # removeFile should not remove directory
+  createDir(dname)
+  try:
+    removeFile(dname)
+  except OSError:
+    echo "Raises"
+  removeDir(dname)
 
-echo dirExists("../dest/a/b")
-echo fileExists("../dest/a/b/file.txt")
+  # test copyDir:
+  createDir("a/b")
+  open("a/b/file.txt", fmWrite).close
+  createDir("a/b/c")
+  open("a/b/c/fileC.txt", fmWrite).close
 
-echo fileExists("../dest/a/b/c/fileC.txt")
-removeDir("../dest")
+  copyDir("a", "../dest/a")
+  removeDir("a")
 
-# test copyDir:
-# if separator at the end of a path
-createDir("a/b")
-open("a/file.txt", fmWrite).close
+  echo dirExists("../dest/a/b")
+  echo fileExists("../dest/a/b/file.txt")
 
-copyDir("a/", "../dest/a/")
-removeDir("a")
+  echo fileExists("../dest/a/b/c/fileC.txt")
+  removeDir("../dest")
 
-echo dirExists("../dest/a/b")
-echo fileExists("../dest/a/file.txt")
-removeDir("../dest")
+  # test copyDir:
+  # if separator at the end of a path
+  createDir("a/b")
+  open("a/file.txt", fmWrite).close
 
-# Test get/set modification times
-# Should support at least microsecond resolution
-import times
-let tm = fromUnix(0) + 100.microseconds
-writeFile("a", "")
-setLastModificationTime("a", tm)
+  copyDir("a/", "../dest/a/")
+  removeDir("a")
 
-when defined(macosx):
-  echo "true"
-else:
-  echo getLastModificationTime("a") == tm
-removeFile("a")
+  echo dirExists("../dest/a/b")
+  echo fileExists("../dest/a/file.txt")
+  removeDir("../dest")
 
-when defined(posix):
-
-  block normalizedPath:
+import times
+block modificationTime:
+  # Test get/set modification times
+  # Should support at least microsecond resolution
+  let tm = fromUnix(0) + 100.microseconds
+  writeFile("a", "")
+  setLastModificationTime("a", tm)
+
+  when defined(macosx):
+    echo "true"
+  else:
+    echo getLastModificationTime("a") == tm
+  removeFile("a")
+
+block normalizedPath:
+  when defined(posix):
     block relative:
       doAssert normalizedPath(".") == "."
       doAssert normalizedPath("..") == ".."
@@ -198,9 +199,7 @@ when defined(posix):
       doAssert normalizedPath("/a/b/c/..") == "/a/b"
       doAssert normalizedPath("/a/b/c/../") == "/a/b"
 
-else:
-
-  block normalizedPath:
+  else:
     block relative:
       doAssert normalizedPath(".") == "."
       doAssert normalizedPath("..") == ".."
@@ -227,3 +226,23 @@ else:
       doAssert normalizedPath("\\a\\\\\\b") == "\\a\\b"
       doAssert normalizedPath("\\a\\b\\c\\..") == "\\a\\b"
       doAssert normalizedPath("\\a\\b\\c\\..\\") == "\\a\\b"
+
+block isHidden:
+  when defined(posix):
+    doAssert ".foo.txt".isHidden
+    doAssert "bar/.foo.ext".isHidden
+    doAssert: not "bar".isHidden
+    doAssert: not "foo/".isHidden
+    # Corner cases: paths are not normalized when determining `isHidden`
+    doAssert: not ".foo/.".isHidden
+    doAssert: not ".foo/..".isHidden
+
+block absolutePath:
+  doAssertRaises(ValueError): discard absolutePath("a", "b")
+  doAssert absolutePath("a") == getCurrentDir() / "a"
+  doAssert absolutePath("a", "/b") == "/b" / "a"
+  when defined(Posix):
+    doAssert absolutePath("a", "/b/") == "/b" / "a"
+    doAssert absolutePath("a", "/b/c") == "/b/c" / "a"
+    doAssert absolutePath("/a", "b/") == "/a"
+