Merge #9709 'fileio: use os_copy to create backups'

ref #8288

runtime/doc/if_lua.txt

@@ -374,11 +374,6 @@ For example, to use the "nvim_get_current_line()" API function, call
 ------------------------------------------------------------------------------
 VIM							*lua-util*
 
-vim.inspect({object}, {options})                                 *vim.inspect*
-	Return a human-readable representation of the passed object. See
-	         https://github.com/kikito/inspect.lua
-	for details and possible options.
-
 vim.stricmp(a, b)					*lua-vim.stricmp*
 	Function used for case-insensitive string comparison.  Takes two 
 	string arguments and returns 0, 1 or -1 if strings are equal, a is 
@@ -422,18 +417,11 @@ vim.types						*lua-vim.types*
 ==============================================================================
 Lua module: vim                                                      *lua-vim*
 
-trim({s})                                                         *vim.trim()*
+inspect({object}, {options})                                   *vim.inspect()*
-                Trim whitespace (Lua pattern "%%s") from both sides of a
+                Return a human-readable representation of the given object.
-                string.
-
-                Parameters: ~
-                    {s}  String to trim
-
-                Return: ~
-                    String with whitespace removed from its beginning and end
 
                 See also: ~
-                    https://www.lua.org/pil/20.2.html
+                    https://github.com/kikito/inspect.lua
 
 
 
@@ -521,4 +509,17 @@ tbl_flatten({t})                                           *vim.tbl_flatten()*
                 Return: ~
                     Flattened copy of the given list-like table.
 
+trim({s})                                                         *vim.trim()*
+                Trim whitespace (Lua pattern "%%s") from both sides of a
+                string.
+
+                Parameters: ~
+                    {s}  String to trim
+
+                Return: ~
+                    String with whitespace removed from its beginning and end
+
+                See also: ~
+                    https://www.lua.org/pil/20.2.html
+
  vim:tw=78:ts=8:ft=help:norl:

runtime/lua/vim/shared.lua

@@ -168,6 +168,16 @@ local function tbl_flatten(t)
   return result
 end
 
