Fix bugs, clean code, add tests.

* Add const specifiers, update comments, add assert.
* Move os_unix.moon tests to os/fs.moon + clean tests.
* Add uv_fs_req_cleanup call.
* Add tests with absolute paths to mch_isdir.
* Add to_cstr to test/unit/helpers.moon and fix respective unit tests.

7 files changed, 131 insertions, 139 deletions

diff --git a/src/os/fs.c b/src/os/fs.c
index d8b1fc7cb0..a76a6358ff 100644
--- a/src/os/fs.c
+++ b/src/os/fs.c
@@ -89,7 +89,7 @@ int mch_full_dir_name(char *directory, char *buffer, int len)
 /*
  * Append to_append to path with a slash in between.
  */
-int append_path(char *path, char *to_append, int max_len)
+int append_path(char *path, const char *to_append, int max_len)
 {
   int current_length = STRLEN(path);
   int to_append_length = STRLEN(to_append);
@@ -163,7 +163,7 @@ int mch_get_absolute_path(char_u *fname, char_u *buf, int len, int force)
 /*
  * Return TRUE if "fname" does not depend on the current directory.
  */
-int mch_is_absolute_path(char_u *fname)
+int mch_is_absolute_path(const char_u *fname)
 {
   return *fname == '/' || *fname == '~';
 }
@@ -173,7 +173,7 @@ int mch_is_absolute_path(char_u *fname)
  * return FALSE if "name" is not a directory
  * return FALSE for error
  */
-int mch_isdir(char_u *name)
+int mch_isdir(const char_u *name)
 {
   uv_fs_t request;
   int result = uv_fs_stat(uv_default_loop(), &request, (const char*) name, NULL);
@@ -192,14 +192,14 @@ int mch_isdir(char_u *name)
   return TRUE;
 }
 
-static int is_executable(char_u *name);
-static int is_executable_in_path(char_u *name);
+static int is_executable(const char_u *name);
+static int is_executable_in_path(const char_u *name);
 
 /*
  * Return TRUE if "name" is executable and can be found in $PATH, is absolute
  * or relative to current dir, FALSE if not.
  */
-int mch_can_exe(char_u *name)
+int mch_can_exe(const char_u *name)
 {
   /* If it's an absolute or relative path don't need to use $PATH. */
   if (mch_is_absolute_path(name) ||
@@ -212,17 +212,21 @@ int mch_can_exe(char_u *name)
 }
 
 /*
- * Return 1 if "name" is an executable file, 0 if not or it doesn't exist.
+ * Return TRUE if "name" is an executable file, FALSE if not or it doesn't
+ * exist.
  */
-static int is_executable(char_u *name)
+static int is_executable(const char_u *name)
 {
   uv_fs_t request;
-  if (0 != uv_fs_stat(uv_default_loop(), &request, (const char*) name, NULL)) {
+  int result = uv_fs_stat(uv_default_loop(), &request, (const char*) name, NULL);
+  uint64_t mode = request.statbuf.st_mode;
+  uv_fs_req_cleanup(&request);
+
+  if (result != 0) {
     return FALSE;
   }
 
-  if (S_ISREG(request.statbuf.st_mode) &&
-     (S_IEXEC & request.statbuf.st_mode)) {
+  if (S_ISREG(mode) && (S_IEXEC & mode)) {
     return TRUE;
   }
 
@@ -233,9 +237,9 @@ static int is_executable(char_u *name)
  * Return TRUE if "name" can be found in $PATH and executed, FALSE if not or an
  * error occurs.
  */
-static int is_executable_in_path(char_u *name)
+static int is_executable_in_path(const char_u *name)
 {
-  char_u *path = (char_u *)getenv("PATH");
+  const char *path = getenv("PATH");
   /* PATH environment variable does not exist or is empty. */
   if (path == NULL || *path == NUL) {
     return FALSE;
@@ -252,15 +256,15 @@ static int is_executable_in_path(char_u *name)
    * is an executable file.
    */
   for (;; ) {
-    char_u *e = (char_u *)strchr((char *)path, ':');
+    const char *e = strchr(path, ':');
     if (e == NULL) {
       e = path + STRLEN(path);
     }
 
     /* Glue together the given directory from $PATH with name and save into
      * buf. */
-    vim_strncpy(buf, path, e - path);
-    append_path((char *) buf, (char *) name, buf_len);
+    vim_strncpy(buf, (char_u *) path, e - path);
+    append_path((char *) buf, (const char *) name, buf_len);
 
     if (is_executable(buf)) {
       /* Found our executable. Free buf and return. */
@@ -269,7 +273,7 @@ static int is_executable_in_path(char_u *name)
     }
 
     if (*e != ':') {
-      /* End of $PATH without without finding any executable called name. */
+      /* End of $PATH without finding any executable called name. */
       vim_free(buf);
       return FALSE;
     }
@@ -277,5 +281,7 @@ static int is_executable_in_path(char_u *name)
     path = e + 1;
   }
 
+  /* We should never get to this point. */
+  assert(false);
   return FALSE;
 }
diff --git a/src/os/os.h b/src/os/os.h
index 1d3a0d2c2b..48e3ffd0dd 100644
--- a/src/os/os.h
+++ b/src/os/os.h
@@ -7,9 +7,9 @@ long_u mch_total_mem(int special);
 int mch_chdir(char *path);
 int mch_dirname(char_u *buf, int len);
 int mch_get_absolute_path(char_u *fname, char_u *buf, int len, int force);
-int mch_is_absolute_path(char_u *fname);
-int mch_isdir(char_u *name);
-int mch_can_exe(char_u *name);
+int mch_is_absolute_path(const char_u *fname);
+int mch_isdir(const char_u *name);
+int mch_can_exe(const char_u *name);
 const char *mch_getenv(const char *name);
 int mch_setenv(const char *name, const char *value, int overwrite);
 char *mch_getenvname_at_index(size_t index);
diff --git a/test/unit/helpers.moon b/test/unit/helpers.moon
index c12a6473f6..b1670d01ea 100644
--- a/test/unit/helpers.moon
+++ b/test/unit/helpers.moon
@@ -30,11 +30,17 @@ internalize = (cdata) ->
   ffi.gc cdata, ffi.C.free
   return ffi.string cdata
 
+cstr = ffi.typeof 'char[?]'
+
+to_cstr = (string) ->
+  cstr (string.len string) + 1, string
+
 return {
   cimport: cimport
   internalize: internalize
   eq: (expected, actual) -> assert.are.same expected, actual
   ffi: ffi
   lib: libnvim
-  cstr: ffi.typeof 'char[?]'
+  cstr: cstr
+  to_cstr: to_cstr
 }
diff --git a/test/unit/misc1.moon b/test/unit/misc1.moon
index d67f867913..f408f3d5ec 100644
--- a/test/unit/misc1.moon
+++ b/test/unit/misc1.moon
@@ -1,4 +1,4 @@
-{:cimport, :internalize, :eq, :ffi, :lib, :cstr} = require 'test.unit.helpers'
+{:cimport, :internalize, :eq, :ffi, :lib, :cstr, :to_cstr} = require 'test.unit.helpers'
 
 --misc1 = cimport './src/misc1.h'
 
@@ -18,8 +18,8 @@ describe 'misc1 function', ->
   describe 'fullpathcmp', ->
 
     fullpathcmp = (s1, s2, cn) ->
-      s1 = cstr (string.len s1) + 1, s1
-      s2 = cstr (string.len s2) + 1, s2
+      s1 = to_cstr s1
+      s2 = to_cstr s2
       misc1.fullpathcmp s1, s2, cn or 0
 
     f1 = 'f1.o'
diff --git a/test/unit/os/env.moon b/test/unit/os/env.moon
index b83fcac5f1..007a9beaff 100644
--- a/test/unit/os/env.moon
+++ b/test/unit/os/env.moon
@@ -1,4 +1,4 @@
-{:cimport, :internalize, :eq, :ffi, :lib, :cstr} = require 'test.unit.helpers'
+{:cimport, :internalize, :eq, :ffi, :lib, :cstr, :to_cstr} = require 'test.unit.helpers'
 require 'lfs'
 
 -- fs = cimport './src/os/os.h'
@@ -10,18 +10,15 @@ int mch_setenv(const char *name, const char *value, int override);
 char *mch_getenvname_at_index(size_t index);
 ]]
 
-str_to_charp = (str) ->
-  cstr (string.len str), str
-
 NULL = ffi.cast 'void*', 0
 
 describe 'env function', ->
 
   mch_setenv = (name, value, override) ->
-    env.mch_setenv (str_to_charp name), (str_to_charp value), override
+    env.mch_setenv (to_cstr name), (to_cstr value), override
 
   mch_getenv = (name) ->
-    rval = env.mch_getenv (str_to_charp name)
+    rval = env.mch_getenv (to_cstr name)
     if rval != NULL
       ffi.string rval
     else
diff --git a/test/unit/os/fs.moon b/test/unit/os/fs.moon
index 81246ddb62..8f141d689a 100644
--- a/test/unit/os/fs.moon
+++ b/test/unit/os/fs.moon
@@ -1,4 +1,4 @@
-{:cimport, :internalize, :eq, :ffi, :lib, :cstr} = require 'test.unit.helpers'
+{:cimport, :internalize, :eq, :ffi, :lib, :cstr, :to_cstr} = require 'test.unit.helpers'
 require 'lfs'
 
 -- fs = cimport './src/os/os.h'
@@ -8,16 +8,36 @@ ffi.cdef [[
 enum OKFAIL {
   OK = 1, FAIL = 0
 };
+enum BOOLEAN {
+  TRUE = 1, FALSE = 0
+};
 int mch_dirname(char_u *buf, int len);
+int mch_isdir(char_u * name);
+int is_executable(char_u *name);
+int mch_can_exe(char_u *name);
 ]]
 
 -- import constants parsed by ffi
 {:OK, :FAIL} = lib
+{:TRUE, :FALSE} = lib
 
 describe 'fs function', ->
+  setup ->
+    lfs.mkdir 'unit-test-directory'
+    lfs.touch 'unit-test-directory/test.file'
 
-  describe 'mch_dirname', ->
+    -- Since the tests are executed, they are called by an executable. We use
+    -- that executable for several asserts.
+    export absolute_executable = arg[0]
+
+    -- Split absolute_executable into a directory and the actual file name for
+    -- later usage.
+    export directory, executable_name = string.match(absolute_executable, '^(.*)/(.*)$')
 
+  teardown ->
+    lfs.rmdir 'unit-test-directory'
+
+  describe 'mch_dirname', ->
     mch_dirname = (buf, len) ->
       fs.mch_dirname buf, len
 
@@ -39,8 +59,8 @@ describe 'fs function', ->
     ffi.cdef 'int mch_full_dir_name(char *directory, char *buffer, int len);'
 
     mch_full_dir_name = (directory, buffer, len) ->
-      directory = cstr (string.len directory), directory
-      fs.mch_full_dir_name(directory, buffer, len)
+      directory = to_cstr directory
+      fs.mch_full_dir_name directory, buffer, len
 
     before_each ->
       -- Create empty string buffer which will contain the resulting path.
@@ -64,17 +84,15 @@ describe 'fs function', ->
       eq FAIL, mch_full_dir_name('does_not_exist', buffer, len)
 
     it 'works with a normal relative dir', ->
-      lfs.mkdir 'empty-test-directory'
-      result = mch_full_dir_name('empty-test-directory', buffer, len)
-      lfs.rmdir 'empty-test-directory'
-      eq lfs.currentdir! .. '/empty-test-directory', (ffi.string buffer)
+      result = mch_full_dir_name('unit-test-directory', buffer, len)
+      eq lfs.currentdir! .. '/unit-test-directory', (ffi.string buffer)
       eq OK, result
 
   describe 'mch_get_absolute_path', ->
     ffi.cdef 'int mch_get_absolute_path(char *fname, char *buf, int len, int force);'
 
     mch_get_absolute_path = (filename, buffer, length, force) ->
-      filename = cstr (string.len filename) + 1, filename
+      filename = to_cstr filename
       fs.mch_get_absolute_path filename, buffer, length, force
 
     before_each ->
@@ -82,14 +100,6 @@ describe 'fs function', ->
       export len = (string.len lfs.currentdir!) + 33
       export buffer = cstr len, ''
 
-      -- Create a directory and an empty file inside in order to know some
-      -- existing relative path.
-      lfs.mkdir 'empty-test-directory'
-      lfs.touch 'empty-test-directory/empty.file'
-
-    after_each ->
-      lfs.rmdir 'empty-test-directory'
-
     it 'fails if given filename contains non-existing directory', ->
       force_expansion = 1
       result = mch_get_absolute_path 'non_existing_dir/test.file', buffer, len, force_expansion
@@ -138,16 +148,18 @@ describe 'fs function', ->
 
     it 'works with some "normal" relative path with directories', ->
       force_expansion = 1
-      result = mch_get_absolute_path 'empty-test-directory/empty.file', buffer, len, force_expansion
+      result = mch_get_absolute_path 'unit-test-directory/test.file', buffer, len, force_expansion
       eq OK, result
-      eq lfs.currentdir! .. '/empty-test-directory/empty.file', (ffi.string buffer)
+      eq lfs.currentdir! .. '/unit-test-directory/test.file', (ffi.string buffer)
 
     it 'does not modify the given filename', ->
       force_expansion = 1
-      filename = cstr 100, 'empty-test-directory/empty.file'
+      filename = to_cstr 'unit-test-directory/test.file'
+      -- Don't use the wrapper here but pass a cstring directly to the c
+      -- function.
       result = fs.mch_get_absolute_path filename, buffer, len, force_expansion
-      eq lfs.currentdir! .. '/empty-test-directory/empty.file', (ffi.string buffer)
-      eq 'empty-test-directory/empty.file', (ffi.string filename)
+      eq lfs.currentdir! .. '/unit-test-directory/test.file', (ffi.string buffer)
+      eq 'unit-test-directory/test.file', (ffi.string filename)
       eq OK, result
 
   describe 'append_path', ->
@@ -155,36 +167,36 @@ describe 'fs function', ->
 
     it 'joins given paths with a slash', ->
      path = cstr 100, 'path1'
-     to_append = cstr 6, 'path2'
+     to_append = to_cstr 'path2'
      eq OK, (fs.append_path path, to_append, 100)
      eq "path1/path2", (ffi.string path)
 
     it 'joins given paths without adding an unnecessary slash', ->
      path = cstr 100, 'path1/'
-     to_append = cstr 6, 'path2'
+     to_append = to_cstr 'path2'
      eq OK, fs.append_path path, to_append, 100
      eq "path1/path2", (ffi.string path)
 
     it 'fails if there is not enough space left for to_append', ->
       path = cstr 11, 'path1/'
-      to_append = cstr 6, 'path2'
+      to_append = to_cstr 'path2'
       eq FAIL, (fs.append_path path, to_append, 11)
 
     it 'does not append a slash if to_append is empty', ->
       path = cstr 6, 'path1'
-      to_append = cstr 1, ''
+      to_append = to_cstr ''
       eq OK, (fs.append_path path, to_append, 6)
       eq 'path1', (ffi.string path)
 
     it 'does not append unnecessary dots', ->
       path = cstr 6, 'path1'
-      to_append = cstr 2, '.'
+      to_append = to_cstr '.'
       eq OK, (fs.append_path path, to_append, 6)
       eq 'path1', (ffi.string path)
 
     it 'copies to_append to path, if path is empty', ->
       path = cstr 7, ''
-      to_append = cstr 7, '/path2'
+      to_append = to_cstr '/path2'
       eq OK, (fs.append_path path, to_append, 7)
       eq '/path2', (ffi.string path)
 
@@ -192,7 +204,7 @@ describe 'fs function', ->
     ffi.cdef 'int mch_is_absolute_path(char *fname);'
 
     mch_is_absolute_path = (filename) ->
-      filename = cstr (string.len filename) + 1, filename
+      filename = to_cstr filename
       fs.mch_is_absolute_path filename
 
     it 'returns true if filename starts with a slash', ->
@@ -203,3 +215,54 @@ describe 'fs function', ->
 
     it 'returns false if filename starts not with slash nor tilde', ->
       eq FAIL, mch_is_absolute_path 'not/in/my/home~/directory'
+
+  describe 'mch_isdir', ->
+    mch_isdir = (name) ->
+      fs.mch_isdir (to_cstr name)
+
+    it 'returns false if an empty string is given', ->
+      eq FALSE, (mch_isdir '')
+
+    it 'returns false if a nonexisting directory is given', ->
+      eq FALSE, (mch_isdir 'non-existing-directory')
+
+    it 'returns false if a nonexisting absolute directory is given', ->
+      eq FALSE, (mch_isdir '/non-existing-directory')
+
+    it 'returns false if an existing file is given', ->
+      eq FALSE, (mch_isdir 'unit-test-directory/test.file')
+
+    it 'returns true if the current directory is given', ->
+      eq TRUE, (mch_isdir '.')
+
+    it 'returns true if the parent directory is given', ->
+      eq TRUE, (mch_isdir '..')
+
+    it 'returns true if an arbitrary directory is given', ->
+      eq TRUE, (mch_isdir 'unit-test-directory')
+
+    it 'returns true if an absolute directory is given', ->
+      eq TRUE, (mch_isdir directory)
+
+  describe 'mch_can_exe', ->
+    mch_can_exe = (name) ->
+      fs.mch_can_exe (to_cstr name)
+
+    it 'returns false when given a directory', ->
+      eq FALSE, (mch_can_exe './unit-test-directory')
+
+    it 'returns false when given a regular file without executable bit set', ->
+      eq FALSE, (mch_can_exe 'unit-test-directory/test.file')
+
+    it 'returns false when the given file does not exists', ->
+      eq FALSE, (mch_can_exe 'does-not-exist.file')
+
+    it 'returns true when given an executable inside $PATH', ->
+      eq TRUE, (mch_can_exe executable_name)
+
+    it 'returns true when given an executable relative to the current dir', ->
+      old_dir = lfs.currentdir!
+      lfs.chdir directory
+      relative_executable = './' .. executable_name
+      eq TRUE, (mch_can_exe relative_executable)
+      lfs.chdir old_dir
diff --git a/test/unit/os_unix.moon b/test/unit/os_unix.moon
deleted file mode 100644
index 6e986de44f..0000000000
--- a/test/unit/os_unix.moon
+++ /dev/null
@@ -1,80 +0,0 @@
-{:cimport, :eq, :ffi, :lib, :cstr} = require 'test.unit.helpers'
-
--- os = cimport './src/os_unix.h'
-os = lib
-ffi.cdef [[
-enum BOOLEAN {
-  TRUE = 1, FALSE = 0
-};
-int mch_isdir(char_u * name);
-int is_executable(char_u *name);
-int mch_can_exe(char_u *name);
-]]
-
-{:TRUE, :FALSE} = lib
-
-describe 'os_unix function', ->
-  setup ->
-    lfs.mkdir 'unit-test-directory'
-    lfs.touch 'unit-test-directory/test.file'
-
-    -- Since the tests are executed, they are called by an executable. We use
-    -- that executable for several asserts.
-    export absolute_executable = arg[0]
-
-    -- Split absolute_executable into a directory and the actual file name for
-    -- later usage.
-    export directory, executable_name = if (string.find absolute_executable, '/')
-      string.match(absolute_executable, '^(.*)/(.*)$')
-    else
-      string.match(absolute_executable, '^(.*)\\(.*)$')
-
-  teardown ->
-    lfs.rmdir 'unit-test-directory'
-
-  describe 'mch_isdir', ->
-    mch_isdir = (name) ->
-      name = cstr (string.len name), name
-      os.mch_isdir(name)
-
-    it 'returns false if an empty string is given', ->
-      eq FALSE, (mch_isdir '')
-
-    it 'returns false if a nonexisting directory is given', ->
-      eq FALSE, (mch_isdir 'non-existing-directory')
-
-    it 'returns false if an existing file is given', ->
-      eq FALSE, (mch_isdir 'non-existing-directory/test.file')
-
-    it 'returns true if the current directory is given', ->
-      eq TRUE, (mch_isdir '.')
-
-    it 'returns true if the parent directory is given', ->
-      eq TRUE, (mch_isdir '..')
-
-    it 'returns true if an arbitrary directory is given', ->
-      eq TRUE, (mch_isdir 'unit-test-directory')
-
-  describe 'mch_can_exe', ->
-    mch_can_exe = (name) ->
-      name = cstr (string.len name), name
-      os.mch_can_exe name
-
-    it 'returns false when given a directory', ->
-      eq FALSE, (mch_can_exe './unit-test-directory')
-
-    it 'returns false when given a regular file without executable bit set', ->
-      eq FALSE, (mch_can_exe 'unit-test-directory/test.file')
-
-    it 'returns false when the given file does not exists', ->
-      eq FALSE, (mch_can_exe 'does-not-exist.file')
-
-    it 'returns true when given an executable inside $PATH', ->
-      eq TRUE, (mch_can_exe executable_name)
-
-    it 'returns true when given an executable relative to the current dir', ->
-      old_dir = lfs.currentdir!
-      lfs.chdir directory
-      relative_executable = './' .. executable_name
-      eq TRUE, (mch_can_exe relative_executable)
-      lfs.chdir old_dir