[RFC] vim-patch:8.1.1378: delete() can not handle a file name that looks li… (#16268)

Problem:    Delete() can not handle a file name that looks like a pattern.
Solution:   Use readdir() instead of appending "/*" and expanding wildcards.
            (Ken Takata, closes vim/vim#4424, closes vim/vim#696)
701ff0a3e5

Cherry-pick a change to Test_delete_rf() from patch 8.1.1921.

Co-authored-by: zeertzjq <zeertzjq@outlook.com>

src/nvim/eval/funcs.c

@@ -6702,15 +6702,21 @@ static void f_range(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   }
 }
 
-/// Evaluate "expr" for readdir().
+/// Evaluate "expr" (= "context") for readdir().
-static varnumber_T readdir_checkitem(typval_T *expr, const char *name)
+static varnumber_T readdir_checkitem(void *context, const char *name)
+  FUNC_ATTR_NONNULL_ALL
 {
+  typval_T *expr = (typval_T *)context;
   typval_T save_val;
   typval_T rettv;
   typval_T argv[2];
   varnumber_T retval = 0;
   bool error = false;
 
+  if (expr->v_type == VAR_UNKNOWN) {
+    return 1;
+  }
+
   prepare_vimvar(VV_VAL, &save_val);
   set_vim_var_string(VV_VAL, name, -1);
   argv[0].v_type = VAR_STRING;
@@ -6736,54 +6742,16 @@ theend:
 /// "readdir()" function
 static void f_readdir(typval_T *argvars, typval_T *rettv, FunPtr fptr)
 {
-  typval_T *expr;
-  const char *path;
-  garray_T ga;
-  Directory dir;
-
   tv_list_alloc_ret(rettv, kListLenUnknown);
-  path = tv_get_string(&argvars[0]);
-  expr = &argvars[1];
-  ga_init(&ga, (int)sizeof(char *), 20);
 
-  if (!os_scandir(&dir, path)) {
+  const char *path = tv_get_string(&argvars[0]);
-    smsg(_(e_notopen), path);
+  typval_T *expr = &argvars[1];
-  } else {
+  garray_T ga;
-    for (;;) {
+  int ret = readdir_core(&ga, path, (void *)expr, readdir_checkitem);
-      bool ignore;
+  if (ret == OK && ga.ga_len > 0) {
-
-      path = os_scandir_next(&dir);
-      if (path == NULL) {
-        break;
-      }
-
-      ignore = (path[0] == '.'
-                && (path[1] == NUL || (path[1] == '.' && path[2] == NUL)));
-      if (!ignore && expr->v_type != VAR_UNKNOWN) {
-        varnumber_T r = readdir_checkitem(expr, path);
-
-        if (r < 0) {
-          break;
-        }
-        if (r == 0) {
-          ignore = true;
-        }
-      }
-
-      if (!ignore) {
-        ga_grow(&ga, 1);
-        ((char **)ga.ga_data)[ga.ga_len++] = xstrdup(path);
-      }
-    }
-
-    os_closedir(&dir);
-  }
-
-  if (rettv->vval.v_list != NULL && ga.ga_len > 0) {
-    sort_strings((char_u **)ga.ga_data, ga.ga_len);
     for (int i = 0; i < ga.ga_len; i++) {
-      path = ((const char **)ga.ga_data)[i];
+      const char *p = ((const char **)ga.ga_data)[i];
-      tv_list_append_string(rettv->vval.v_list, path, -1);
+      tv_list_append_string(rettv->vval.v_list, p, -1);
     }
   }
   ga_clear_strings(&ga);

src/nvim/fileio.c

@@ -22,6 +22,7 @@
 #include "nvim/diff.h"
 #include "nvim/edit.h"
 #include "nvim/eval/userfunc.h"
+#include "nvim/eval/typval.h"
 #include "nvim/ex_cmds.h"
 #include "nvim/ex_docmd.h"
 #include "nvim/ex_eval.h"
@@ -5343,37 +5344,82 @@ static void vim_maketempdir(void)
   (void)umask(umask_save);
 }
 
+/// Core part of "readdir()" function.
+/// Retrieve the list of files/directories of "path" into "gap".
+///
+/// @return  OK for success, FAIL for failure.
+int readdir_core(garray_T *gap, const char *path, void *context, CheckItem checkitem)
+  FUNC_ATTR_NONNULL_ARG(1, 2)
+{
+  ga_init(gap, (int)sizeof(char *), 20);
+
+  Directory dir;
+  if (!os_scandir(&dir, path)) {
+    smsg(_(e_notopen), path);
+    return FAIL;
+  }
+
+  for (;;) {
+    const char *p = os_scandir_next(&dir);
+    if (p == NULL) {
+      break;
+    }
+
+    bool ignore = (p[0] == '.' && (p[1] == NUL || (p[1] == '.' && p[2] == NUL)));
+    if (!ignore && checkitem != NULL) {
+      varnumber_T r = checkitem(context, p);
+      if (r < 0) {
+        break;
+      }
+      if (r == 0) {
+        ignore = true;
+      }
+    }
+
+    if (!ignore) {
+      ga_grow(gap, 1);
+      ((char **)gap->ga_data)[gap->ga_len++] = xstrdup(p);
+    }
+  }
+
+  os_closedir(&dir);
+
+  if (gap->ga_len > 0) {
+    sort_strings((char_u **)gap->ga_data, gap->ga_len);
+  }
+
+  return OK;
+}
+
 /// Delete "name" and everything in it, recursively.
 ///
 /// @param name  The path which should be deleted.
 ///
 /// @return  0 for success, -1 if some file was not deleted.
 int delete_recursive(const char *name)
+  FUNC_ATTR_NONNULL_ALL
 {
   int result = 0;
 
   if (os_isrealdir(name)) {
-    snprintf((char *)NameBuff, MAXPATHL, "%s/*", name);  // NOLINT
+    char *exp = xstrdup(name);
-
+    garray_T ga;
-    char_u **files;
+    if (readdir_core(&ga, exp, NULL, NULL) == OK) {
-    int file_count;
+      for (int i = 0; i < ga.ga_len; i++) {
-    char_u *exp = vim_strsave(NameBuff);
+        vim_snprintf((char *)NameBuff, MAXPATHL, "%s/%s", exp, ((char_u **)ga.ga_data)[i]);
-    if (gen_expand_wildcards(1, &exp, &file_count, &files,
+        if (delete_recursive((const char *)NameBuff) != 0) {
-                             EW_DIR | EW_FILE | EW_SILENT | EW_ALLLINKS
-                             | EW_DODOT | EW_EMPTYOK) == OK) {
-      for (int i = 0; i < file_count; i++) {
-        if (delete_recursive((const char *)files[i]) != 0) {
           result = -1;
         }
       }
-      FreeWild(file_count, files);
+      ga_clear_strings(&ga);
     } else {
       result = -1;
     }
-
+    // Note: "name" value may be changed in delete_recursive().  Must use the saved value.
+    result = os_rmdir(exp) == 0 ? 0 : -1;
     xfree(exp);
-    os_rmdir(name);
   } else {
+    // Delete symlink only.
     result = os_remove(name) == 0 ? 0 : -1;
   }
 

src/nvim/fileio.h

@@ -3,6 +3,8 @@
 
 #include "nvim/autocmd.h"
 #include "nvim/buffer_defs.h"
+#include "nvim/eval/typval.h"
+#include "nvim/garray.h"
 #include "nvim/os/os.h"
 
 // Values for readfile() flags
@@ -17,6 +19,8 @@
 
 #define READ_STRING(x, y) (char_u *)read_string((x), (size_t)(y))
 
+typedef varnumber_T (*CheckItem)(void *expr, const char *name);
+
 #ifdef INCLUDE_GENERATED_DECLARATIONS
 // Events for autocommands
 # include "fileio.h.generated.h"

src/nvim/testdir/test_functions.vim

@@ -1620,10 +1620,6 @@ func Test_platform_name()
 endfunc
 
 func Test_readdir()
-  if isdirectory('Xdir')
-    call delete('Xdir', 'rf')
-  endif
-
   call mkdir('Xdir')
   call writefile([], 'Xdir/foo.txt')
   call writefile([], 'Xdir/bar.txt')
@@ -1653,6 +1649,21 @@ func Test_readdir()
   call delete('Xdir', 'rf')
 endfunc
 
+func Test_delete_rf()
+  call mkdir('Xdir')
+  call writefile([], 'Xdir/foo.txt')
+  call writefile([], 'Xdir/bar.txt')
+  call mkdir('Xdir/[a-1]')  " issue #696
+  call writefile([], 'Xdir/[a-1]/foo.txt')
+  call writefile([], 'Xdir/[a-1]/bar.txt')
+  call assert_true(filereadable('Xdir/foo.txt'))
+  call assert_true('Xdir/[a-1]/foo.txt'->filereadable())
+
+  call assert_equal(0, delete('Xdir', 'rf'))
+  call assert_false(filereadable('Xdir/foo.txt'))
+  call assert_false(filereadable('Xdir/[a-1]/foo.txt'))
+endfunc
+
 func Test_call()
   call assert_equal(3, call('len', [123]))
   call assert_equal(3, 'len'->call([123]))

test/functional/legacy/delete_spec.lua

@@ -1,6 +1,7 @@
 local helpers = require('test.functional.helpers')(after_each)
 local clear, source = helpers.clear, helpers.source
 local eq, eval, command = helpers.eq, helpers.eval, helpers.command
+local exc_exec = helpers.exc_exec
 
 describe('Test for delete()', function()
   before_each(clear)
@@ -8,6 +9,23 @@ describe('Test for delete()', function()
     os.remove('Xfile')
   end)
 
+  it('file delete', function()
+    command('split Xfile')
+    command("call setline(1, ['a', 'b'])")
+    command('wq')
+    eq(eval("['a', 'b']"), eval("readfile('Xfile')"))
+    eq(0, eval("delete('Xfile')"))
+    eq(-1, eval("delete('Xfile')"))
+  end)
+
+  it('directory delete', function()
+    command("call mkdir('Xdir1')")
+    eq(1, eval("isdirectory('Xdir1')"))
+    eq(0, eval("delete('Xdir1', 'd')"))
+    eq(0, eval("isdirectory('Xdir1')"))
+    eq(-1, eval("delete('Xdir1', 'd')"))
+  end)
+
   it('symlink delete', function()
     source([[
       split Xfile
@@ -43,39 +61,8 @@ describe('Test for delete()', function()
     eq(0, eval("delete('Xdir1', 'd')"))
   end)
 
-  it('symlink recursive delete', function()
+  it('gives correct emsgs', function()
-    source([[
+    eq('Vim(call):E474: Invalid argument', exc_exec("call delete('')"))
-      call mkdir('Xdir3')
+    eq('Vim(call):E15: Invalid expression: 0', exc_exec("call delete('foo', 0)"))
-      call mkdir('Xdir3/subdir')
-      call mkdir('Xdir4')
-      split Xdir3/Xfile
-      call setline(1, ['a', 'b'])
-      w
-      w Xdir3/subdir/Xfile
-      w Xdir4/Xfile
-      close
-      if has('win32')
-        silent !mklink /j Xdir3\Xlink Xdir4
-      else
-        silent !ln -s ../Xdir4 Xdir3/Xlink
-      endif
-    ]])
-
-    eq(1, eval("isdirectory('Xdir3')"))
-    eq(eval("['a', 'b']"), eval("readfile('Xdir3/Xfile')"))
-    eq(1, eval("isdirectory('Xdir3/subdir')"))
-    eq(eval("['a', 'b']"), eval("readfile('Xdir3/subdir/Xfile')"))
-    eq(1, eval("isdirectory('Xdir4')"))
-    eq(1, eval("isdirectory('Xdir3/Xlink')"))
-    eq(eval("['a', 'b']"), eval("readfile('Xdir4/Xfile')"))
-
-    eq(0, eval("delete('Xdir3', 'rf')"))
-    eq(0, eval("isdirectory('Xdir3')"))
-    eq(-1, eval("delete('Xdir3', 'd')"))
-    -- symlink is deleted, not the directory it points to
-    eq(1, eval("isdirectory('Xdir4')"))
-    eq(eval("['a', 'b']"), eval("readfile('Xdir4/Xfile')"))
-    eq(0, eval("delete('Xdir4/Xfile')"))
-    eq(0, eval("delete('Xdir4', 'd')"))
   end)
 end)