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 4 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,53 @@ 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) | ||
{ | ||
for (const listitem_T *li = list->lv_first; li != NULL; li = li->li_next) { | ||
int error = 0; | ||
const char *const s = (const char *)get_tv_string((typval_T *)&li->li_tv); | ||
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; | ||
break; | ||
} | ||
} | ||
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; | ||
} | ||
} | ||
if (ret == false) { | ||
EMSG(_(e_write)); | ||
break; | ||
if (error != 0) { | ||
emsgf(_("E80: Error while writing: %s"), os_strerror(error)); | ||
return false; | ||
} | ||
} | ||
return ret; | ||
return true; | ||
} | ||
|
||
/// Saves a typval_T as a string. | ||
|
@@ -18151,19 +18176,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. | ||
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 *const fname = (const char *)get_tv_string(&argvars[1]); | ||
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,93 @@ | ||
local helpers = require('test.functional.helpers')(after_each) | ||
|
||
local clear = helpers.clear | ||
local eq = helpers.eq | ||
local funcs = helpers.funcs | ||
local exc_exec = helpers.exc_exec | ||
local read_file = helpers.read_file | ||
|
||
local fname = 'Xtest-functional-eval-writefile' | ||
|
||
before_each(clear) | ||
after_each(function() os.remove(fname) 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('errors out with invalid arguments', function() | ||
eq('Vim(call):E119: Not enough arguments for function: writefile', | ||
exc_exec('call writefile()')) | ||
eq('Vim(call):E119: Not enough arguments for function: writefile', | ||
exc_exec('call writefile([])')) | ||
eq('Vim(call):E118: Too many arguments for function: writefile', | ||
exc_exec(('call writefile([], "%s", "b", 1)'):format(fname))) | ||
for _, arg in ipairs({'0', '0.0', 'function("tr")', '{}', '"test"'}) do | ||
eq('Vim(call):E686: Argument of writefile() must be a List', | ||
exc_exec(('call writefile(%s, "%s", "b")'):format(arg, fname))) | ||
end | ||
for _, args in ipairs({'%s, "b"', '"' .. fname .. '", %s'}) do | ||
eq('Vim(call):E806: using Float as a String', | ||
exc_exec(('call writefile([], %s)'):format(args:format('0.0')))) | ||
eq('Vim(call):E730: using List as a String', | ||
exc_exec(('call writefile([], %s)'):format(args:format('[]')))) | ||
eq('Vim(call):E731: using Dictionary as a String', | ||
exc_exec(('call writefile([], %s)'):format(args:format('{}')))) | ||
eq('Vim(call):E729: using Funcref as a String', | ||
exc_exec(('call writefile([], %s)'):format(args:format('function("tr")')))) | ||
end | ||
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.