1,510
edits
Difference between revisions of "Commit Guidelines"
Jump to navigation
Jump to search
→Commit guidelines: added bit about linear history
Salty-horse (talk | contribs) (→Rules: Describe ALL and JANITORIAL) |
(→Commit guidelines: added bit about linear history) |
||
| (8 intermediate revisions by 6 users not shown) | |||
| Line 1: | Line 1: | ||
This page contains some guidelines about commits and commit messages. | |||
== | == Commit guidelines == | ||
Commits shouldn't contain multiple unrelated changes; try and make piecemeal changes if you can, to make it easier to review and merge. In particular, don't commit style/whitespace changes and functionality changes in a single commit. | |||
If you change shared common code, then (while not necessarily in the same commit, due to the above guideline!) you should also make at least a best-effort attempt to make sure all of the engine/backend code stays working. | |||
Assure that ScummVM compiles with every commit. In case of regressions, this helps to track down the commit introducing the regression. | |||
Changes to user facing elements and common code also requires proper documentation. | |||
No merge commits are allowed, we enforce linear history. | |||
== Commit message formatting == | |||
We won't start beating people up, but everyone should at least all be aware of the issue at hand; namely, what distinguishes good commit messages from bad ones. This page hopes to help all of us to improve our commit messages a bit... The benefit will be to the full team as commit logs will be more readable. | |||
The rules on commit messages are the following (they're standard to git, except our SUBSYSTEM requirement): | |||
<pre> | <pre> | ||
SUBSYSTEM: Short (50 chars or less) summary of changes | SUBSYSTEM: Short (50 chars or less) summary of changes | ||
| Line 27: | Line 41: | ||
</pre> | </pre> | ||
Particularly critical is the first line: This first line is used in short log format, and providing engine name, sound subsystem, backend name or whatever lets decide quickly should reviewers look deeply into commit or skip. | Particularly critical is the first line: This first line is used in a short log format, and providing engine name, sound subsystem, backend name, or whatever - lets us decide quickly should reviewers look deeply into commit or skip. | ||
The subsystem names which were used so far: | The subsystem names which were used so far: | ||
* '''AGOS''', '''SCI''', '''SCUMM''', whatever engine name | * '''AGOS''', '''SCI''', '''SCUMM''', whatever engine name | ||
* '''SDL''', ''' | * '''SDL''', '''ANDROID''', '''WII''', '''NDS''', whatever backend name | ||
* ''' | * '''OSYSTEM''': our middleware code | ||
* ''' | * '''I18N''': internationalization | ||
* '''GUI''': all gui-related | * '''GUI''': all gui-related | ||
* '''TOOLS''': ScummVM tools module or built-in tools | * '''TOOLS''': ScummVM tools module or built-in tools | ||
* '''MIDI''': MIDI sound-related | * '''MIDI''': MIDI sound-related | ||
* '''ALL''': A change that covers all subsystems | * '''BUILD''': Files related to building the project, makefiles, and related | ||
* '''ALL''': A change that covers multiple, or all, subsystems | |||
* '''JANITORIAL''': Cross-subsystem cleanups and formatting changes | * '''JANITORIAL''': Cross-subsystem cleanups and formatting changes | ||
After first line, an empty line is required which is used as separator. This first line has to be short and less than 50 characters so the log will fit on a standard 80-column terminal. | After the first line, an empty line is required which is used as the separator. This first line has to be short and less than 50 characters so the log will fit on a standard 80-column terminal. | ||
== | == Commit messages == | ||
Commit messages like the following aren't very helpful: | Commit messages like the following aren't very helpful: | ||
* "not needed" -> uhm, what is not needed? | * "not needed" -> uhm, what is not needed? | ||
| Line 62: | Line 77: | ||
Don't be too verbose in your message either. You don't have to tell people what the next step in the grand scheme is in a long paragraph. | Don't be too verbose in your message either. You don't have to tell people what the next step in the grand scheme is in a long paragraph. | ||
== Documentation == | |||
Commits and PR should follow our documentation guidelines as part of the submission process. Documentation falls under two categories: User facing and Developer related | |||
=== User facing === | |||
Any user facing changes must also have proper documentation. These commits fall under the '''DOCS''' subsystem. For information on how to commit and build our docs portal can be found at the [[Developer Central]] | |||
== More | Example of changes that require accompanying documentation: | ||
* '''GUI''' - New features, text changes of existing UI elements, addition of UI options / elements, Engine/Game specific settings | |||
* '''INI''' - Added keys and groups | |||
* '''Keymap''' - New keymaps, or change to existing keymaps | |||
* '''Ports''' - New ports, changes to documented ports | |||
* '''Features''' - Brand new features, for example Cloud Integratio | |||
=== Developer changes: === | |||
Any code contributions to '''common''' code outside of '''engines''' and '''backends''' must be accompanied with proper doxygen style comments. This includes but not limited to: | |||
* New methods | |||
* Signature changes | |||
* Changes to documented and undocumented methods | |||
* New classes | |||
Documentation can be committed separately under the '''DOXYGEN''' subsystem. | |||
Code contributions in engines and backends do not have a hard requirement for doxygen style comments, but they are encouraged. | |||
== More to read == | |||
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html | http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html | ||
| Line 80: | Line 117: | ||
* Avoid committing style or whitespace fixes and functionality fixes in one go. It makes merging difficult, and also makes it harder to understand just what functional changes were made. In the case of documentation files, it can make the job of the translation teams more complicated, as it becomes difficult for them to determine exactly what content changes need to be translated. | * Avoid committing style or whitespace fixes and functionality fixes in one go. It makes merging difficult, and also makes it harder to understand just what functional changes were made. In the case of documentation files, it can make the job of the translation teams more complicated, as it becomes difficult for them to determine exactly what content changes need to be translated. | ||
* Avoid committing changes to multiple files in one go with a generic, vague message. Instead, commit each file (or small, related groups of files) with tailored commit messages. | * Avoid committing changes to multiple files in one go with a generic, vague message. Instead, commit each file (or small, related groups of files) with tailored commit messages. | ||