New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RDY] Refactor writefile() and create more tests for it #6111
Changes from all commits
85e1a56
222d983
fe0eecf
2e17921
066e6b8
f489827
0086991
31cdb22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
#include "nvim/ex_eval.h" | ||
#include "nvim/ex_getln.h" | ||
#include "nvim/fileio.h" | ||
#include "nvim/os/fileio.h" | ||
#include "nvim/func_attr.h" | ||
#include "nvim/fold.h" | ||
#include "nvim/getchar.h" | ||
|
@@ -18021,29 +18022,60 @@ static void f_winsaveview(typval_T *argvars, typval_T *rettv, FunPtr fptr) | |
} | ||
|
||
/// Writes list of strings to file | ||
static bool write_list(FILE *fd, list_T *list, bool binary) | ||
{ | ||
int ret = true; | ||
|
||
for (listitem_T *li = list->lv_first; li != NULL; li = li->li_next) { | ||
for (char_u *s = get_tv_string(&li->li_tv); *s != NUL; ++s) { | ||
if (putc(*s == '\n' ? NUL : *s, fd) == EOF) { | ||
ret = false; | ||
break; | ||
/// | ||
/// @param fp File to write to. | ||
/// @param[in] list List to write. | ||
/// @param[in] binary Whether to write in binary mode. | ||
/// | ||
/// @return true in case of success, false otherwise. | ||
static bool write_list(FileDescriptor *const fp, const list_T *const list, | ||
const bool binary) | ||
{ | ||
int error = 0; | ||
for (const listitem_T *li = list->lv_first; li != NULL; li = li->li_next) { | ||
const char *const s = (const char *)get_tv_string_chk( | ||
(typval_T *)&li->li_tv); | ||
if (s == NULL) { | ||
return false; | ||
} | ||
const char *hunk_start = s; | ||
for (const char *p = hunk_start;; p++) { | ||
if (*p == NUL || *p == NL) { | ||
if (p != hunk_start) { | ||
const ptrdiff_t written = file_write(fp, hunk_start, | ||
(size_t)(p - hunk_start)); | ||
if (written < 0) { | ||
error = (int)written; | ||
goto write_list_error; | ||
} | ||
} | ||
if (*p == NUL) { | ||
break; | ||
} else { | ||
hunk_start = p + 1; | ||
const ptrdiff_t written = file_write(fp, (char[]){ NUL }, 1); | ||
if (written < 0) { | ||
error = (int)written; | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
if (!binary || li->li_next != NULL) { | ||
if (putc('\n', fd) == EOF) { | ||
ret = false; | ||
break; | ||
const ptrdiff_t written = file_write(fp, "\n", 1); | ||
if (written < 0) { | ||
error = (int)written; | ||
goto write_list_error; | ||
} | ||
} | ||
if (ret == false) { | ||
EMSG(_(e_write)); | ||
break; | ||
} | ||
} | ||
return ret; | ||
if ((error = file_fsync(fp)) != 0) { | ||
goto write_list_error; | ||
} | ||
return true; | ||
write_list_error: | ||
emsgf(_("E80: Error while writing: %s"), os_strerror(error)); | ||
return false; | ||
} | ||
|
||
/// Saves a typval_T as a string. | ||
|
@@ -18143,27 +18175,42 @@ static void f_writefile(typval_T *argvars, typval_T *rettv, FunPtr fptr) | |
bool binary = false; | ||
bool append = false; | ||
if (argvars[2].v_type != VAR_UNKNOWN) { | ||
if (vim_strchr(get_tv_string(&argvars[2]), 'b')) { | ||
const char *const flags = (const char *)get_tv_string_chk(&argvars[2]); | ||
if (flags == NULL) { | ||
return; | ||
} | ||
if (strchr(flags, 'b')) { | ||
binary = true; | ||
} | ||
if (vim_strchr(get_tv_string(&argvars[2]), 'a')) { | ||
if (strchr(flags, 'a')) { | ||
append = true; | ||
} | ||
} | ||
|
||
// Always open the file in binary mode, library functions have a mind of | ||
// their own about CR-LF conversion. | ||
char_u *fname = get_tv_string(&argvars[1]); | ||
FILE *fd; | ||
if (*fname == NUL || (fd = mch_fopen((char *)fname, | ||
append ? APPENDBIN : WRITEBIN)) == NULL) { | ||
EMSG2(_(e_notcreate), *fname == NUL ? (char_u *)_("<empty>") : fname); | ||
rettv->vval.v_number = -1; | ||
const char buf[NUMBUFLEN]; | ||
const char *const fname = (const char *)get_tv_string_buf_chk(&argvars[1], | ||
(char_u *)buf); | ||
if (fname == NULL) { | ||
return; | ||
} | ||
FileDescriptor *fp; | ||
int error; | ||
rettv->vval.v_number = -1; | ||
if (*fname == NUL) { | ||
EMSG(_("E482: Can't open file with an empty name")); | ||
} else if ((fp = file_open_new(&error, fname, | ||
((append ? kFileAppend : kFileTruncate) | ||
| kFileCreate), 0666)) == NULL) { | ||
emsgf(_("E482: Can't open file %s for writing: %s"), | ||
fname, os_strerror(error)); | ||
} else { | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be too early to signal a successful write. Flushing of the remaining |
||
} | ||
if ((error = file_free(fp)) != 0) { | ||
emsgf(_("E80: Error when closing file %s: %s"), | ||
fname, os_strerror(error)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of |
||
} | ||
fclose(fd); | ||
} | ||
} | ||
/* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
local helpers = require('test.functional.helpers')(after_each) | ||
local lfs = require('lfs') | ||
|
||
local clear = helpers.clear | ||
local eq = helpers.eq | ||
local funcs = helpers.funcs | ||
local meths = helpers.meths | ||
local exc_exec = helpers.exc_exec | ||
local read_file = helpers.read_file | ||
local write_file = helpers.write_file | ||
local redir_exec = helpers.redir_exec | ||
|
||
local fname = 'Xtest-functional-eval-writefile' | ||
local dname = fname .. '.d' | ||
local dfname_tail = '1' | ||
local dfname = dname .. '/' .. dfname_tail | ||
local ddname_tail = '2' | ||
local ddname = dname .. '/' .. ddname_tail | ||
|
||
before_each(function() | ||
lfs.mkdir(dname) | ||
lfs.mkdir(ddname) | ||
clear() | ||
end) | ||
|
||
after_each(function() | ||
os.remove(fname) | ||
os.remove(dfname) | ||
lfs.rmdir(ddname) | ||
lfs.rmdir(dname) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. helpers.rmdir() is more robust, in particular for the Windows build. (Would allow omitting the os.remove calls above, too) |
||
end) | ||
|
||
describe('writefile()', function() | ||
it('writes empty list to a file', function() | ||
eq(nil, read_file(fname)) | ||
eq(0, funcs.writefile({}, fname)) | ||
eq('', read_file(fname)) | ||
os.remove(fname) | ||
eq(nil, read_file(fname)) | ||
eq(0, funcs.writefile({}, fname, 'b')) | ||
eq('', read_file(fname)) | ||
os.remove(fname) | ||
eq(nil, read_file(fname)) | ||
eq(0, funcs.writefile({}, fname, 'ab')) | ||
eq('', read_file(fname)) | ||
os.remove(fname) | ||
eq(nil, read_file(fname)) | ||
eq(0, funcs.writefile({}, fname, 'a')) | ||
eq('', read_file(fname)) | ||
end) | ||
|
||
it('writes list with an empty string to a file', function() | ||
eq(0, exc_exec( | ||
('call writefile([$XXX_NONEXISTENT_VAR_XXX], "%s", "b")'):format( | ||
fname))) | ||
eq('', read_file(fname)) | ||
eq(0, exc_exec(('call writefile([$XXX_NONEXISTENT_VAR_XXX], "%s")'):format( | ||
fname))) | ||
eq('\n', read_file(fname)) | ||
end) | ||
|
||
it('appends to a file', function() | ||
eq(nil, read_file(fname)) | ||
eq(0, funcs.writefile({'abc', 'def', 'ghi'}, fname)) | ||
eq('abc\ndef\nghi\n', read_file(fname)) | ||
eq(0, funcs.writefile({'jkl'}, fname, 'a')) | ||
eq('abc\ndef\nghi\njkl\n', read_file(fname)) | ||
os.remove(fname) | ||
eq(nil, read_file(fname)) | ||
eq(0, funcs.writefile({'abc', 'def', 'ghi'}, fname, 'b')) | ||
eq('abc\ndef\nghi', read_file(fname)) | ||
eq(0, funcs.writefile({'jkl'}, fname, 'ab')) | ||
eq('abc\ndef\nghijkl', read_file(fname)) | ||
end) | ||
|
||
it('correctly treats NLs', function() | ||
eq(0, funcs.writefile({'\na\nb\n'}, fname, 'b')) | ||
eq('\0a\0b\0', read_file(fname)) | ||
eq(0, funcs.writefile({'a\n\n\nb'}, fname, 'b')) | ||
eq('a\0\0\0b', read_file(fname)) | ||
end) | ||
|
||
it('correctly overwrites file', function() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: "correctly" and "successfully" are redundant in test descriptions, it should be assumed that we (almost) always want correct behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
eq(0, funcs.writefile({'\na\nb\n'}, fname, 'b')) | ||
eq('\0a\0b\0', read_file(fname)) | ||
eq(0, funcs.writefile({'a\n'}, fname, 'b')) | ||
eq('a\0', read_file(fname)) | ||
end) | ||
|
||
it('shows correct file name when supplied numbers', function() | ||
meths.set_current_dir(dname) | ||
eq('\nE482: Can\'t open file 2 for writing: illegal operation on a directory', | ||
redir_exec(('call writefile([42], %s)'):format(ddname_tail))) | ||
end) | ||
|
||
it('errors out with invalid arguments', function() | ||
write_file(fname, 'TEST') | ||
eq('\nE119: Not enough arguments for function: writefile', | ||
redir_exec('call writefile()')) | ||
eq('\nE119: Not enough arguments for function: writefile', | ||
redir_exec('call writefile([])')) | ||
eq('\nE118: Too many arguments for function: writefile', | ||
redir_exec(('call writefile([], "%s", "b", 1)'):format(fname))) | ||
for _, arg in ipairs({'0', '0.0', 'function("tr")', '{}', '"test"'}) do | ||
eq('\nE686: Argument of writefile() must be a List', | ||
redir_exec(('call writefile(%s, "%s", "b")'):format(arg, fname))) | ||
end | ||
for _, args in ipairs({'[], %s, "b"', '[], "' .. fname .. '", %s'}) do | ||
eq('\nE806: using Float as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('0.0')))) | ||
eq('\nE730: using List as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('[]')))) | ||
eq('\nE731: using Dictionary as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('{}')))) | ||
eq('\nE729: using Funcref as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('function("tr")')))) | ||
end | ||
eq('TEST', read_file(fname)) | ||
end) | ||
|
||
it('stops writing to file after error in list', function() | ||
local args = '["tset"] + repeat([%s], 3), "' .. fname .. '"' | ||
eq('\nE806: using Float as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('0.0')))) | ||
eq('tset\n', read_file(fname)) | ||
write_file(fname, 'TEST') | ||
eq('\nE730: using List as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('[]')))) | ||
eq('tset\n', read_file(fname)) | ||
write_file(fname, 'TEST') | ||
eq('\nE731: using Dictionary as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('{}')))) | ||
eq('tset\n', read_file(fname)) | ||
write_file(fname, 'TEST') | ||
eq('\nE729: using Funcref as a String', | ||
redir_exec(('call writefile(%s)'):format(args:format('function("tr")')))) | ||
eq('tset\n', read_file(fname)) | ||
write_file(fname, 'TEST') | ||
end) | ||
end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A previous error could be overwritten.