Commit messages

From Koha Wiki
Jump to navigation Jump to search

When you submit code to the project (using Git) please write useful commit messages. On this page you will find an example commit message, as well as a detailed analysis of what's in it.

Good commit messages

    Bug 9070: Authority searches in auth_finder error out
    
    When using authority records imported into Koha from elsewhere, you
    can get an error like:
        Can't use string ("HASH(0xbc6c{30)") as a HASH ref while "strict refs" in use at /usr/share/koha/lib/C4/AuthoritiesMarc.pm line 363.
    in authorities/auth_finder.pl. This patch fixes that error.
    
    To test:
    1) You will need records imported from elsewhere.
    2) Use the authority control plugin in a bib record to search for one of
       those headings.
    3) Observe you get a nasty error.
    4) Apply patch.
    5) Repeat step 2.
    6) Observe the error is gone.
    7) Sign off.

    Sponsored-by: The Library

    Signed-off-by: Magnus Enger <magnus@...>
    Works as advertised. No warning about "defined(%hash) is deprecated"
    under perl v5.10.1.

Subject line

    Bug 9070: Authority searches in auth_finder error out

The first line of a commit message is the subject. The subject should *always* provide a bug number and a brief description, ideally in 50 characters or fewer.

The bug number must be the very first thing on the line, and be formatted like "Bug 1234: "

Bug/feature description

    When using authority records imported into Koha from elsewhere, you
    can get an error like:
        Can't use string ("HASH(0xbc6c{30)") as a HASH ref while "strict refs" in use at /usr/share/koha/lib/C4/AuthoritiesMarc.pm line 363.
    in authorities/auth_finder.pl. This patch fixes that error.

A description of what problem the bug addresses, or what feature it adds. If it's a very simple patch, this might be covered by the subject line, but probably not. Whenever possible, this should be wrapped at 72 characters, though error messages and file paths/URLs will often have to go longer.

Documentation notes

If there are particular things that the documentation team should know about, a note to that effect should be included in the commit message. For example, a list of system preferences, new permissions, and new cron jobs might be appropriate, depending on the patch.

Test plan

    To test:
    1) You will need records imported from elsewhere.
    2) Use the authority control plugin in a bib record to search for one of
       those headings.
    3) Observe you get a nasty error.
    4) Apply patch.
    5) Repeat step 2.
    6) Observe the error is gone.
    7) Sign off.

A detailed test plan, with a clear heading. It should include enough detail that it could be followed by someone familiar with Koha who does not use whatever section of the code you are editing. A test plan with forty steps may seem daunting, but it really isn't. It's considerate. Ideally, the test plan should cover how to confirm the bug *and* how to confirm that the patch fixed the problem, if the two procedures are not entirely identical (for example, if your patch involves Zebra configuration changes, note which files need to be updated; I favor a tl;dr section which includes information of this sort). Again, this should be wrapped at 72 characters where possible, and use a hanging indent..

Including a "sign off" step is unnecessary, of course. 

    Sponsored-by: The Library
    Sponsored-by: The Sponsor <https://sponsor-website.com>

If your patch was sponsored by an institution that wants credit in the release notes, add a Sponsored-by: line with the name of the institution as you would like it to appear.

Signoffs/QA Signoffs

    Signed-off-by: Magnus Enger <magnus@...>
    Works as advertised. No warning about "defined(%hash) is deprecated"
    under perl v5.10.1.

When signing off or QAing a patch, you should add your Signed-off-by: line by using `git commit -s` and add a (brief) description of what you tested.

Other trailers

You are encouraged to use the following trailers to add more information to commits:

  • Co-authored-by, if the patch has multiple authors
  • Mentored-by, if the patch author has been mentored by someone
  • Reported-by, the bug reporter
  • Thanks-to, to credit people that do not fit in previous categories
  • Rescued-by, to credit people who have rescued a patch

Example: 

Bug 30000: Destroy the death star

