The Elements of Good Commit Messages

Transcript

1. The Elements of Good Commit Messages Patrick Tsai 1
2. Summerized from Examples of Various Open-Source Projects 2
3. 3 Principles 5 Elements 3
4. Let's Start from Some Typical Scenarios 4
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
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
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
8. Principle #1 Don't explain what is obvious 8
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
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
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
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
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
14. Did You See the Pattern? 14
15. Element #1: Motivation Explain why is the change necessary 15
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
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
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
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
20. Element #2: Context Explain why the issue happened 20
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
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
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
24. Element #3: Rationale Explain why you solve it in a particular way 24
25. Principle #2 Prefer why to what/how 25
26. Principle #3 Describe non-obvious what/how 26
27. Typically Obvious: 4 What a change solves 4 How it achieves its goal 27
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
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
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
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
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
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
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
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
36. Element #4: Details Explain non-obvious what/how 36
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
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
39. Elements #5: Reference Provide link to additional info 39
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
41. 3 Principles Don't explain what is obvious Prefer why to what/how Describe non-obvious what/how 41
42. 5 Elements Motivation, Context, Rationale Details, Reference 42
44. 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
45. Don't follow rules bindly. Use your own judgement. 45
46. 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