[RDY] Refactor writefile() and create more tests for it #6111

ZyX-I · 2017-02-13T22:15:01Z

Part of #5119

According to the documentation fsync() may fail with EROFS or EINVAL if “file descriptor is bound to a special file which does not support synchronization” (e.g. /dev/stderr). This condition is completely valid in this case since main point of `file_fsync()` is dumping buffered input.

justinmk · 2017-02-13T22:35:42Z

src/nvim/eval.c

@@ -18153,17 +18178,25 @@ static void f_writefile(typval_T *argvars, typval_T *rettv, FunPtr fptr)

  // Always open the file in binary mode, library functions have a mind of
  // their own about CR-LF conversion.


"binary mode" isn't relevant now

justinmk · 2017-02-13T22:44:40Z

test/functional/eval/writefile_spec.lua

+    eq('a\0\0\0b', read_file(fname))
+  end)
+
+  it('correctly overwrites file', function()


Suggestion: "correctly" and "successfully" are redundant in test descriptions, it should be assumed that we (almost) always want correct behavior.

I usually use such word because previously it did overwrite file/etc, but did this incorrectly: e.g. it is some kind of regression test.

oni-link · 2017-02-14T13:54:48Z

src/nvim/eval.c

-        break;
+      const ptrdiff_t written = file_write(fp, "\n", 1);
+      if (written < 0) {
+        error = (int)written;


A previous error could be overwritten.

Previously it could attempt to write trailing newline before returning.

oni-link · 2017-02-14T17:04:41Z

src/nvim/eval.c

+    }
+    if ((error = file_free(fp)) != 0) {
+      emsgf(_("E80: Error when closing file %s: %s"),
+            fname, os_strerror(error));


The use of fname here looks like UB, because get_tv_string() uses a static buffer for Number variables.
fname is defined as a const char *, but the buffer content could be changed by the previous call to write_list() if the list contains a Number variable.

1. When calling writefile(list, fname, []) do not show error message twice. 2. Do not allow file name to be overwritten for writefile([1], 2). 3. Do not show “Can’t open file with an empty name” error after error like “using Float as a String” when type of the second argument is not correct. 4. Do not give multiple error messages and still continue for code like `writefile(["test", [], [], [], "tset"])`. Note that to fix 4. ideally I need tv_check_str_or_nr which is currently present in two PRs: neovim#6114 and neovim#5119. I would want to avoid copying this function into a yet another PR. Ref vim/vim#1476.

ZyX-I · 2017-02-16T17:21:45Z

Should be ready now.

ZyX-I · 2017-02-20T20:58:16Z

Ping.

oni-link · 2017-02-22T21:08:04Z

src/nvim/eval.c

-    if (write_list(fd, argvars[0].vval.v_list, binary) == false) {
-      rettv->vval.v_number = -1;
+    if (write_list(fp, argvars[0].vval.v_list, binary)) {
+      rettv->vval.v_number = 0;


This could be too early to signal a successful write. Flushing of the remaining RBuffer could fail in file_free() and writing to the file could be incomplete.

This way success/failure return from this function is more precise.

ZyX-I · 2017-02-25T11:22:42Z

Looks like another random failure, restarting.

[ RUN      ] ...d/neovim/neovim/test/functional/ui/screen_basic_spec.lua @ 565: Screen resize has minimum width/height values
Warning: Screen changes have been received after the expected state was seen.
This is probably due to an indeterminism in the test. Try adding
`wait()` (or even a separate `screen:expect(...)`) at a point of possible
indeterminism, typically in between a `feed()` or `execute()` which is non-
synchronous, and a synchronous api call.
Note that sometimes a `wait` can trigger redraws and consequently generate more
indeterminism. If adding `wait` calls seems to increase the frequency of these
messages, try removing every `wait` call in the test.
If everything else fails, use Screen:redraw_debug to help investigate what is
  causing the problem.
      
stack traceback:
	./test/functional/ui/screen.lua:292: in function 'wait'
	./test/functional/ui/screen.lua:209: in function 'expect'
	...d/neovim/neovim/test/functional/ui/screen_basic_spec.lua:567: in function <...d/neovim/neovim/test/functional/ui/screen_basic_spec.lua:565>
./test/functional/ui/screen.lua:298: Row 1 did not match.
Expected:
  |*{2:-- INS^ERT --}|
  |            |
Actual:
  |*resize^      |
  |            |
To print the expect() call that would assert the current screen state, use
screen:snaphot_util(). In case of non-deterministic failures, use
screen:redraw_debug() to show all intermediate screen states.  
stack traceback:
	./test/functional/ui/screen.lua:298: in function 'wait'
	./test/functional/ui/screen.lua:209: in function 'expect'
	...d/neovim/neovim/test/functional/ui/screen_basic_spec.lua:567: in function <...d/neovim/neovim/test/functional/ui/screen_basic_spec.lua:565>
[  ERROR   ] ...d/neovim/neovim/test/functional/ui/screen_basic_spec.lua @ 565: Screen resize has minimum width/height values (4.85 ms)

ZyX-I · 2017-02-25T22:43:44Z

CI passed, should be ready again now.

justinmk · 2017-02-27T09:18:56Z

test/functional/eval/writefile_spec.lua

+  os.remove(fname)
+  os.remove(dfname)
+  lfs.rmdir(ddname)
+  lfs.rmdir(dname)


helpers.rmdir() is more robust, in particular for the Windows build. (Would allow omitting the os.remove calls above, too)

ZyX-I added 3 commits February 14, 2017 01:09

os/fileio: Support appending to a file

222d983

eval: writefile: Give more adequate IO errors and do not call putc()

fe0eecf

justinmk reviewed Feb 13, 2017

View reviewed changes

eval: Remove outdated comment

2e17921

justinmk reviewed Feb 13, 2017

View reviewed changes

oni-link reviewed Feb 14, 2017

View reviewed changes

eval: Return immediately after an error in write_list()

066e6b8

Previously it could attempt to write trailing newline before returning.

oni-link reviewed Feb 14, 2017

View reviewed changes

ZyX-I added 2 commits February 14, 2017 20:46

eval: Fix linter error

0086991

ZyX-I changed the title ~~Refactor writefile() and create more tests for it~~ [RDY] Refactor writefile() and create more tests for it Feb 16, 2017

marvim added the RDY label Feb 16, 2017

oni-link reviewed Feb 22, 2017

View reviewed changes

eval: Flush buffer in write_list

31cdb22

This way success/failure return from this function is more precise.

justinmk reviewed Feb 27, 2017

View reviewed changes

justinmk merged commit 8c8ce18 into neovim:master Feb 27, 2017

justinmk removed the RDY label Feb 27, 2017

ZyX-I deleted the split-eval'/os-fileio branch February 27, 2017 12:19

justinmk mentioned this pull request Jun 22, 2018

[RDY] vim-patch:8.0.0642 #8621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] Refactor writefile() and create more tests for it #6111

[RDY] Refactor writefile() and create more tests for it #6111

ZyX-I commented Feb 13, 2017 •

edited

justinmk Feb 13, 2017

justinmk Feb 13, 2017

ZyX-I Feb 13, 2017

oni-link Feb 14, 2017

oni-link Feb 14, 2017

ZyX-I commented Feb 16, 2017

ZyX-I commented Feb 20, 2017

oni-link Feb 22, 2017

ZyX-I commented Feb 25, 2017

ZyX-I commented Feb 25, 2017

justinmk Feb 27, 2017 •

edited

		@@ -18153,17 +18178,25 @@ static void f_writefile(typval_T argvars, typval_T rettv, FunPtr fptr)

		// Always open the file in binary mode, library functions have a mind of
		// their own about CR-LF conversion.

[RDY] Refactor writefile() and create more tests for it #6111

[RDY] Refactor writefile() and create more tests for it #6111

Conversation

ZyX-I commented Feb 13, 2017 • edited

justinmk Feb 13, 2017

Choose a reason for hiding this comment

justinmk Feb 13, 2017

Choose a reason for hiding this comment

ZyX-I Feb 13, 2017

Choose a reason for hiding this comment

oni-link Feb 14, 2017

Choose a reason for hiding this comment

oni-link Feb 14, 2017

Choose a reason for hiding this comment

ZyX-I commented Feb 16, 2017

ZyX-I commented Feb 20, 2017

oni-link Feb 22, 2017

Choose a reason for hiding this comment

ZyX-I commented Feb 25, 2017

ZyX-I commented Feb 25, 2017

justinmk Feb 27, 2017 • edited

Choose a reason for hiding this comment

ZyX-I commented Feb 13, 2017 •

edited

justinmk Feb 27, 2017 •

edited