Upgrade to Pro — share decks privately, control downloads, hide ads and more …

How ordinary Vim user contributed to Vim

Daisuke Suzuki
November 04, 2017

How ordinary Vim user contributed to Vim

VimConf 2017

Daisuke Suzuki

November 04, 2017
Tweet

More Decks by Daisuke Suzuki

Other Decks in Technology

Transcript

  1. • 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
  2. 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
  3. 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
  4. 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
  5. 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,
  6. 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
  7. 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)
  8. --- 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
  9. 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
  10. 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
  11. 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)
  12. --- 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
  13. 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
  14. 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
  15. 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)
  16. --- 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; }
  17. 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
  18. 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
  19. 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
  20. 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
  21. 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