Skip to content
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.

Sign up for GitHub

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

Jump to bottom

eval: save_tv_as_string: Correctly handle an empty string #6558

Merged
merged 1 commit into from Jan 25, 2018
Merged

eval: save_tv_as_string: Correctly handle an empty string #6558

merged 1 commit into from Jan 25, 2018

Conversation

jamessan
Copy link
Member

When tv_get_string_chk returns a non-NULL value, we have a valid string.
Propagating an error state (*len = -1, NULL return) for an empty string
is invalid.

Closes #6554

@jamessan jamessan mentioned this pull request Apr 20, 2017
Closed
@jamessan jamessan added this to the 0.2.1 milestone Apr 20, 2017
@jamessan jamessan force-pushed the tv_as_string-fix branch 3 times, most recently from 6426e40 to 03a0be3 Compare April 21, 2017 01:03
@jamessan
Copy link
Member Author

@ZyX-I Any idea why the test is sometimes failing in Travis like this:

[ RUN      ] system() passing input works with an empty string: ERR
./test/functional/helpers.lua:93: Failed to evaluate expression
stack traceback:
	./test/functional/helpers.lua:93: in function 'eval'
	...build/neovim/neovim/test/functional/eval/system_spec.lua:258: in function <...build/neovim/neovim/test/functional/eval/system_spec.lua:257>

I've been running system_spec.lua in a loop on my VM here and haven't had any issues so far. In my previous version of the PR, I tested cherry-picking the commit onto code before the big typval refactor and it seemed to work fine there.

@jamessan
Copy link
Member Author

Hmm, after clearing Travis' cache, everything ran fine except for the flaky timers tests.

@justinmk
Copy link
Member

Let's re-run it a few times...

@jamessan
Copy link
Member Author

Ok, it happened again in the coverage build.

There was also this stderr output reported at the end of the run:

-- Output to stderr:
2017/04/28 16:34:19 ERROR 10786/open_log_file:127: Couldn't open USR_LOG_FILE, logging to stderr! This may be caused by attempting to LOG() before initialization functions are called (e.g. init_homedir()).
2017/04/28 16:34:19 ERROR 10786/call_set_error:723: RPC: ch 1 was closed by the client
2017/04/28 16:34:41 ERROR 11266/open_log_file:127: Couldn't open USR_LOG_FILE, logging to stderr! This may be caused by attempting to LOG() before initialization functions are called (e.g. init_homedir()).
2017/04/28 16:34:41 ERROR 11266/call_set_error:723: RPC: ch 1 was closed by the client

justinmk
justinmk reviewed Apr 28, 2017
View reviewed changes
src/nvim/eval.c Outdated
@@ -17409,7 +17409,8 @@ static char *save_tv_as_string(typval_T *tv, ptrdiff_t *const len, bool endnl)
// print an error.
if (tv->v_type != VAR_LIST) {
const char *ret = tv_get_string_chk(tv);
if (ret && (*len = strlen(ret))) {
if (ret) {
*len = strlen(ret);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth temporarily reverting this change to see if the problem still occurs with the new test. To avoid having to look at a bunch of failed builds, change the expect result to what happens on master.

eq("", eval('system("echo test", "")'))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No lua tracebacks yet with the code reverted...

justinmk
justinmk reviewed Apr 28, 2017
View reviewed changes
test/functional/eval/system_spec.lua
@@ -255,7 +255,7 @@ describe('system()', function()
eq(2, eval("1+1")) -- Still alive?
Copy link
Member

@justinmk justinmk Apr 28, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off topic, just noticed the test on lin 250 should be:

feed_command('call system("cat - &", "input")')

Edit: fixed in 15119f9

@jamessan jamessan changed the title eval: save_tv_as_string: Correctly handle an empty string [WIP] eval: save_tv_as_string: Correctly handle an empty string May 3, 2017
@marvim marvim added the WIP label May 3, 2017
@justinmk justinmk modified the milestones: 0.2.1, 0.2.2 Sep 16, 2017
@jamessan
eval: save_tv_as_string: Correctly handle an empty string
3ff92ba 
When tv_get_string_chk returns a non-NULL value, we have a valid string.
Propagating an error state (*len = -1, NULL return) for an empty string
is invalid.

Closes neovim#6554
@jamessan jamessan changed the title [WIP] eval: save_tv_as_string: Correctly handle an empty string eval: save_tv_as_string: Correctly handle an empty string Jan 24, 2018
@jamessan
Copy link
Member Author

I've run the Travis builds a few times now and none of them have failed. Looking better now.

@jamessan jamessan merged commit 83880cc into neovim:master Jan 25, 2018
@jamessan jamessan deleted the tv_as_string-fix branch January 25, 2018 19:09
@jszakmeister jszakmeister removed the WIP label Jan 25, 2018
@justinmk
Copy link
Member

Hmm, saw this in #7924 :

system() passing input works with an empty string: �[1m�[31mERR�[0m�[0m
test/functional/helpers.lua:93: Failed to evaluate expression

stack traceback:
	test/functional/helpers.lua:93: in function 'eval'
	...build/neovim/neovim/test/functional/eval/system_spec.lua:262: in function <...build/neovim/neovim/test/functional/eval/system_spec.lua:261>

@justinmk
Copy link
Member

justinmk commented Jan 28, 2018 •
edited

I can repro it locally, will investigate. #7951

justinmk added a commit to justinmk/neovim that referenced this pull request Feb 1, 2018
@justinmk
os_system(): do not set up input stream for empty string
458da4c 
Test failure:
test/functional/eval/system_spec.lua: "works with an empty string"
E5677: Error writing input to shell-command: EPIPE

ref neovim#6558 (comment)
@justinmk justinmk mentioned this pull request Feb 1, 2018
Merged
justinmk added a commit to justinmk/neovim that referenced this pull request Feb 1, 2018
@justinmk
os_system(): do not set up input stream for empty string neovim#7951
44ed6b0 
Test failure:
test/functional/eval/system_spec.lua: "works with an empty string"
E5677: Error writing input to shell-command: EPIPE

ref neovim#6558 (comment)
ref neovim#6554
justinmk added a commit to justinmk/neovim that referenced this pull request Feb 1, 2018
@justinmk
os_system(): do not set up input stream for empty string neovim#7951
648fed9 
Test failure:
test/functional/eval/system_spec.lua: "works with an empty string"
E5677: Error writing input to shell-command: EPIPE

ref neovim#6558 (comment)
ref neovim#6554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmk justinmk justinmk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.3.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jamessan @justinmk @jszakmeister @marvim