Early Warning System

Once a new review is created or an old review has been updated, the Early Warning System kicks in, by watching for new patch reviews to process.

This system is basically a collection of bots that will run a wide array of tests on the patch. The crucial part about these tests is that they should all be quick to run, 5-10 minutes or perhaps even less. The idea is not to catch all errors, but to get immediate/early feedback on a patch.

If you get a -1 from a review bot, please ensure you look at the issues noted by the bot, and upload a new version. The review bots will then automatically review the update and give you feedback.

Sanity Bot

This bot runs the Simple Sanitizer (see below) for the Gerrit server. It does two additional checks:

1. Work In Progress push check. This is meant to avoid that somebody accidentally submits something you marked for later cleanup. It checks for the strings "WIP" and "***" in the summary line.
2. A check about touching certain files. Some files are considered "endangered" - seemingly innocuous modifications almost always turn out to be bogus when not done by one of a few experts. Touching other files may require followup changes, e.g., documentation updates after GUI changes. The bot automatically invites relevant reviewers.

The bot's verdict is a recommendation, and may be overridden by every Approver. However, don't ignore it unless you have a really good reason. "I need to get this done now" is not a valid reason. Neither is "I cannot be bothered to understand what it wants from me". If there is any doubt, discuss alternatives with somebody appropriate. Each override needs to come with a justifying comment on Gerrit.

The Simple Sanitizer

The sanitizer script is available from the qtrepotools repository. Every contributor should install it locally in every repository. See the qtrepotools/git-hooks/git_post_commit_hook script (it contains installation instructions).

It does the following checks:

1. The line ending check. We are developing in a heterogeneous environment with both Unix and Windows machines. Therefore it is imperative to have all files in the repository in the canonical LF-only format. Windows users need to set the git option core.autocrlf to true to automatically get CRLF line endings which are suitable for the native tools.
2. Conflict marker check. Unless you are adding a text file which contains the patterns on purpose, you most probably botched a merge/rebase.
3. Generated file check. Don't commit generated files- they bloat the repository and create spurious changes. If a file is generated but you don't know how to automate the build process, ask an expert.
4. Backup file check. Don't commit backup files, for hopefully obvious reasons.
5. Copyright header check. Source files must declare copyright (unless they are very small). They should also name a licence, but the check does not cover that.
6. Check for project/config files of alien build systems. As we are using qmake, these will in most cases fall under the "generated file" rule anyway.
7. Big file addition check. Think three times whether you really need that huge media file, test binary or screenshot. As we are using git, such mistakes are not correctable - once you added a huge file, everyone who clones the repository will have to pay for it with network traffic and disk space for all times. Source code files are exempt from the check at all; for the rest it is a bit paranoid (it will complain about anything bigger than 50 KiB or an instantaneous file growth over 150% if the file is bigger than 20 KiB).
8. Huge file addition check. If you trigger that one, you almost certainly did something wrong. This blocks submission; ask an admin.
9. Some simple checks on the commit message:
   1. Compliance with the expected structure.
   2. That reverts are properly explained, which follows directly from the Commit Policy point 8.2.
   3. That the author email address does not reflect an unconfigured git setup.
   4. That the author real name looks reasonable. Use proper capitalization and periods after initials.
      • Exceptions can be requested, but must be reasonably justified.
   5. That the summary is between 7 and 70 characters long. Up to 120 characters are accepted grudgingly.
   6. That the summary does not end with a period. Summaries are titles, and titles do not end with periods.
   7. That the summary is in an imperative mood, i.e. "spoken or written as if giving a command or instruction".
   8. That the summary does not refer to JIRA issues. Use Task-number footers for that. If the reader needs to know which issue a commit refers to, they most definitely should read more than just the commit summary anyway.
   9. That no Reviewed-by footers have been manually added, as they would stay behind even if the mentioned person did not actually make a review.
   10. That no Signed-off-by footers have been added, as they are redundant with the act of signing the CLA (advisory only).
   11. That the Pick-to footer points to reasonable branches for the repo to which the change has been submitted
   12. That changes outside a repo's masterlike branch (usually "dev") don't omit the Pick-to footer
       • See Branch Guidelines -> "How to Cherry-pick" for more information on the Pick-to footer.
   13. That there is an empty line before the footers.
   14. That there are no empty lines between footers. The footers (including any "(cherry picked from …)" lines) should form one tidy set-off block. If you are getting an empty line before the automatically added Change-Id line, you are most probably using a bad commit-msg hook - get a fresh copy from our Gerrit, or use the one from our qtrepotools repository. If you are using the Qt supermodule, you can run init-repository -f --force-hooks to update your hooks.
   15. That footers are not followed by more non-footer text.
       • The "Conflicts:" section in merge commits will trigger this. Move the section above the footers.
   16. That JIRA issues are not referenced by URL. URLs make the commit messages noisy and unnecessarily bind them to the current location of the bug tracker. Use the Task-number footer according to the commit template to make Gerrit automatically link issues.
   17. That other commits are not referenced by Gerrit Change-Id or URL. Change-Ids have the disadvantage of introducing an unnecessary indirection when browsing history. URLs are also noisy, non-relocatable, and make off-line lookup impossible. Use the SHA1 of the commit that has been actually merged (the automatically added new PatchSet seen in Gerrit Changes).
   18. That [ChangeLog] is correctly formed if present:
       1. That the tag is camel-cased, and does not try to be a footer.
       2. That the paragraph is preceeded and followed by an empty line.
       3. That tags don't mention JIRA issues. Use Task-number footers for that.
       4. That tags don't mention the current repository, as that's redundant.
       5. That the last tag is followed by a space (or line break), for tidyness.
