Fix systemlist() #1135

thinca · 2016-09-29T16:39:01Z

:help systemlist() says:

Output is the same as readfile() will output with {binary} argument
set to "b".

And :help readfile() says:

When {binary} contains "b" binary mode is used:

When the last line ends in a NL an extra empty list item is
added.

No CR characters are removed.

systemlist() doesn't satisfy the first matter.

:echo systemlist('echo hello')
" => ['hello']
" `echo` command outputs text with NL,
" so this should be ['hello', '']

This change fixes the above problem.

vim-ml · 2016-09-30T01:02:29Z

Hi thinca,

2016-9-30(Fri) 1:39:34 UTC+9 thinca:

:help systemlist() says:

Output is the same as readfile() will output with {binary} argument

set to "b".

And :help readfile() says:

When {binary} contains "b" binary mode is used:

When the last line ends in a NL an extra empty list item is
added.
No CR characters are removed.

systemlist() doesn't satisfy the first matter.

:echo systemlist('echo hello')
" => ['hello']
" echo command outputs text with NL,
" so this should be ['hello', '']

This change fixes the above problem.

You can view, comment on, or merge this pull request online at:

#1135

Commit Summary

Fix systemlist()

File Changes
M
src/Makefile
(1)


M
src/evalfunc.c
(2)


A
src/testdir/test_system_func.vim
(20)
Patch Links:

https://github.com/vim/vim/pull/1135.patch
https://github.com/vim/vim/pull/1135.diff

Good patch👍
Unfortunately, test that your have to add is not running in the Travis CI and AppVeyor.
You should modified as follows:
diff -ru vim.orig/src/testdir/Make_all.mak vim/src/testdir/Make_all.mak
--- vim.orig/src/testdir/Make_all.mak 2016-09-29 11:54:58.000000000 +0900
+++ vim/src/testdir/Make_all.mak 2016-09-30 09:26:47.771610300 +0900
@@ -182,6 +182,7 @@
test_stat.res
test_substitute.res
test_syntax.res \

  test_system_func.res \
test_textobjects.res \
test_undo.res \
test_usercommands.res \

If you want to run from test_alot.vim as follows:
diff -ru vim.orig/src/testdir/test_alot.vim vim/src/testdir/test_alot.vim
--- vim.orig/src/testdir/test_alot.vim 2016-09-29 11:54:58.000000000 +0900
+++ vim/src/testdir/test_alot.vim 2016-09-30 10:00:40.479191900 +0900
@@ -41,6 +41,7 @@
source test_tagjump.vim
source test_timers.vim
source test_true_false.vim
+source test_system_func.vim
source test_unlet.vim
source test_window_cmd.vim
source test_options.vim

Best regards,
Hirohito Higashi (a.k.a. h_east)

thinca · 2016-09-30T03:01:27Z

@h-east Thank you for your advice!
I updated the patch.

vim-ml · 2016-09-30T03:12:49Z

Hi thinca,

2016-9-30(Fri) 12:01:55 UTC+9 thinca:

@h-east Thank you for your advice!

I updated the patch.

You need select one of the two patches.
I think the first one is good.

diff -ru vim.orig/src/testdir/Make_all.mak vim/src/testdir/Make_all.mak
--- vim.orig/src/testdir/Make_all.mak 2016-09-29 11:54:58.000000000 +0900
+++ vim/src/testdir/Make_all.mak 2016-09-30 09:26:47.771610300 +0900
@@ -182,6 +182,7 @@
test_stat.res
test_substitute.res
test_syntax.res \

test_system_func.res
test_textobjects.res
test_undo.res
test_usercommands.res \

If you want to run from test_alot.vim as follows:
diff -ru vim.orig/src/testdir/test_alot.vim vim/src/testdir/test_alot.vim
--- vim.orig/src/testdir/test_alot.vim 2016-09-29 11:54:58.000000000 +0900
+++ vim/src/testdir/test_alot.vim 2016-09-30 10:00:40.479191900 +0900
@@ -41,6 +41,7 @@
source test_tagjump.vim
source test_timers.vim
source test_true_false.vim
+source test_system_func.vim
source test_unlet.vim
source test_window_cmd.vim
source test_options.vim

Best regards,
Hirohito Higashi (a.k.a. h_east)

thinca · 2016-09-30T03:42:37Z

@h-east oops, sorry and thanks!
And, Windows test failed. I'll check and fix it later.

:help systemlist() says: > Output is the same as readfile() will output with {binary} argument > set to "b". And :help readfile() says: > When {binary} contains "b" binary mode is used: > - When the last line ends in a NL an extra empty list item is > added. > - No CR characters are removed. systemlist() doesn't satisfy the first matter. ```vim :echo systemlist('echo hello') " => ['hello'] " `echo` command outputs text with NL, " so this should be ['hello', ''] ``` This change fixes the above problem.

thinca · 2016-09-30T07:24:08Z

I updated the patch, and all tests passed.

thinca · 2016-10-01T18:29:40Z

This is todolisted.

2ec618c#diff-38d7929bd26d74d92ceddf984bbfc8dbR152

