Commit Digest/Guidelines: Difference between revisions

From KDE Community Wiki
m (→‎Inclusion Guidelines: include first contribs from new devs)
 
(6 intermediate revisions by 2 users not shown)
Line 4: Line 4:


Composing better guidelines for reviewing and classifying commits for the Commit Digest. The source of the original inclusion guidelines can be found [https://github.com/dannyakakong/Enzyme/wiki/Inclusion-Guidelines-%28Review%29 here].
Composing better guidelines for reviewing and classifying commits for the Commit Digest. The source of the original inclusion guidelines can be found [https://github.com/dannyakakong/Enzyme/wiki/Inclusion-Guidelines-%28Review%29 here].
==Overview==
The commits are first reviewed and triaged (see section ''Inclusion Guideline'' below) by the reviewer team and then classified by the classifier team. The Editors will then write a short intro summary (synopsis) and ready the digest for publication.
Developers are encouraged to write commit messages which are easy to understand by non-developers. They are also encouraged to use the ''"Digest"'' tag to suggest inclusion of the commit in the digest and give a brief summary of the effect of the commit.
At the KDE summit [http://akademy.kde.org/program Akadamy 2012] Marta Rybczyńska held a talk ''"How to make your commit seen"'' concerning the creation workflow of the digest and the interaction between developers and the digest team.
The [http://files.kde.org/akademy/2012/slides/How_to_make_your_commit_seen_-_Marta_Rybczynska.pdf presentation] (PDF) and the [http://files.kde.org/akademy/2012/videos/How_to_make_your_commit_seen_-_Marta_Rybczynska.m4v talk] (M4V) are available.


==Inclusion Guidelines==
==Inclusion Guidelines==
Line 9: Line 18:
On average about 5% of all commits in a week will be included in the commit digest (also known as the '''5% rule'''). This means that those who review the commits should choose carefully which commits should be included. To get a feeling for which messages should or should not be included, it could be useful to read existing Commit Digests to see what kind of commits they contain. There is no definitive set of rules when to include a commit message and when not. However, the following gives some guidelines.
On average about 5% of all commits in a week will be included in the commit digest (also known as the '''5% rule'''). This means that those who review the commits should choose carefully which commits should be included. To get a feeling for which messages should or should not be included, it could be useful to read existing Commit Digests to see what kind of commits they contain. There is no definitive set of rules when to include a commit message and when not. However, the following gives some guidelines.


Hint: you can always investigate a commit further by clicking on the commit id. This gives you helpful details about which files were modified and allows for a line by line comparison of the source code.
((Hint: you can always investigate a commit further by clicking on the commit id. This gives you helpful details about which files were modified and allows for a line by line comparison of the source code. NOT WORKING WITH GIT COMMITS))


===In general include commits in the Digest if they ===
===In general include commits in the Digest if they ===
Line 15: Line 24:
# fix a bug (if you’re certain the bug is minor or unimportant you can exclude them), are an optimization or add a feature (for the user); this is in particular the case when there is a "broken box" symbol in the lower right corner of the message,
# fix a bug (if you’re certain the bug is minor or unimportant you can exclude them), are an optimization or add a feature (for the user); this is in particular the case when there is a "broken box" symbol in the lower right corner of the message,
# new or significantly improved icons.
# new or significantly improved icons.
# are the first submission of a new contributor.


===Generally do not include messages if they===
===Generally do ''not'' include messages if they===
# are back-ports, i.e., the message basically appears two times (then do not include the backported/branch version, only include the version in trunk),
# are back-ports, i.e., the message basically appears two times (then do not include the backported/branch version, only include the version in trunk),
# are refactorings, internal clean-ups, additions to the API (unless they seem to be interesting, e.g., a whole new API for Nepomuk),
# are refactorings, internal clean-ups, additions to the API (unless they seem to be interesting, e.g., a whole new API for Nepomuk),
Line 23: Line 33:
# are fixes or additions to a localization (without bug number)
# are fixes or additions to a localization (without bug number)
# introduce trivial changes such as the resizing of a line-edit or changes in button placement.
# introduce trivial changes such as the resizing of a line-edit or changes in button placement.
NOTE: Make sure to use the "save" button in the lower right corner if you intend to save your changes. If you leave the review page without saving, the changes will be lost.


=== Specific guidelines ===
=== Specific guidelines ===
Line 31: Line 44:
# Commits which increase the version number should be excluded.
# Commits which increase the version number should be excluded.
# Reverts of commits can occur, often because a recent commit introduced a bug. Because the reverted commit was so recent very few people will the existence of the bug, so in general reverts of commits should be excluded unless there is a good reason not to do so.
# Reverts of commits can occur, often because a recent commit introduced a bug. Because the reverted commit was so recent very few people will the existence of the bug, so in general reverts of commits should be excluded unless there is a good reason not to do so.
# Since the migration to Git commits simply mentioning a branch merge from branch X to the master branch, where the code lives that will eventually be released, but it can also be the other way around often occur. Developers use branches to work on the code in isolation, so that the code living in master which is going to be released is not disturbed with bugs and such. These should not be included because they simply fail the criterium that the commit should be interesting.
# Since the migration to Git commits simply mentioning a branch merge from branch X to the master branch, where the code lives that will eventually be released, but it can also be the other way around often occur. Developers use branches to work on the code in isolation, so that the code living in master which is going to be released is not disturbed with bugs and such. These should not be included because they simply fail the criterium that the commit should be interesting. In other words: exclude merge commits. Break the rule if their extraordinary significance is mentioned.  
# KOffice split into two projects, the old KOffice and the new Calligra, but some developers work on both projects. As a result identical commits to both projects can occur. We do not want to include information twice, but we don’t want any bias for one of the two projects either. So simply include the first commit to any of the two projects, and ignore the following identical commit.
# KOffice split into two projects, the old KOffice and the new Calligra, but some developers work on both projects. As a result identical commits to both projects can occur. We do not want to include information twice, but we don’t want any bias for one of the two projects either. So simply include the first commit to any of the two projects, and ignore the following identical commit.



Latest revision as of 17:35, 9 November 2012

<< back

Goal of this page

Composing better guidelines for reviewing and classifying commits for the Commit Digest. The source of the original inclusion guidelines can be found here.


Overview

The commits are first reviewed and triaged (see section Inclusion Guideline below) by the reviewer team and then classified by the classifier team. The Editors will then write a short intro summary (synopsis) and ready the digest for publication.

Developers are encouraged to write commit messages which are easy to understand by non-developers. They are also encouraged to use the "Digest" tag to suggest inclusion of the commit in the digest and give a brief summary of the effect of the commit.

At the KDE summit Akadamy 2012 Marta Rybczyńska held a talk "How to make your commit seen" concerning the creation workflow of the digest and the interaction between developers and the digest team. The presentation (PDF) and the talk (M4V) are available.

Inclusion Guidelines

On average about 5% of all commits in a week will be included in the commit digest (also known as the 5% rule). This means that those who review the commits should choose carefully which commits should be included. To get a feeling for which messages should or should not be included, it could be useful to read existing Commit Digests to see what kind of commits they contain. There is no definitive set of rules when to include a commit message and when not. However, the following gives some guidelines.

((Hint: you can always investigate a commit further by clicking on the commit id. This gives you helpful details about which files were modified and allows for a line by line comparison of the source code. NOT WORKING WITH GIT COMMITS))

In general include commits in the Digest if they

  1. are interesting (in general these messages have a longer text describing them),
  2. fix a bug (if you’re certain the bug is minor or unimportant you can exclude them), are an optimization or add a feature (for the user); this is in particular the case when there is a "broken box" symbol in the lower right corner of the message,
  3. new or significantly improved icons.
  4. are the first submission of a new contributor.

Generally do not include messages if they

  1. are back-ports, i.e., the message basically appears two times (then do not include the backported/branch version, only include the version in trunk),
  2. are refactorings, internal clean-ups, additions to the API (unless they seem to be interesting, e.g., a whole new API for Nepomuk),
  3. are about CMake or desktop files, documentation changes (without bug number),
  4. are about unit tests ( http://en.wikipedia.org/wiki/Unit_testing ),
  5. are fixes or additions to a localization (without bug number)
  6. introduce trivial changes such as the resizing of a line-edit or changes in button placement.


NOTE: Make sure to use the "save" button in the lower right corner if you intend to save your changes. If you leave the review page without saving, the changes will be lost.

Specific guidelines

  1. Commits to the http://websvn.kde.org/trunk/kdesupport/emerge/ directory are mostly related to build fixes for KDE on Windows. In general they should not be included unless they are to be judged important based on their description.
  2. Often you can see commits related to accounts of developers taking place in http://websvn.kde.org/trunk/kde-common/accounts/, these commits should be excluded.
  3. If work on new icons is being done there can be a high frequency of commits related to them. Only include the most relevant commits stating for example that all 32x32 icons are done or if the last of the icons are committed. A maximum of three of those repeated messages are desirable for a Commit Digest. Because it can be difficult to see in advance if an icon related commit will be the last one for a specific week, the best compromise is to select the last commit in a series of commits related to icons.
  4. Commits which change the contents of the files called NEWS, AUTHORS, Changelog, README and TODO should be excluded.
  5. Commits which increase the version number should be excluded.
  6. Reverts of commits can occur, often because a recent commit introduced a bug. Because the reverted commit was so recent very few people will the existence of the bug, so in general reverts of commits should be excluded unless there is a good reason not to do so.
  7. Since the migration to Git commits simply mentioning a branch merge from branch X to the master branch, where the code lives that will eventually be released, but it can also be the other way around often occur. Developers use branches to work on the code in isolation, so that the code living in master which is going to be released is not disturbed with bugs and such. These should not be included because they simply fail the criterium that the commit should be interesting. In other words: exclude merge commits. Break the rule if their extraordinary significance is mentioned.
  8. KOffice split into two projects, the old KOffice and the new Calligra, but some developers work on both projects. As a result identical commits to both projects can occur. We do not want to include information twice, but we don’t want any bias for one of the two projects either. So simply include the first commit to any of the two projects, and ignore the following identical commit.

Classification Guidelines

Bug Fixes: this category contains all commits which mention a bug number in the message (if the bug refers to a bug and is not a wishlist item (*)). It also contains commits which fix something according to the comment but do not have a bug report attached (these commits need to be important though, remember the 5% rule!).

Features: comprises all commits that either mention a bug number (which refers to a wishlist item (*)) or any commits which are about the addition of a interesting or important feature.

Optimize: this category is only meant for commits that make things either faster, use less memory, or in rare cases, make the interface more streamlined ("now this can be done in 2 clicks instead of 10").

Other: this category is for commits that don't really fit anywhere else: for example for commits that are really interesting or funny even if they otherwise wouldn't be important (like someone writing in the message that they learned something really important, etc.).

Specific guidelines:

  • New icons and other new artwork should be classified under the area 'User Interface' and under the type 'Feature'.


(*) this can be done by looking at the bugs severity in the summary box: "normal", "major", "critical" refers to a bug (e.g., [1]), while "wishlist" refers to a feature (e.g., [2]). If the severity is "minor" or "trivial" [3] it is in general not recommended to include the respective commit.