use job for s:system() on vim8 #594

mattn · 2017-02-15T12:25:04Z

Vim dispose jobs in main thread. So vim stop job operations while calling system(). This use job in s:system().

Left is without this patch. Right is with this patch.

junegunn · 2017-02-16T04:55:10Z

Thanks, it looks interesting. But there are places we refer to v:shell_error after s:system call to see if the call was successful but job_start, understandably, doesn't update the value. And I believe that's the reason the test cases are failing.

e.g. :call system('xxx') | PlugUpdate.

Also I wonder why bumping up maxfuncdepth is necessary. I can confirm that if it's not increased Vim gives errors when running :PlugUpdate99, but I still don't see why Vim complains when there are no recursive calls.

mattn · 2017-02-16T05:18:37Z

I will add code for v:shell_error. About maxfuncdepth, job_status catch-up another running jobs. So when another job make the error, exception will be raised at the line at job_status. The effect of this change, muliple jobs can be spawned in same time. So we need to grow-up maxfuncdepth.

junegunn · 2017-02-17T01:35:00Z

I think we should stop using v:shell_error. We can make s:system() return [result, error] pair.

mattn · 2017-02-17T02:38:07Z

yes, we have better to separate s:system and s:systm_with_error? as far as i see, s:system is used in many part like let foo = s:some_func(s:system(cmd)).

mattn · 2017-02-21T04:26:20Z

added s:system_with_error which return two values.

junegunn · 2017-02-21T09:19:38Z

plug.vim

@@ -1979,34 +1982,70 @@ endfunction

 function! s:system(cmd, ...)


s:system can be simplified to s:system_with_error(...)[0].

junegunn · 2017-02-21T09:26:25Z

plug.vim

-              \ a:spec.branch), a:spec.dir)), '\t')
-        if !v:shell_error && ahead
+              \ a:spec.branch), a:spec.dir)
+        let [ahead, behind] = split(s:lastline(output), '\t')


Hmm, this can fail if output does not have \t because of an error, right? It was my oversight.

junegunn · 2017-02-21T09:28:49Z

plug.vim

@@ -1053,14 +1053,16 @@ function! s:update_finish()
      if !pos
        continue
      endif
+      let shellerror = 0
      if has_key(spec, 'commit')
        call s:log4(name, 'Checking out '.spec.commit)
        let out = s:checkout(spec)


s:checkout can fail. It used to set v:shell_error which is tested at line 1079, but now we have to update shellerror. The same goes for line 1072 and 1076.

junegunn · 2017-02-22T03:38:37Z

plug.vim

+              \ a:spec.branch), a:spec.dir)
+        let [ahead; behind] = split(s:lastline(output), '\t')
+        if !shellerror && ahead
+          if len(behind) > 0


behind is a list, so line 2060 should be updated accordingly.

Do you want the behind is joined with " " ?

The original code expected exactly two numbers, which is a correct assumption given that git rev-list --count --left-right HEAD...origin/master works as expected. I think behind[0] should suffice. What do you think?

I think your's is best. will fix soon.

Maybe we should test len(behind) == 1 instead?

EDIT: 1

junegunn · 2017-02-22T04:15:41Z

plug.vim

        if !shellerror && ahead
-          if behind
+          if len(behind) == 1


Ah, sorry, one more thing, I think we should change these lines to

if !shellerror && ahead && len(behind) == 1 if behind[0]

junegunn · 2017-02-22T05:30:00Z

plug.vim

        if !shellerror && ahead
-          if behind
+          if !shellerror && ahead && len(behind) == 1


Sorry if I wasn't being clear. What I meant is to change line 2054 and 2055 as follows:

if !shellerror && ahead && len(behind) == 1 if behind[0]

First if statement checks if the output of the command conforms to the expected format

Second if statement checks if behind[0] is a non-zero value.

junegunn · 2017-02-22T15:28:09Z

A few test cases are still failing on Vim 8:

https://s3.amazonaws.com/archive.travis-ci.org/jobs/204097914/log.txt

Looks like the lines in the output of the new system function are not properly separated by newline characters ( ... new_branch_nameHEAD is now at ...), any idea?

