Upgrade to Pro — share decks privately, control downloads, hide ads and more …

The Elements of Good Commit Messages

The Elements of Good Commit Messages

Patrick Tsai

October 22, 2015
Tweet

Other Decks in Programming

Transcript

  1. The Elements of Good
    Commit Messages
    Patrick Tsai
    1

    View full-size slide

  2. Summerized from Examples
    of Various Open-Source
    Projects
    2

    View full-size slide

  3. 3 Principles
    5 Elements
    3

    View full-size slide

  4. Let's Start from Some
    Typical Scenarios
    4

    View full-size slide

  5. Scenario: Fix Typo
    encoding/gob: fix typo in documentation
    4 Oneliner is good enough.
    path: fix a typo in documentation of Split
    4 Better if you mention where.
    5

    View full-size slide

  6. Scenario: Update Documentation
    crypt/rand: update docs for Linux
    Update the docs to explain the code added in
    commit 67e1d400.
    4 A little more details won't harm.
    6

    View full-size slide

  7. Scenario: Fix Coding Style
    fs: ext3: super: fixed a space coding style issue
    4 Good enough for a trivial omission.
    [Compiler] use Art indentation standard
    First of several CLs to bring code closer to alignment with Art and LLVM
    standards. Move to 2-space indenting. Sticking with 80-col line
    length (which LLVM apparently also wants). LLVM also prefers camel
    case names, so keeping Dalvik convention there as well (for now).
    4 If it is worthy to explain more. Do it.
    7

    View full-size slide

  8. Principle #1
    Don't explain what is obvious
    8

    View full-size slide

  9. Scenario: Rename Something
    cmd/internal/gc, cmd/[56789]g: rename stackcopy to blockcopy
    To avoid confusion with the runtime concept of copying stack.
    4 Explain why rename is necessary.
    9

    View full-size slide

  10. Scenario: Remove Something
    Remove use of '_' variable name
    '_' may become invalid in future versions of the Java language
    and generates warnings in OpenJDK 8.
    4 Explain why it should be removed.
    runtime: remove unneeded C header files
    4 Oneliner is good again if you can explain why briefly
    in title.
    10

    View full-size slide

  11. Scenario: Disable Something
    net/http: disable new flaky TestTransportCancelBeforeResponseHeaders test
    I'll rewrite this later. It's apparently dependent on scheduling order.
    The earlier fix in git rev 9d56c181 seems fine, though.
    4 Explain why it should be disabled
    11

    View full-size slide

  12. Scenario: Update Something
    Update instructions in IPowerManager.aidl.
    Clarify IPowerManager.aidl's instructions for keeping the
    C++ implementation in sync.
    4 Explain why the instructions need to be updated.
    12

    View full-size slide

  13. Scenario: Reorganize Something
    writeback: reorganize mm/backing-dev.c
    Move wb_shutdown(), bdi_register(), bdi_register_dev(),
    bdi_prune_sb(), bdi_remove_from_list() and bdi_unregister() so that
    init / exit functions are grouped together. This w ill make updating
    init / exit paths for cgroup writeback support easier.
    4 Explain why it should be reorganized.
    13

    View full-size slide

  14. Did You See the Pattern?
    14

    View full-size slide

  15. Element #1: Motivation
    Explain why is the change necessary
    15

    View full-size slide

  16. Scenario: Fix Build Error/Warning #1
    liblink: fix arm build errors
    This was supposed to be in CL 135490044
    but got lost in a transfer from machine to machine.
    4 Explain why the build error was not catched by
    youself.
    16

    View full-size slide

  17. Scenario: Fix Build Error/Warning #2
    usb: phy: fix phy-qcom-8x16-usb build
    Fix build errors that happen when USB_QCOM_8X16_PHY=y and EXTCON=m:
    drivers/built-in.o: In function `phy_8x16_init':
    phy-qcom-8x16-usb.c:(.text+0x86ef4): undefined reference to `extcon_get_cable_state'
    4 Include error message help reader to understand
    the problem.
    4 Describe the conditions causing the build error.
    17

    View full-size slide

  18. Scenario: Fix a Bug #1
    Fix incorrect android.telecom.Call.Details equality check.
    The android.telecom.Call.Details class provides its own equals
    implementation. Recently added in M is to also check if the mExtras
    and mIntentExtras are different. Unfortunately, Bundles do not implement
    equals. As a result when Telecom calls are parceled and sent to the
    InCallServices, this means that the internalUpdate method will always
    assume that the Details of a call have changed, even if they have not.
    This was causing a LOT of extra calls to onUpdate in the InCall UI (2x the
    amount). Although there is still room for improvement in the number of
    callbacks from Telecom, this fix prevents a pretty significant regression
    on that front.
    4 Explain why this bug happened in M.
    18

    View full-size slide

  19. Scenario: Fix a Bug #2
    Fix race conditions for camera prewarm service
    When the unbind request came in before the service was actually
    bound, we dropped the unbind request because mPrewarmBound was still
    false. Fix that by tracking whether a bind is pending and if a unbind
    event comes in during that time, set another flag to unbind it
    directly again when the service is actually bound. In addition, don't
    allow binding again if any of the previous events are still pending.
    4 Explain why the race condition happened, which is
    hard to see in diff.
    19

    View full-size slide

  20. Element #2: Context
    Explain why the issue happened
    20

    View full-size slide

  21. Scenario: Performance Tuning
    iwlwifi: mvm: limit aggregation size in low latency to 6
    This is a tradeoff between enabling better throughput for
    bursty traffic and low latency. The number 6 was found to be
    a good tradeoff for the Miracast use case which is the
    major use case for low latency.
    4 Good to explain the rationale behind the magic
    number.
    21

    View full-size slide

  22. Scenario: Improve Something
    go/parser: slightly improved error message by adding hint
    It's difficult to make this much better w/o much
    more effort. This is a rare case and probably not
    worth it.
    4 Explain why only small improvement was made.
    22

    View full-size slide

  23. Most problems have multiple possible
    solutions.
    Writing down why you want to solve a problem in a
    particular way forces you to reflect and think deeply
    about why you did that piece of work, and this puts
    you in a better mindset to create better solutions in
    the first place.
    23

    View full-size slide

  24. Element #3: Rationale
    Explain why you solve it in a particular
    way
    24

    View full-size slide

  25. Principle #2
    Prefer why to what/how
    25

    View full-size slide

  26. Principle #3
    Describe non-obvious what/how
    26

    View full-size slide

  27. Typically Obvious:
    4 What a change solves
    4 How it achieves its goal
    27

    View full-size slide

  28. runtime: update gcController.scanWork regularly
    Currently, gcController.scanWork is updated as lazily as possible
    since it is only read at the end of the GC cycle. We're about to read
    it during the GC cycle to improve the assist ratio revisions, so
    modify gcDrain* to regularly flush to gcController.scanWork in much
    the same way as we regularly flush to gcController.bgScanCredit.
    One consequence of this is that it's difficult to keep gcw.scanWork
    monotonic, so we give up on that and simply return the amount of scan
    work done by gcDrainN rather than calculating it in the caller.
    4 Describe rationale.
    4 Provide high-level description of new approach.
    4 Describe side effect.
    28

    View full-size slide

  29. runtime: rewrite addb/subtractb to be simpler to compile; introduce add1, subtract1
    This reduces the depth of the inlining at a particular call site.
    The inliner introduces many temporary variables, and the compiler can do
    a better job with fewer. Being verbose in the bodies of these helper functions
    seems like a reasonable tradeoff: the uses are still just as readable, and
    they run faster in some important cases.
    4 Describe tradeoffs.
    29

    View full-size slide

  30. runtime: add copy of math.sqrt for use by arm softfloat
    If it's not used (such as on other systems or if softfloat
    is disabled) the linker will discard it.
    The alternative is to teach cmd/go that every binary
    depends on math implicitly on arm. I started down that
    path but it's too scary. If we're going to get dependencies
    right we should get dependencies right.
    4 Describe alternatives you have tried.
    30

    View full-size slide

  31. net/http: fix server/transport data race when sharing the request body
    Introduced in https://go-review.googlesource.com/12865 (git rev c2db5f4).
    This fix doesn't add any new lock acquistions: it just moves the
    existing one taken by the unreadDataSize method and moves it out
    wider.
    It became flaky at rev c2db5f4, but now reliably passes again:
    $ go test -v -race -run=TestTransportAndServerSharedBodyRace -count=100 net/http
    4 Good to list tests you have done to verify the fix.
    31

    View full-size slide

  32. runtime: fix software FP regs corruption when emulating SQRT on ARM
    When emulating ARM FSQRT instruction, the sqrt function itself
    should not use any floating point arithmetics, otherwise it will
    clobber the user software FP registers.
    Fortunately, the sqrt function only uses floating point instructions
    to test for corner cases, so it's easy to make that function does
    all it job using pure integer arithmetic only. I've verified that
    after this change, runtime.stepflt and runtime.sqrt doesn't contain
    any call to _sfloat. (Perhaps we should add //go:nosfloat to make
    the compiler enforce this?)
    4 Provide high-level description of new approach.
    4 Explain how you verified the fix.
    32

    View full-size slide

  33. os, syscall: revert Yosemite readdir workaround
    Reverts https://golang.org/cl/119530044 (OS X 10.10 Yosemite beta
    14A299l workaround), since it was fixed in the final Yosemite release.
    I verified that the C program http://swtch.com/~rsc/readdirbug.c
    passes on Yosemite.
    Adds a new test to the os package too, to verify that reading a
    regular file as a directory fails.
    4 Good to list test case you used to verify the fix.
    33

    View full-size slide

  34. math: optimize ceil/floor functions on amd64
    Use SSE 4.1 rounding instruction to perform rounding
    Results (haswell):
    name old time/op new time/op delta
    Floor-48 2.71ns ± 0% 1.87ns ± 1% -31.17% (p=0.000 n=16+19)
    Ceil-48 3.09ns ± 3% 2.16ns ± 0% -30.16% (p=0.000 n=19+12)
    4 Use numbers to back up your improvements.
    34

    View full-size slide

  35. Non-Obvious Details
    4 What is the side effect.
    4 What difficulties or tradeoffs made.
    4 What are alternative solutions considered.
    4 What tests you have done to verify the change.
    4 What benchmark you have done for performance
    optimization.
    35

    View full-size slide

  36. Element #4: Details
    Explain non-obvious what/how
    36

    View full-size slide

  37. Scenario: Improve Something
    math: improved Sin, Cos and Tan precision for very large arguments
    The existing implementation has poor numerical properties for
    large arguments, so use the McGillicutty algorithm to improve
    accuracy above 1e10.
    The algorithm is described at http://wikipedia.org/wiki/McGillicutty_Algorithm
    4 Good to reference online resources and relevant
    spec.
    37

    View full-size slide

  38. archive/zip: fix size value in ZIP64 end central directory record
    Section 4.3.14.1 of the ZIP file format
    spec (https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT) says,
    The value stored into the "size of zip64 end of central directory
    record" should be the size of the remaining record and should not
    include the leading 12 bytes.
    We were previously writing the full size, including the 12 bytes.
    4 Provide reference to ZIP file format.
    38

    View full-size slide

  39. Elements #5: Reference
    Provide link to additional info
    39

    View full-size slide

  40. References
    4 Bug number: provide motivation and context
    4 Code review URL/Number: provide details
    4 Functional spec: provide rationale
    4 Standards: provide rationale
    4 Online reference: provide rationale
    40

    View full-size slide

  41. 3 Principles
    Don't explain what is obvious
    Prefer why to what/how
    Describe non-obvious what/how
    41

    View full-size slide

  42. 5 Elements
    Motivation, Context, Rationale
    Details, Reference
    42

    View full-size slide

  43. Some Hints
    4 It’s perfectly OK for your commit message to be
    longer than your commit.
    4 It’s perfectly OK to spend more time crafting your
    commit message than writing the code for your
    commit.
    4 Solve Only One Problem per patch.
    44

    View full-size slide

  44. Don't follow rules bindly.
    Use your own judgement.
    45

    View full-size slide

  45. Further Reading
    4 How to Get Your Change Into the Linux Kernel
    4 The secret to great commit messages
    4 Writing Good Commit Messages
    46

    View full-size slide