How ordinary Vim user contributed to Vim

VimConf 2017

Transcript

1. How ordinary Vim user
   contributed to Vim
   VimConf 2017
   daisuzu
   

   View Slide

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
   

   View Slide

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
   

   View Slide

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
   

   View Slide

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
   

   View Slide

6. Episode of v8.0.0102
   

   View Slide

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,
   

   View Slide

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
   

   View Slide

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)
   

   View Slide

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
    

    View Slide

11. Episode of v8.0.0104
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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)
    

    View Slide

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
    

    View Slide

16. Episode of v8.0.0512
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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)
    

    View Slide

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;
    }
    

    View Slide

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
    

    View Slide

22. Useful techniques
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide