How ordinary Vim user contributed to Vim

VimConf 2017

Transcript

1. How ordinary Vim user contributed to Vim VimConf 2017 daisuzu
2. • Daisuke Suzuki ◦ https://github.com/daisuzu ◦ id:daisuzu ◦ @dice_zu • Vim activities ◦ Create plugins ▪ Not really famous ones ◦ Writing articles for publishing ▪ Software Design (July 2012 and October 2013) ◦ Send patches ▪ 3 in 2016 and 2017; I became a contributor About me
3. Introduction Don't you think that it is difficult to send patches to Vim? • Skills ◦ That obscure source code written in C is called the Demon World • Manners ◦ English ◦ How should I behave at vim_dev? ◦ Hey all pull requests in the Vim repository are closed... • Opportunities ◦ What should I contribute? From these things, you may feel a big hurdle, but, all you need is a great passion that you want to be a contributor
4. Agenda 1. Episodes about my contribution ◦ v8.0.0102 ◦ v8.0.0104 ◦ v8.0.0512 2. Useful techniques ◦ Debugging ◦ Testing ◦ How to send a patch
5. My attributes • My skills ◦ Vim experience: About 10 years ◦ C: Almost college level ◦ English: Thank you, @ujm • My attitude toward OSS ◦ Sending lots pull requests aggressively?: No ◦ Like to discuss at many issues?: No ◦ When I find something wrong, backing up by operation?: Yes
6. Episode of v8.0.0102
7. Fix can not specify the path to the dictionary file for dictionary completion. 1. Add invalid characters to file name in v8.0.0100, 2. Add flags to check characters to some options in v8.0.0101. What src/option.c 5883 - && vim_strpbrk(*varp, (char_u *)"/\\*?[|<>") != NULL) 5883 + && vim_strpbrk(*varp, (char_u *)"/\\*?[|;&<>\r\n") != NULL) src/option.c 995 - {"dictionary", "dict", P_STRING|P_EXPAND|P_VI_DEF|P_ONECOMMA|P_NODUP, 995 + {"dictionary", "dict", P_STRING|P_EXPAND|P_VI_DEF|P_ONECOMMA|P_NODUP|P_NFNAME,
8. Details • One morning, Vim got an error when startup after build • Found out the cause was v8.0.0101 from commit logs ◦ And it is ridiculous to back up by operation... • Decided to write a patch ◦ Enthusiasm was rising by VimConf 2016 held on the same month ◦ Using dictionary completion is minorities, so I thought that I had to fix it myself ◦ I taught dictionary completion to my teammates a while ago, and it will be in trouble if it can't be used ◦ Maybe it was a careless mistake, but I'd like to add a test code so that it will not reoccur at all E474: Invalid argument: dictionary=/usr/share/dict/words
9. Action • Created a branch that undo the option with test added in same day ◦ I didn't know how to add tests, nay, how to run tests • Decided to send patch to vim_dev, after wondering where to send ◦ There were issues on GitHub, but there was a person reporting it with vim_dev earlier than that • However, Bram declared the fix in vim_dev, so I sent only the test part ◦ It seemed to be fixed without doing anything, but I didn't want to waste the effort so far • Merged in the next morning patch 8.0.0102 Problem: Cannot set 'dictionary' to a path. Solution: Allow for slash and backslash. Add a test (partly by Daisuke Suzuki, closes #1279, closes #1284)
10. --- a/src/option.c +++ b/src/option.c @@ -458,0 +459 @@ struct vimoption +#define P_NDNAME 0x8000000L /* only normal dir name chars allowed */ @@ -995 +996 @@ static struct vimoption options[] = - {"dictionary", "dict", P_STRING|P_EXPAND|P_VI_DEF|P_ONECOMMA|P_NODUP|P_NFNAME, + {"dictionary", "dict", P_STRING|P_EXPAND|P_VI_DEF|P_ONECOMMA|P_NODUP|P_NDNAME, @@ -5882,2 +5883,4 @@ did_set_string_option( - else if ((options[opt_idx].flags & P_NFNAME) + else if (((options[opt_idx].flags & P_NFNAME) && vim_strpbrk(*varp, (char_u *)"/\\*?[|;&<>\r\n") != NULL) + || ((options[opt_idx].flags & P_NDNAME) + && vim_strpbrk(*varp, (char_u *)"*?[|;&<>\r\n") != NULL)) --- a/src/testdir/test_options.vim +++ b/src/testdir/test_options.vim @@ -108,0 +109,15 @@ endfunc + +func Test_dictionary() + " Check that it's possible to set the option. + set dictionary=/usr/share/dict/words + call assert_equal('/usr/share/dict/words', &dictionary) + set dictionary=/usr/share/dict/words,/and/there + call assert_equal('/usr/share/dict/words,/and/there', &dictionary) + set dictionary=/usr/share/dict\ words + call assert_equal('/usr/share/dict words', &dictionary) + + " Check rejecting weird characters. + call assert_fails("set dictionary=/not&there", "E474:") + call assert_fails("set dictionary=/not>there", "E474:") + call assert_fails("set dictionary=/not.*there", "E474:") +endfunc
11. Episode of v8.0.0104
12. What Fix missing of the string check in thesaurus option. • Similar feature of dictionary option used in thesaurus completion • It should have the same fix as v8.0.0102, but it was not
13. Details • I knew the option • It was not included in v8.0.0101, so I doubted that it was forgotten • The previous patch only added tests, so I thought it was a chance ◦ Add a test (partly by Daisuke Suzuki ... ◦ Enthusiasm was not cool down yet
14. Action • Created a patch the next day of v8.0.0102 ◦ The contents are almost the same ◦ But, test has been added • Create a pull request on GitHub • It was merged in less than an hour patch 8.0.0104 Problem: Value of 'thesaurus' option not checked properly. Solution: Add P_NDNAME flag. (Daisuke Suzuki)
15. --- a/src/option.c +++ b/src/option.c @@ -2663 +2663 @@ static struct vimoption options[] = - {"thesaurus", "tsr", P_STRING|P_EXPAND|P_VI_DEF|P_ONECOMMA|P_NODUP, + {"thesaurus", "tsr", P_STRING|P_EXPAND|P_VI_DEF|P_ONECOMMA|P_NODUP|P_NDNAME, --- a/src/testdir/test_options.vim +++ b/src/testdir/test_options.vim @@ -109,15 +109,23 @@ endfunc -func Test_dictionary() +func Check_dir_option(name) " Check that it's possible to set the option. - set dictionary=/usr/share/dict/words - call assert_equal('/usr/share/dict/words', &dictionary) - set dictionary=/usr/share/dict/words,/and/there - call assert_equal('/usr/share/dict/words,/and/there', &dictionary) - set dictionary=/usr/share/dict\ words - call assert_equal('/usr/share/dict words', &dictionary) + exe 'set ' . a:name . '=/usr/share/dict/words' + call assert_equal('/usr/share/dict/words', eval('&' . a:name)) + exe 'set ' . a:name . '=/usr/share/dict/words,/and/there' + call assert_equal('/usr/share/dict/words,/and/there', eval('&' . a:name)) + exe 'set ' . a:name . '=/usr/share/dict\ words' + call assert_equal('/usr/share/dict words', eval('&' . a:name)) " Check rejecting weird characters. - call assert_fails("set dictionary=/not&there", "E474:") - call assert_fails("set dictionary=/not>there", "E474:") - call assert_fails("set dictionary=/not.*there", "E474:") + call assert_fails("set " . a:name . "=/not&there", "E474:") + call assert_fails("set " . a:name . "=/not>there", "E474:") + call assert_fails("set " . a:name . "=/not.*there", "E474:") +endfunc + +func Test_dictionary() + call Check_dir_option('dictionary') +endfunc + +func Test_thesaurus() + call Check_dir_option('thesaurus') endfunc
16. Episode of v8.0.0512
17. What Fix the problem that the candidate of insert mode completion might be a long time. • There was unnecessary waiting when looking for candidates from files and buffers • That is, as there are more candidates like dictionary completion becomes slow ◦ There was also an influence on other completions, but it doesn't matter when not many candidates ◦ It occurred only in the CUI version of Linux and Mac, and it did not affect Windows or MacVim
18. Details • At first I thought that it was due to my PC, ◦ The specs were not so good ◦ Despite this, heavy background processing was frequently performed • Colleague said it's became slow, so I found out ◦ It was fast when turn back v8.0.0104 to v8.0.0000 • I planned to make my colleague a contributor, ◦ At least, nobody seemed to be in trouble ◦ Even if he could not make it to the end, I thought that just only have to report the results so far ◦ I had been investigating so that I can give him a hint anytime • Without progress, decided to change my job and write a patch
19. Action • Immediately found out the patch and function that caused it ◦ It is v8.0.0050, result of git bisect ◦ mch_inchar() in os_unix.c was greatly modified • But I could not find a root cause at all, so trial and error over and over ◦ using debugger • I managed to fix it, but no confidence, so I asked vim-jp for help ◦ Thank you, @h_east ◦ • After that, I created a pull request and it was merged on the same day patch 8.0.0512: check for available characters takes too long Problem: Check for available characters takes too long. Solution: Only check did_start_blocking if wtime is negative. (Daisuke Suzuki, closes #1591)
20. --- a/src/os_unix.c +++ b/src/os_unix.c @@ -506,17 +506,17 @@ mch_inchar( if (do_resize /* interrupted by SIGWINCH signal */ #ifdef FEAT_CLIENTSERVER || server_waiting() #endif #ifdef MESSAGE_QUEUE || interrupted #endif || wait_time > 0 - || !did_start_blocking) + || (wtime < 0 && !did_start_blocking)) continue; /* no character available or interrupted */ break; } return 0; }
21. Intermediate Summary • Bug fix patches are easily approved ◦ Recommended for beginner ◦ Building Vim everyday is a good habit to find problems • Vim talk at work is important ◦ There is something new discovery ◦ It leads to problem solving
22. Useful techniques
23. Debugging (gdb) break os_unix.c:399 (gdb) break os_unix.c:507 (gdb) ignore 1 100000 (gdb) ignore 2 100000 (gdb) continue (gdb) info breakpoints Num Type Disp Enb Address What 1 breakpoint keep y 0x000000000052fa50 in mch_inchar at os_unix.c:399 breakpoint already hit 3615 times ignore next 96385 hits 2 breakpoint keep y 0x000000000052fb02 in mch_inchar at os_unix.c:507 breakpoint already hit 20073 times ignore next 79927 hits • gdb ◦ n(next), s(step), l(list), bt(backtrace), b(break) ◦ Count the numbers executed with the combination of break and ignore
24. Testing • make test ◦ When adding a test, modify Make_all.mak and write a test with Vim script ◦ For details, refer to src/testdir/README.txt in repository
25. How to send a patch • vim_dev ◦ Google Groups(https://groups.google.com/d/forum/vim_dev) ◦ A patch can be attached or a link to gist ▪ Unified diff format is preferred ◦ Posting requires approval of account • GitHub ◦ https://github.com/vim/vim ◦ Pull request like other OSS ◦ But instead of merging on github, it is merged at Bram's hand and committed to master
26. Conclusion • Let's do it who want to be a contributor or interested in become contributor! ◦ Let's get started from where you can • (For Japanese) If there is something, feel free to contact vim-jp! ◦ It's better than holding alone or just posted to twitter ◦ Someone will take a consultation even when there is no confidence in the patch or English