This RFC replaces Commit Message Guideline
This RFC proposes the addition of the following commit message guidelines to pull_request.rst, under section Submit a Pull Request, below subsection Guidelines, as a subsection named “Commit Message Guideline”. The text in the second-last item in subsection Guidelines that mentions PR tags will also be extended to mentioned this guideline.
Commit Message Guideline
Apache TVM uses the Github (GH) platform for patch submission and code review via Pull Requests (PRs). The final commit (title and body) that is merged into the Apache TVM main tree is composed from the PR’s title and body and must be kept updated and reflecting the new changes in the code as per the reviews and discussions. The PR’s title and body are also checked automatically by a linter tool and some rules (see below) are enforced.
Hence, although these guidelines apply essentially to the PRs’ title and body messages, because GH auto-generates the PR’s title and body from the commits on a given branch, it’s recommended to follow these guidelines right from the beginning, when preparing commits in general to be submitted to the Apache TVM project. This will ease the creation of a new PR, avoiding rework, and also will help the review.
The guidelines in this document are very similar to the rules used by other open source projects, like gcc, LLVM, and Linux.
The rules below will help to achieve uniformity that has several benefits, both for review and for the code base maintenance as a whole, helping you to write commit messages with a good quality suitable for the Apache TVM project, allowing fast log searches, bisecting, and so on.
PR/commit title:
- Guarantee a title exists (enforced);
- Don’t use Github usernames in the title, like @username (enforced);
- Check if a tag should be present as a hint about what component(s) of the code the commits “touch”. For example [BugFix], [CI], [microTVM], and [TVMC]. Tags go between square brackets and appear first in the title. Tags help reviewers to identify the PRs they can/want to review and also help the release folks when generating the release notes (enforced);
- Use an imperative mood. Avoid titles like “Added operator X” and “Updated image Y in the CI”, instead use the forms “Add feature X” and “Update image Y in the CI” instead;
- Observe proper use of caps at the beginning (uppercase for the first letter) and for acronyms, like, for instance, TVM, FVP, OpenCL. Hence instead of “fix tvm use of opencl library”, write it as “Fix TVM use of OpenCL library”;
- No period at the end of the title is necessary.
PR/commit body:
- Guarantee a body exists (enforced);
- Don’t use Github usernames in body text, like @username (enforced);
- Avoid “bullet” commit message bodies: “bullet” commit message bodies are not bad per se, but “bullet” commit messages without any description or explanation is likely as bad as commits without any description, rationale, or explanation in the body;
For minor deviations from these guidelines, the community will normally favor reminding the contributor of this policy over reverting or blocking a commit / PR.
Commits and PRs without a title and/or a body are not considered minor deviations from these guidelines and hence must be avoided.
Most importantly, the contents of the commit message, especially the body, should be written to convey the intention of the change, so it should avoid being vague. For example, commits with a title like “Fix”, “Cleanup”, and “Fix flaky test” and without any body text should be avoided. Also, for the review, it will leave the reviewer wondering about what exactly was fixed or changed and why the change is necessary, slowing the review.
Below is an example that can be used as a model:
[microTVM] Zephyr: Remove zephyr_board option from build, flash, and open_transport methods
Currently it’s necessary to pass the board type via ‘zephyr_board’ option to the Project API build, flash, and open_transport methods.
However, since the board type is already configured when the project is created (i.e. when the generate_project method is called), it’s possible to avoid this redundancy by obtaining the board type from the project configuration files.
This commit adds code to obtain the board type from the project CMake files, removing this option from build, flash, and open_transport methods, so it’s only necessary to specify the ‘zephyr_board’ option when calling generate_project.
This commit also moves the ‘verbose’ and ‘west_cmd’ options from ‘build’ method to ‘generate_project’, reducing further the number of required options when building a project, since the ‘build’ method is usually called more often than the ‘generate_project’.
If a PR is created from more than one commit (a patchset), then ideally the changes should be split into commits in a reasonable way. For instance, if it’s a fix and so it also adds a regression test, it’s a good practice to split the changes having one commit for the fix and another one for the test, applying the rules and recommendations in this guideline to each commit.
After a new PR is created and the review starts it’s common that reviewers will request changes. Usually the author will address the reviewers’ comments and push additional commits on top of the initial ones. For these additional commits there is no recommendation regarding the commit messages. The only recommendation is that if the additional commits render the PR title and/or body outdated then the PR title and body must reflect the new changes in the code and be updated accordingly (remember that the PR title and body will be used to compose the final commit message that will land in the main tree).
There are some prerequisites and itens to be discussed before accepting this Commit Message Guideline, which are:
-
Committers agree on merging code to the main tree using the tvm-bot, because the bot will correctly take care of using the PR’s title and body (and parsing them) when preparing the final commit that will land into the TVM’s main tree, regardless if the PR is composed of one or more commits? Currently GH workflow is confusing because it has different behavior depending on the number of commits composing the PR: if there is only one commit GH will use the commit title and body to generate the final commit to get merged to the main tree; otherwise, if there is more than one commit, it will use the PR’s title as the final commit title and as for the final commit body it will be al concatenation of all the commit titles and bodies, sequentially, which can get quite messy since even the additional commits - the ones added as the outcome of the review process - will get concatenated.
-
Will tvm-bot keep the commit message authorship?
-
How is access control applied to the tvm-bot? Who is allowed exactly to merge the PR using the tvm-bot? Just committers?
-
Should we block on a lint error against the PR (enforced rules) or just warn the user? I believe we need to block on enforced rules.
-
Line breakage must be applied to the text in the PR’s body, enforcing 72 characters per line (tvm-bot will guarantee that)?