Patch for systemlist(), add empty item. (thinca, Sep 30, #1135)
Adjust the documentation instead? Do include the test.

Currently, we can't know the result of command ends with NL.

echo systemlist('echo hello')
" => ['hello']
echo systemlist('echo -n hello')
" => ['hello']

Therefore I think this change is needed.

lcd047 · 2016-10-01T19:24:37Z

Currently, we can't know the result of command ends with NL.

Sadly, your patch won't change that:

echo systemlist('echo hello; echo')
" => ['hello', '']

ZyX-I · 2016-10-01T19:44:55Z

I would suggest to copy encode_list_write from Neovim:

/// Msgpack callback for writing to readfile()-style list
int encode_list_write(void *const data, const char *const buf, const size_t len)
  FUNC_ATTR_NONNULL_ARG(1)
{
  if (len == 0) {
    return 0;
  }
  list_T *const list = (list_T *) data;
  const char *const end = buf + len;
  const char *line_end = buf;
  listitem_T *li = list->lv_last;

  // Continue the last list element
  if (li != NULL) {
    line_end = xmemscan(buf, NL, len);
    if (line_end != buf) {
      const size_t line_length = (size_t)(line_end - buf);
      char *str = (char *)li->li_tv.vval.v_string;
      const size_t li_len = (str == NULL ? 0 : strlen(str));
      li->li_tv.vval.v_string = xrealloc(str, li_len + line_length + 1);
      str = (char *)li->li_tv.vval.v_string + li_len;
      memcpy(str, buf, line_length);
      str[line_length] = 0;
      memchrsub(str, NUL, NL, line_length);
    }
    line_end++;
  }

  while (line_end < end) {
    const char *line_start = line_end;
    line_end = xmemscan(line_start, NL, (size_t) (end - line_start));
    char *str = NULL;
    if (line_end != line_start) {
      const size_t line_length = (size_t)(line_end - line_start);
      str = xmemdupz(line_start, line_length);
      memchrsub(str, NUL, NL, line_length);
    }
    tv_list_append_allocated_string(list, str);
    line_end++;
  }
  if (line_end == end) {
    tv_list_append_allocated_string(list, NULL);
  }
  return 0;
}

This function was tested also with input like \n\n\n and is used directly in some cases.

brammool · 2016-10-01T20:57:34Z

thinca wrote:

This is todolisted.

2ec618c#diff-38d7929bd26d74d92ceddf984bbfc8dbR152

Patch for systemlist(), add empty item. (thinca, Sep 30, #1135)
Adjust the documentation instead? Do include the test.

Currently, we can't know the result of command ends with NL.
echo systemlist('echo hello')
" => ['hello']
echo systemlist('echo -n hello')
" => ['hello']
Therefore I think this change is needed.

I can't think of a situation where it matters. Shell output is normally
line-by-line. Whether the last line as a NL or not does not seem to
make a difference for what you would want to do with a shell command.

ARTHUR: Shut up! Will you shut up!
DENNIS: Ah, now we see the violence inherent in the system.
ARTHUR: Shut up!
DENNIS: Oh! Come and see the violence inherent in the system!
HELP! HELP! I'm being repressed!
The Quest for the Holy Grail (Monty Python)

/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \
\ an exciting new programming language -- http://www.Zimbu.org ///
\ help me help AIDS victims -- http://ICCF-Holland.org ///

ZyX-I · 2016-10-01T21:30:21Z

@brammool This is essential for things like using the output of git cat-file. Especially if you are obtaining binary file this way. Also for other commands which may return binary output: curl, wget, …

brammool · 2016-10-02T12:49:44Z

Nikolai Pavlov wrote:

@brammool This is essential for things like using the output of git cat-file. Especially if you are obtaining binary file this way.
Also for other commands which may return binary output: curl,
wget, …

I think it's rare to read a binary file. We can make it possible, but
I don't think it should be the default behavior.

Also, compared to readfile() there is no way to handle the input as
not-binary. Ignoring CR characters and BOM is clumsy.

Also, if we change this now it's not backwards compatible. The extra
emtpy list item at the end is unexpected. Anybody just getting the last
item will run into this.

I think we should add an argument that indicates binary or not-binary,
and when omitted falls back to the current behavior.

"Hegel was right when he said that we learn from history that man can
never learn anything from history." (George Bernard Shaw)

/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \
\ an exciting new programming language -- http://www.Zimbu.org ///
\ help me help AIDS victims -- http://ICCF-Holland.org ///

brammool · 2019-09-27T17:07:13Z

Isn't it possible to do the same with system('cmd')->split('\n', 1) ?

brammool · 2020-06-10T20:33:22Z

Closing this, because the solution is not backwards compatible.

thinca mentioned this pull request Sep 29, 2016

コマンドの出力末尾が改行の場合の systemlist() の結果がおかしい vim-jp/issues#956

Closed

thinca force-pushed the fix-systemlist branch from b510aee to 3f9c1ba Compare September 30, 2016 02:58

thinca force-pushed the fix-systemlist branch from 3f9c1ba to 5b2d022 Compare September 30, 2016 06:52

brammool closed this Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix systemlist() #1135

Fix systemlist() #1135

thinca commented Sep 29, 2016

vim-ml commented Sep 30, 2016

thinca commented Sep 30, 2016

vim-ml commented Sep 30, 2016

thinca commented Sep 30, 2016

thinca commented Sep 30, 2016

thinca commented Oct 1, 2016

lcd047 commented Oct 1, 2016

ZyX-I commented Oct 1, 2016

brammool commented Oct 1, 2016

ZyX-I commented Oct 1, 2016

brammool commented Oct 2, 2016

brammool commented Sep 27, 2019

brammool commented Jun 10, 2020

Fix systemlist() #1135

Fix systemlist() #1135

Conversation

thinca commented Sep 29, 2016

vim-ml commented Sep 30, 2016

thinca commented Sep 30, 2016

vim-ml commented Sep 30, 2016

thinca commented Sep 30, 2016

thinca commented Sep 30, 2016

thinca commented Oct 1, 2016

lcd047 commented Oct 1, 2016

ZyX-I commented Oct 1, 2016

brammool commented Oct 1, 2016

ZyX-I commented Oct 1, 2016

brammool commented Oct 2, 2016

brammool commented Sep 27, 2019

brammool commented Jun 10, 2020