+--- Trim whitespace (Lua pattern "%%s") from both sides of a string.
+---
+--@see https://www.lua.org/pil/20.2.html
+--@param s String to trim
+--@returns String with whitespace removed from its beginning and end
+local function trim(s)
+  assert(type(s) == 'string', 'Only strings can be trimmed')
+  return s:match('^%s*(.*%S)') or ''
+end
+
 local module = {
   deepcopy = deepcopy,
   gsplit = gsplit,
@@ -175,5 +185,6 @@ local module = {
   tbl_contains = tbl_contains,
   tbl_extend = tbl_extend,
   tbl_flatten = tbl_flatten,
+  trim = trim,
 }
 return module

src/nvim/fileio.c

@@ -2781,20 +2781,12 @@ buf_write (
     else
       backup_ext = p_bex;
 
-    if (backup_copy && (fd = os_open((char *)fname, O_RDONLY, 0)) >= 0) {
+    if (backup_copy) {
-      int bfd;
+      char_u *wp;
-      char_u      *copybuf, *wp;
+      int some_error = false;
-      int some_error = FALSE;
       char_u      *dirp;
       char_u      *rootname;
 
-      copybuf = verbose_try_malloc(BUFSIZE + 1);
-      if (copybuf == NULL) {
-        // out of memory
-        some_error = TRUE;
-        goto nobackup;
-      }
-
       /*
        * Try to make the backup in each directory in the 'bdir' option.
        *
@@ -2812,8 +2804,8 @@ buf_write (
         /*
          * Isolate one directory name, using an entry in 'bdir'.
          */
-        (void)copy_option_part(&dirp, copybuf, BUFSIZE, ",");
+        (void)copy_option_part(&dirp, IObuff, IOSIZE, ",");
-        rootname = get_file_in_dir(fname, copybuf);
+        rootname = get_file_in_dir(fname, IObuff);
         if (rootname == NULL) {
           some_error = TRUE;                /* out of memory */
           goto nobackup;
@@ -2875,87 +2867,47 @@ buf_write (
         if (backup != NULL) {
           /* remove old backup, if present */
           os_remove((char *)backup);
-          /* Open with O_EXCL to avoid the file being created while
+
-           * we were sleeping (symlink hacker attack?) */
+          // set file protection same as original file, but
-          bfd = os_open((char *)backup,
+          // strip s-bit.
-              O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW,
+          (void)os_setperm((const char *)backup, perm & 0777);
-              perm & 0777);
-          if (bfd < 0) {
-            xfree(backup);
-            backup = NULL;
-          } else {
-            // set file protection same as original file, but
-            // strip s-bit.
-            (void)os_setperm((const char *)backup, perm & 0777);
 
 #ifdef UNIX
-            /*
+          //
-             * Try to set the group of the backup same as the
+          // Try to set the group of the backup same as the original file. If
-             * original file. If this fails, set the protection
+          // this fails, set the protection bits for the group same as the
-             * bits for the group same as the protection bits for
+          // protection bits for others.
-             * others.
+          //
-             */
+          if (file_info_new.stat.st_gid != file_info_old.stat.st_gid
-            if (file_info_new.stat.st_gid != file_info_old.stat.st_gid
+              && os_chown((char *)backup, -1, file_info_old.stat.st_gid) != 0) {
-                && os_fchown(bfd, -1, file_info_old.stat.st_gid) != 0) {
+            os_setperm((const char *)backup,
-              os_setperm((const char *)backup,
+                       (perm & 0707) | ((perm & 07) << 3));
-                         (perm & 0707) | ((perm & 07) << 3));
+          }
-            }
-# ifdef HAVE_SELINUX
-            mch_copy_sec(fname, backup);
-# endif
 #endif
 
-            /*
+          // copy the file
-             * copy the file.
+          if (os_copy((char *)fname, (char *)backup, UV_FS_COPYFILE_FICLONE)
-             */
+              != 0) {
-            write_info.bw_fd = bfd;
+            SET_ERRMSG(_("E506: Can't write to backup file "
-            write_info.bw_buf = copybuf;
+                         "(add ! to override)"));
-#ifdef HAS_BW_FLAGS
+          }
-            write_info.bw_flags = FIO_NOCONVERT;
-#endif
-            while ((write_info.bw_len = read_eintr(fd, copybuf,
-                        BUFSIZE)) > 0) {
-              if (buf_write_bytes(&write_info) == FAIL) {
-                SET_ERRMSG(_(
-                    "E506: Can't write to backup file (add ! to override)"));
-                break;
-              }
-              os_breakcheck();
-              if (got_int) {
-                SET_ERRMSG(_(e_interr));
-                break;
-              }
-            }
 
-            int error;
-            if ((error = os_close(bfd)) != 0 && errmsg == NULL) {
-              SET_ERRMSG_ARG(_("E507: Close error for backup file "
-                               "(add ! to override): %s"),
-                             error);
-            }
-            if (write_info.bw_len < 0) {
-              SET_ERRMSG(_(
-                  "E508: Can't read file for backup (add ! to override)"));
-            }
 #ifdef UNIX
-            set_file_time(backup,
+          set_file_time(backup,
-                          file_info_old.stat.st_atim.tv_sec,
+                        file_info_old.stat.st_atim.tv_sec,
-                          file_info_old.stat.st_mtim.tv_sec);
+                        file_info_old.stat.st_mtim.tv_sec);
 #endif
 #ifdef HAVE_ACL
-            mch_set_acl(backup, acl);
+          mch_set_acl(backup, acl);
 #endif
 #ifdef HAVE_SELINUX
-            mch_copy_sec(fname, backup);
+          mch_copy_sec(fname, backup);
 #endif
-            break;
+          break;
-          }
         }
       }
-nobackup:
-      os_close(fd);  // Ignore errors for closing read file.
-      xfree(copybuf);
 
+nobackup:
       if (backup == NULL && errmsg == NULL) {
         SET_ERRMSG(_(
             "E509: Cannot create backup file (add ! to override)"));
@@ -3537,28 +3489,11 @@ restore_backup:
           MSG(_(e_interr));
           ui_flush();
         }
-        if ((fd = os_open((char *)backup, O_RDONLY, 0)) >= 0) {
-          if ((write_info.bw_fd = os_open((char *)fname,
-                                          O_WRONLY | O_CREAT | O_TRUNC,
-                                          perm & 0777)) >= 0) {
-            // copy the file.
-            write_info.bw_buf = smallbuf;
-#ifdef HAS_BW_FLAGS
-            write_info.bw_flags = FIO_NOCONVERT;
-#endif
-            while ((write_info.bw_len = read_eintr(fd, smallbuf,
-                                                   SMBUFSIZE)) > 0) {
-              if (buf_write_bytes(&write_info) == FAIL) {
-                break;
-              }
-            }
 
-            if (close(write_info.bw_fd) >= 0
+        // copy the file.
-                && write_info.bw_len == 0) {
+        if (os_copy((char *)backup, (char *)fname, UV_FS_COPYFILE_FICLONE)
-              end = 1;                          // success
+            == 0) {
-            }
+          end = 1;  // success
-          }
-          close(fd);            // ignore errors for closing read file
         }
       } else {
         if (vim_rename(backup, fname) == 0) {

src/nvim/lua/vim.lua

@@ -154,15 +154,11 @@ local function _update_package_paths()
   last_nvim_paths = cur_nvim_paths
 end
 
---- Trim whitespace (Lua pattern "%%s") from both sides of a string.
+--- Return a human-readable representation of the given object.
 ---
---@see https://www.lua.org/pil/20.2.html
+--@see https://github.com/kikito/inspect.lua
---@param s String to trim
+local function inspect(object, options)  -- luacheck: no unused
---@returns String with whitespace removed from its beginning and end
+  error(object, options)  -- Stub for gen_vimdoc.py
-local function trim(s)
-  assert(type(s) == 'string', 'Only strings can be trimmed')
-  local result = s:gsub('^%s+', ''):gsub('%s+$', '')
-  return result
 end
 
 local function __index(t, key)
@@ -181,7 +177,6 @@ local module = {
   _os_proc_children = _os_proc_children,
   _os_proc_info = _os_proc_info,
   _system = _system,
-  trim = trim,
 }
 
 setmetatable(module, {

src/nvim/os/fs.c

@@ -643,6 +643,21 @@ ptrdiff_t os_write(const int fd, const char *const buf, const size_t size,
   return (ptrdiff_t)written_bytes;
 }
 
+/// Copies a file from `path` to `new_path`.
+///
+/// @see http://docs.libuv.org/en/v1.x/fs.html#c.uv_fs_copyfile
+///
+/// @param path Path of file to be copied
+/// @param path_new Path of new file
+/// @param flags Bitwise OR of flags defined in <uv.h>
+/// @return 0 on success, or libuv error code on failure.
+int os_copy(const char *path, const char *new_path, int flags)
+{
+  int r;
+  RUN_UV_FS_FUNC(r, uv_fs_copyfile, path, new_path, flags, NULL);
+  return r;
+}
+
 /// Flushes file modifications to disk.
 ///
 /// @param fd the file descriptor of the file to flush to disk.
@@ -697,12 +712,24 @@ int os_setperm(const char *const name, int perm)
   return (r == kLibuvSuccess ? OK : FAIL);
 }
 
-/// Changes the ownership of the file referred to by the open file descriptor.
+/// Changes the owner and group of a file, like chown(2).
 ///
-/// @return `0` on success, a libuv error code on failure.
+/// @return 0 on success, or libuv error code on failure.
 ///
-/// @note If the `owner` or `group` is specified as `-1`, then that ID is not
+/// @note If `owner` or `group` is -1, then that ID is not changed.
-/// changed.
+int os_chown(const char *path, uv_uid_t owner, uv_gid_t group)
+{
+  int r;
+  RUN_UV_FS_FUNC(r, uv_fs_chown, path, owner, group, NULL);
+  return r;
+}
+
+/// Changes the owner and group of the file referred to by the open file
+/// descriptor, like fchown(2).
+///
+/// @return 0 on success, or libuv error code on failure.
+///
+/// @note If `owner` or `group` is -1, then that ID is not changed.
 int os_fchown(int fd, uv_uid_t owner, uv_gid_t group)
 {
   int r;

test/functional/core/fileio_spec.lua

@@ -10,6 +10,8 @@ local request = helpers.request
 local retry = helpers.retry
 local rmdir = helpers.rmdir
 local sleep = helpers.sleep
+local read_file = helpers.read_file
+local trim = helpers.trim
 
 describe('fileio', function()
   before_each(function()
@@ -18,6 +20,7 @@ describe('fileio', function()
     command(':qall!')
     os.remove('Xtest_startup_shada')
     os.remove('Xtest_startup_file1')
+    os.remove('Xtest_startup_file1~')
     os.remove('Xtest_startup_file2')
     rmdir('Xtest_startup_swapdir')
   end)
@@ -64,5 +67,25 @@ describe('fileio', function()
     command('write')
     eq(4, request('nvim__stats').fsync)
   end)
+
+  it('backup #9709', function()
+    clear({ args={ '-i', 'Xtest_startup_shada',
+                   '--cmd', 'set directory=Xtest_startup_swapdir' } })
+
+    command('write Xtest_startup_file1')
+    feed('ifoo<esc>')
+    command('set backup')
+    command('set backupcopy=yes')
+    command('write')
+    feed('Abar<esc>')
+    command('write')
+
+    local foobar_contents = trim(read_file('Xtest_startup_file1'))
+    local bar_contents = trim(read_file('Xtest_startup_file1~'))
+
+    eq('foobar', foobar_contents);
+    eq('foo', bar_contents);
+
+  end)
 end)
 

test/unit/helpers.lua

@@ -15,6 +15,7 @@ local dedent = global_helpers.dedent
 local neq = global_helpers.neq
 local map = global_helpers.map
 local eq = global_helpers.eq
+local trim = global_helpers.trim
 
 -- C constants.
 local NULL = ffi.cast('void*', 0)
@@ -119,10 +120,6 @@ local deinit = only_separate(function()
   end
 end)
 
-local function trim(s)
-  return s:match('^%s*(.*%S)') or ''
-end
-
 -- a Set that keeps around the lines we've already seen
 local cdefs_init = Set:new()
 local cdefs_mod = nil