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. 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