10. Some simple checks for incorrect use of Apple brands/trademarks/terminology:
    1. That the deprecated defines Q_OS_MAC, Q_OS_MACX, and Q_OS_OSX are not being used. Use Q_OS_MACOS to refer to macOS, Apple's desktop operating system, or Q_OS_DARWIN to refer to all Apple platforms.
    2. That the macOS operating system is correctly named in all instances. ONLY use "macOS" to refer to the desktop operating system. The operating system was formerly named "OS X" but was changed to "macOS" in 2016 with the release of macOS 10.12 Sierra. Terms such as "Mac", "Macintosh", "Mac OS", "Mac OS X", "OS X", etc., with any combination of spacing and punctuation, should not be used to refer to the operating system. Note that the hardware platform on which macOS runs is correctly termed "Mac" or "Macintosh". Also note that when referring to the operating system in the documentation (source), the QDoc macro \macos can be used; it expands to the correct name "macOS".
    3. That the deprecated qmake scopes mac, macx , and osx are not being used. Their replacements, macos (when referring to the desktop operating system) and darwin (when referring to all Apple platforms), should be used instead. Note that some uses of macx in a qmake file may still be valid, for example macx- or macx* in reference to certain mkspecs (which cannot be renamed for backwards compatibility reasons).
11. A check for common spelling mistakes.
12. A check for malformed PNG images. You can test images for format defects using the identify command, from the ImageMagick package. In particular, some tools create PNG images with a bogus "sRGB chunk" that can readily be removed using:

    $ pngcrush -ow -brute -rem allb -reduce image.png

    (It may be necessary to add a -force if this wouldn't reduce the file size much.) While this doesn't catch every PNG formatting defect, it deals with the common cases. For images with more stubborn defects,

    $ optipng -o7 strip all image.png

    may help. If this last reports that it found "recoverable errors", it'll suggest passing it the -fix option; be sure to check the result actually looks like what you started with.

13. Check for mixing whitespace and non-whitespace changes. As implied by the "don't mix unrelated changes" rule, a commit may contain only whitespace changes or "proper" changes (with whitespace cleanups only on already changed lines, and, as an exception, additions and removals of empty lines). This helps to ensure the usefulness of git blame (note that a clean history is better than using git blame -w, as the latter will also hide significant whitespace changes). A negative verdict may be overridden if it is a false positive resulting from imperfect heuristics, in particular if the change is fixing coding style issues beyond mere whitespace cleanup.
14. Various whitespace abuse checks. Tabs in files they are not approved in, trailing whitespace, missing newline at end of file, etc.
15. Some simple coding style checks. (currently advisory only)

Bugs and feature requests can be filed in JIRA.