diff options
Diffstat (limited to 'CONTRIBUTING.md')
| -rw-r--r-- | CONTRIBUTING.md | 206 |
1 files changed, 143 insertions, 63 deletions
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dbc048d939..b7392eaf0c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,46 +1,82 @@ # Contributing to Neovim -## Getting started - -- Help us review [open pull requests](https://github.com/neovim/neovim/pulls)! - See [Reviewing](#reviewing) for guidelines. -- Try an [entry-level issue][entry-level] if you are wondering where to start. -- Or [merge a Vim patch]. - -## Reporting problems - -- Check the [**FAQ**][wiki-faq]. -- Search [existing issues][github-issues] (including closed!) +Getting started +--------------- + +If you want to help but don't know where to start, here are some +low-risk/isolated tasks: + +- [Merge a Vim patch]. +- Try a [complexity:low] issue. +- Fix bugs found by [Clang](#clang-scan-build), [PVS](#pvs-studio) or + [Coverity](#coverity). + +Developer guidelines +-------------------- + +- Nvim contributors should read `:help dev`. +- External UI developers should read `:help dev-ui`. +- API client developers should read `:help dev-api-client`. +- Nvim developers are _strongly encouraged_ to install `ninja` for faster builds. + ``` + sudo apt-get install ninja-build + make distclean + make # Nvim build system uses ninja automatically, if available. + ``` + +Reporting problems +------------------ + +- [Check the FAQ][wiki-faq]. +- [Search existing issues][github-issues] (including closed!) - Update Neovim to the latest version to see if your problem persists. -- If you're using a plugin manager, comment out your plugins, then add them back - in one by one, to narrow down the cause of the issue. -- Crash reports which include a stacktrace are 10x more valuable. -- [Bisecting][git-bisect] to the cause of a regression often leads to an - immediate fix. - -## Pull requests ("PRs") - -- To avoid duplicate work, you may want to create a `[WIP]` pull request so that - others know what you are working on. -- Avoid cosmetic changes to unrelated files in the same commit: extra noise - makes reviews more difficult. -- Use a [feature branch][git-feature-branch] instead of the master branch. -- [Rebase your feature branch][git-rebasing] onto (upstream) master before - opening the PR. -- After addressing the review comments, it's fine to rebase and force-push to - your review. -- Try to [tidy your history][git-history-rewriting]: combine related commits - with interactive rebasing, separate monolithic commits, etc. - -### Stages: WIP, RFC +- Disable plugins incrementally, to narrow down the cause of the issue. +- When reporting a crash, [include a stacktrace](https://github.com/neovim/neovim/wiki/Development-tips#backtrace-linux). +- [Bisect][git-bisect] to the cause of a regression, if you are able. This is _extremely_ helpful. +- Check `$NVIM_LOG_FILE`, if it exists. +- Include `cmake --system-information` for build-related issues. -Pull requests are processed in two stages: _WIP_ (Work In Progress) and _RFC_ -(Request For Comment). +Pull requests ("PRs") +--------------------- -- Untagged PRs are assumed to be RFC, meaning the work is ready for review and - you would like feedback. -- Preprend `[WIP]` to the PR title if you are _not_ ready for feedback and the - work is still in flux. This saves time and confusion. +- To avoid duplicate work, create a `[WIP]` pull request as soon as possible. +- Avoid cosmetic changes to unrelated files in the same commit. +- Use a [feature branch][git-feature-branch] instead of the master branch. +- Use a **rebase workflow** for small PRs. + - After addressing review comments, it's fine to rebase and force-push. +- Use a **merge workflow** for big, high-risk PRs. + - Merge `master` into your PR when there are conflicts or when master + introduces breaking changes. + - Use the `ri` git alias: + ``` + [alias] + ri = "!sh -c 't=\"${1:-master}\"; s=\"${2:-HEAD}\"; mb=\"$(git merge-base \"$t\" \"$s\")\"; if test \"x$mb\" = x ; then o=\"$t\"; else lm=\"$(git log -n1 --merges \"$t..$s\" --pretty=%H)\"; if test \"x$lm\" = x ; then o=\"$mb\"; else o=\"$lm\"; fi; fi; test $# -gt 0 && shift; test $# -gt 0 && shift; git rebase --interactive \"$o\" \"$@\"'" + ``` + This avoids unnecessary rebases yet still allows you to combine related + commits, separate monolithic commits, etc. + - Do not edit commits that come before the merge commit. +- During a squash/fixup, use `exec make -C build unittest` between each + pick/edit/reword. + +### Stages: WIP, RFC, RDY + +Pull requests have three stages: `[WIP]` (Work In Progress), `[RFC]` (Request +For Comment) and `[RDY]` (Ready). + +- Untagged PRs are assumed to be `[RFC]`, i.e. you are requesting a review. +- Prepend `[WIP]` to the PR title if you are _not_ requesting feedback and the + work is still in flux. +- Prepend `[RDY]` to the PR title if you are _done_ with the PR and are only + waiting on it to be merged. + +For example, a typical workflow is: + +1. You open a `[WIP]` PR where the work is _not_ ready for feedback, you just want to + let others know what you are doing. +2. Once the PR is ready for review, you replace `[WIP]` in the title with `[RFC]`. + You may add fix up commits to address issues that come up during review. +3. Once the PR is ready for merging, you rebase/squash your work appropriately and + then replace `[RFC]` in the title with `[RDY]`. ### Commit messages @@ -50,43 +86,86 @@ the VCS/git logs more valuable. - Try to keep the first line under 72 characters. - **Prefix the commit subject with a _scope_:** `doc:`, `test:`, `foo.c:`, `runtime:`, ... - - For commits that contain only style/lint changes, a single-word subject - line is preferred: `style` or `lint`. + - Subject line for commits with only style/lint changes can be a single + word: `style` or `lint`. - A blank line must separate the subject from the description. - Use the _imperative voice_: "Fix bug" rather than "Fixed bug" or "Fixes bug." ### Automated builds (CI) -Each pull request must pass the automated builds ([travis CI] and [quickbuild]). +Each pull request must pass the automated builds on [Travis CI], [QuickBuild] +and [AppVeyor]. -- CI builds are compiled with [`-Werror`][gcc-warnings], so if your PR - introduces any compiler warnings, the build will fail. +- CI builds are compiled with [`-Werror`][gcc-warnings], so compiler warnings + will fail the build. - If any tests fail, the build will fail. - See [Building Neovim#running-tests][wiki-run-tests] to run tests locally. + See [test/README.md#running-tests][run-tests] to run tests locally. Passing locally doesn't guarantee passing the CI build, because of the different compilers and platforms tested against. -- CI runs [ASan] and other analyzers. To run valgrind locally: - `VALGRIND=1 make test` +- CI runs [ASan] and other analyzers. + - To run valgrind locally: `VALGRIND=1 make test` + - To run Clang ASan/UBSan locally: `CC=clang make CMAKE_FLAGS="-DCLANG_ASAN_UBSAN=ON"` - The `lint` build ([#3174][3174]) checks modified lines _and their immediate neighbors_. This is to encourage incrementally updating the legacy style to meet our style guidelines. - - A single word (`lint` or `style`) is sufficient as the subject line of - a commit that contains only style changes. - [How to investigate QuickBuild failures](https://github.com/neovim/neovim/pull/4718#issuecomment-217631350) -### Coverity +QuickBuild uses this invocation: + + mkdir -p build/${params.get("buildType")} \ + && cd build/${params.get("buildType")} \ + && cmake -G "Unix Makefiles" -DBUSTED_OUTPUT_TYPE=TAP -DCMAKE_BUILD_TYPE=${params.get("buildType")} + -DTRAVIS_CI_BUILD=ON ../.. && ${node.getAttribute("make", "make")} + VERBOSE=1 nvim unittest-prereqs functionaltest-prereqs -[Coverity](https://scan.coverity.com/projects/neovim-neovim) runs against the -master build. If you want to view the defects, just request access at the -_Contributor_ level. An Admin will grant you permission. -Use this commit-message format for coverity fixes: +### Clang scan-build - coverity/<id>: <description of what fixed the defect> +View the [Clang report] to see potential bugs found by the Clang +[scan-build](https://clang-analyzer.llvm.org/scan-build.html) analyzer. -where `<id>` is the Coverity ID (CID). For example see [#804](https://github.com/neovim/neovim/pull/804). +- Search the Neovim commit history to find examples: + ``` + git log --oneline --no-merges --grep clang + ``` +- To verify a fix locally, run `scan-build` like this: + ``` + rm -rf build/ + scan-build --use-analyzer=/usr/bin/clang make + ``` -## Reviewing +### PVS-Studio + +View the [PVS report](https://neovim.io/doc/reports/pvs/PVS-studio.html.d/) to +see potential bugs found by [PVS Studio](https://www.viva64.com/en/pvs-studio/). + +- Use this format for commit messages (where `{id}` is the PVS warning-id)): + ``` + PVS/V{id}: {description} + ``` +- Search the Neovim commit history to find examples: + ``` + git log --oneline --no-merges --grep PVS + ``` +- Try `./scripts/pvscheck.sh` to run PVS locally. + +### Coverity + +[Coverity](https://scan.coverity.com/projects/neovim-neovim) runs against the +master build. To view the defects, just request access; you will be approved. + +- Use this format for commit messages (where `{id}` is the CID (Coverity ID); + ([example](https://github.com/neovim/neovim/pull/804))): + ``` + coverity/{id}: {description} + ``` +- Search the Neovim commit history to find examples: + ``` + git log --oneline --no-merges --grep coverity + ``` + +Reviewing +--------- To help review pull requests, start with [this checklist][review-checklist]. @@ -101,10 +180,8 @@ commits in the feature branch which aren't in the `master` branch; `-p` shows each commit's diff. To show the whole surrounding function of a change as context, use the `-W` argument as well. - -[entry-level]: https://github.com/neovim/neovim/issues?labels=entry-level&state=open [gcc-warnings]: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html -[git-bisect]: http://git-scm.com/book/tr/v2/Git-Tools-Debugging-with-Git +[git-bisect]: http://git-scm.com/book/en/v2/Git-Tools-Debugging-with-Git [git-feature-branch]: https://www.atlassian.com/git/tutorials/comparing-workflows [git-history-filtering]: https://www.atlassian.com/git/tutorials/git-log/filtering-the-commit-history [git-history-rewriting]: http://git-scm.com/book/en/v2/Git-Tools-Rewriting-History @@ -115,10 +192,13 @@ as context, use the `-W` argument as well. [hygiene]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html [style-guide]: http://neovim.io/develop/style-guide.xml [ASan]: http://clang.llvm.org/docs/AddressSanitizer.html -[wiki-run-tests]: https://github.com/neovim/neovim/wiki/Building-Neovim#running-tests +[run-tests]: https://github.com/neovim/neovim/blob/master/test/README.md#running-tests [wiki-faq]: https://github.com/neovim/neovim/wiki/FAQ [review-checklist]: https://github.com/neovim/neovim/wiki/Code-review-checklist [3174]: https://github.com/neovim/neovim/issues/3174 -[travis CI]: https://travis-ci.org/neovim/neovim -[quickbuild]: http://neovim-qb.szakmeister.net/dashboard -[merge a Vim patch]: https://github.com/neovim/neovim/wiki/Merging-patches-from-upstream-Vim +[Travis CI]: https://travis-ci.org/neovim/neovim +[QuickBuild]: http://neovim-qb.szakmeister.net/dashboard +[AppVeyor]: https://ci.appveyor.com/project/neovim/neovim +[Merge a Vim patch]: https://github.com/neovim/neovim/wiki/Merging-patches-from-upstream-Vim +[Clang report]: https://neovim.io/doc/reports/clang/ +[complexity:low]: https://github.com/neovim/neovim/issues?q=is%3Aopen+is%3Aissue+label%3Acomplexity%3Alow |