Reported-by: Leia Organa <leia@rebellion.org>
Co-authored-by: Han Solo <han.solo@millennium.ship>
Co-authored-by: Chewbacca <chewie@wookie.net>
Mentored-by: Yoda <yoda@dagobah.planet>
Thanks-to: The Rebellion for being very supportive

Here are a few git aliases that might help you writing these trailer lines: 

[alias]
    ; Generic aliases
    trailer-add = "!f(){ GIT_EDITOR=\"git interpret-trailers --trailer='$1: $2' --in-place\" git commit --amend; }; f"
    trailer-add-me = "!f() { git trailer-add \"$1\" \"$(git config user.name) <$(git config user.email)>\"; }; f"

    ; Specific aliases to avoid mistyping the token part (the part before ':')
    co-authored-by = "!git trailer-add Co-authored-by"
    co-authored-by-me = "!git trailer-add-me Co-authored-by"

You can use them to modify the last commit like this: 

git trailer-add Mentored-by "Yoda <yoda@dagobah.planet>"
git trailer-add-me Reported-by
git co-authored-by "Han Solo <han.solo@millennium.ship>"
git co-authored-by-me

Or if you want to add these trailers to multiple commits at once: 

git rebase -x 'git trailer-add "Thanks-to" "everybody involved in this bug"' HEAD~X   # where X is the number of commits you want to rewrite

A gentle reminder

If there is information needed for testing a patch, it should be in your commit message, not just in comments on the bug. If -- in the course of looking at a patch for pushing to main -- I follow the test instructions in a commit message and the patch does not work, or if I can't quickly divine what the patch is about, I will ask for clarification and move on. If I don't understand, I have no reason to think that I will start to understand after spending more time on the issue, and any time I spend not understanding one patch is time during which no other patch is getting reviewed.

Follow-ups

In order to standardize the commit messages, here is how the follow-up must be written: "(follow-up)" or "(QA follow-up)", followed by a description of the patch (not [follow-up], (followup), [QA Followup], etc.)

For instance: 

 Bug 12345: (follow-up) same commit message as before

or 

 Bug 12345: (follow-up) This was missing

And QA follow-ups (added by QA team member): 

 Bug 12345: (QA follow-up) Add/remove this stuff

Formatting

Ideally we are shooting for a subject which is 50 characters or fewer, and the body of commit messages should be wrapped at 72 characters. A good blog post on this topic can be found at http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

Examples

Good commit messages

The bug report title must shortly tell what the *bug* is, whereas the commit messages must tell what the *fix* does.

Sometimes they are not related at all.

For instance, for bugs: Bug report titles / commit messages:

  • Feature X is not working correctly when Y is enabled / Fix Feature X when Y is enabled
  • Click on button 'Save' does not save the record / Make sure behaviour is correct when button is clicked
  • Saving stuff on screen X does not work
  • This script generates error logs / Remove warnings from this script
  • Mistake is misspelled in this script / Correct spelling of mistake in this script
  • Behavior X no longer work as expected / Fix regression on behaviour X

For enhancements / new feature, we could have the same title and commit messages in some cases:

Bug report titles / commit messages:

  • Add new awesome feature / Add new awesome feature
  • Add the ability to do that / Add the ability to do that
  • Code is duplicated in method Koha::Stuff->method / Remove code duplication in method Koha::Stuff->method

Bad commit messages

The worst kind of commit message is an inaccurate one, followed closely by an empty one.

The following commit messages are wrong:

  • Start with: "Bug 12345 - "
  • "This button did not work" (that is not what the patch actually does)
  • "Address comment #42" (be more precise!)
  • "follow-up" (follow-up of what?)
  • "QA follow-up"

Checklist before submission

  • no need to add new subroutines to C4 namespace, it will be rejected
  • no need to add or update methods without test coverage, it will be rejected


Developer handbook

Retrieved from "https://wiki.koha-community.org/w/index.php?title=Commit_messages&oldid=34102"