(109/134) [EXECUTE] Commit hash support
> ['Updated. Elapsed time: 0.256267 sec.', '[=x]', '', '- Finishing ... Done!', 'x goyo.vim:', ' error: pathspec ''ffffffff'' did not match any file(s) known to git.', '- vim-emoji: Note: checking out ''9db7fcfee0d90dafdbcb7a32090c0a9085eb054a''.You are in ''detached HEAD'' state. You can look around, make experimentalchanges and commit them, and you can discard any commits you make in thisstate without impacting any branches by performing another checkout.If you want to create a new branch to retain commits you create, you maydo so (now or later) by using -b with the checkout command again. Example: git checkout -b new_branch_nameHEAD is now at 9db7fcf... MIT license (close #2)']

mattn · 2017-02-22T16:56:49Z

Sorry, It's difficult to read the test result for me.

junegunn · 2017-02-23T14:19:06Z

I'll look into those errors when I get some time. By the way, I tested the patched version on my machine.

I can notice the difference when I do PlugUpdate99 that Vim becomes responsive earlier, I can move the cursor before every plugin is queued. However, the total time for the command is more or less the same; 8~10 seconds for 62 plugins with or without the patch. Maybe the difference is more noticeable on Windows?
A subtle issue I noticed is that when Vim becomes responsive, my cursor ends up on a random line. Before the patch, it's always on line 2.

For testing/debugging the issue I mentioned above, I tried commenting out redraw line. Then I noticed that some random errors occur during the update as shown below.

use job for s:system() on vim8

90ad034

add system_with_error for handling v:shell_error using job

ed4c861

junegunn requested changes Feb 21, 2017

View reviewed changes

now s:system() return two values instead of s:system_with_error()

5b2a6e1

junegunn requested changes Feb 22, 2017

View reviewed changes

mattn force-pushed the use-job-system branch 2 times, most recently from b3e794b to 352cc75 Compare February 22, 2017 04:02

junegunn requested changes Feb 22, 2017

View reviewed changes

mattn force-pushed the use-job-system branch from 352cc75 to 5e5bfac Compare February 22, 2017 04:50

junegunn requested changes Feb 22, 2017

View reviewed changes

avoid error when string not have \t

7dd874e

mattn force-pushed the use-job-system branch from 5e5bfac to 7dd874e Compare February 22, 2017 07:40

set err_mode

7f7d829

This was referenced Aug 11, 2017

Prefer Python over Vim 8 jobs in Windows #666

Closed

Check patch-8.0.87 for vim 8 job #669

Closed

janlazo mentioned this pull request Dec 11, 2017

Flick when invoke some commands junegunn/fzf.vim#543

Open

3 tasks

janlazo mentioned this pull request Nov 30, 2019

Don't use chcp. #913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use job for s:system() on vim8 #594

use job for s:system() on vim8 #594

mattn commented Feb 15, 2017

junegunn commented Feb 16, 2017

mattn commented Feb 16, 2017

junegunn commented Feb 17, 2017

mattn commented Feb 17, 2017

mattn commented Feb 21, 2017

junegunn Feb 21, 2017

junegunn Feb 21, 2017

junegunn Feb 21, 2017

junegunn Feb 22, 2017

mattn Feb 22, 2017 •

edited

Loading

junegunn Feb 22, 2017 •

edited

Loading

mattn Feb 22, 2017

junegunn Feb 22, 2017 •

edited

Loading

mattn Feb 22, 2017

junegunn Feb 22, 2017

junegunn Feb 22, 2017

mattn Feb 22, 2017

junegunn commented Feb 22, 2017 •

edited

Loading

mattn commented Feb 22, 2017

junegunn commented Feb 23, 2017 •

edited

Loading

		@@ -1979,34 +1982,70 @@ endfunction

		function! s:system(cmd, ...)

use job for s:system() on vim8 #594

Are you sure you want to change the base?

use job for s:system() on vim8 #594

Conversation

mattn commented Feb 15, 2017

junegunn commented Feb 16, 2017

mattn commented Feb 16, 2017

junegunn commented Feb 17, 2017

mattn commented Feb 17, 2017

mattn commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattn Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

junegunn Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junegunn Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junegunn commented Feb 22, 2017 • edited Loading

mattn commented Feb 22, 2017

junegunn commented Feb 23, 2017 • edited Loading

mattn Feb 22, 2017 •

edited

Loading

junegunn Feb 22, 2017 •

edited

Loading

junegunn Feb 22, 2017 •

edited

Loading

junegunn commented Feb 22, 2017 •

edited

Loading

junegunn commented Feb 23, 2017 •

edited

Loading