git-commit消息怎么写

Posted on | | Visitors:

翻译自Thoughts on improving OpenStack GIT commit practice/history

Git commit 中良好的习惯

本文基于使用git用于代码开发，bug fix及代码review的工程实践经验，比如libvirt、qemu、openstack nova。其他开源工程比如说linux内核、core utils，GNULIB等也遵循一个相对通用的习惯。本文主要用于促进openstack nova工程提升代码质量。虽然“质量”在计算领域是难以衡量的，对某人来说的“漂亮代码”对另一人来说可能意味着“邪恶hack”，但我们能提出一些通用性指导意见来指导我们在工程中发布commit。本文主要从两方面进行阐述：

  1. 代码修改的结构化整合和分割
  2. commit message中的信息应该有哪些

    概要

本文首先强调将代码修改分割为一系列的单个提交，及其良好附加消息的重要性。若这一指导意见能够得到广泛应用，那么Openstack GIT历史质量将会有极大提升。不过这一效果达成既需要有“胡萝卜”也需要有“大棒”，本文主要是作为“胡萝卜”，而那些通过Gerrit进行代码review的人要成为坚守原则的“大棒”。

换句话说，当在Gerrit revie代码时，不仅仅是简单地分析代码正确性，还要Review commit message本身及要求作者提升其质量，发现代码多个修改混合在一起以及要求提交者将它们分割成若干单个commit，及保证空格修改不会和函数修改夹杂在一起，保证代码重构和函数修改是分开的。等等。

可能也有人提到说gerrit对paitch系列化的的处理做的不是很好。这并不是一个避免patch序列化的有效原因。工具要服从开发者需要，而且它们是开源的，更能够提升。但是软件源码是“读多，写少”的，因此最重要的一点在于提升其被社区的众多者维护时提升其可维护性，而不是满足某些永远不再碰那些代码的人的诉求。

以下是关于良好或者不好的实践例子和详细指导。

代码修改的结构化分割

创建一个commit的基本原则在于每个commit有且仅有一个“逻辑修改”：

  1. 代码改变的越少，那么越快越容易review和发现其中的缺点。
  2. 如果某个修改后面发现有缺陷，那么需要回退这个有缺陷的commit，相比于夹杂着其他无关代码修改的情况，这更容易处理。
  3. 当使用GIT bisect工具定位问题时，小的良好定义的修改能够有助于定位哪个代码引入的问题。
  4. 当使用GIT annotate/blame 浏览历史时，小的良好定义的修改能够有助于定位引入问题的代码。

commit禁忌

  1. 夹杂着函数修改和空格修改。空格修改会遮掩重要的函数修改，这会让代码审查者很难正确地发现修改是正确的。

方案:创建2个commit，一个专门用于空格修改，一个用于函数修改。一般来说需要首先做空格修改，但并不是说一定要这样。
2. 两个无关的代码修改混合。这也会让代码审查者很难确定缺陷。如果后续又需要对某个修改进行整理时，它会增加引发bug的风险。
3. 用一个巨大的commit实现一个大的新特性。可能某个feature需要所有修改呈现后才会有效。然而，这并不意味着整个feature需要在单独某个commit中提交。新的feature通常需要重构当前代码，这种情况下最好是将当前代码重构部分和新特性实现部分分开。甚至新写的代码也能分成多个独立模块来进行review。比如说，添加新的APIs/classes的修改能够在自包含commit中。而这有助于代码review。如果整个feature还无法马上merge时，这也允许其他的开发者择优选择小部分代码。额外的APIs和接口实现需要同实际的内部实现分开。这能鼓励作者&审查者考虑通用API/RPC设计，而不是选择一个当前容易的设计。

基本规则在于：
如果代码修改能够分为patch/commits序列，那就分开。少不是多，多才是多。

错误示范

例1

1
2
3
4
5
6
7
8
9
10
11
12
13
14
commit ae878fc8b9761d099a4145617e4a48cbeb390623
Author: [removed]
Date: Fri Jun 1 01:44:02 2012 +0000

Refactor libvirt create calls

* minimizes duplicated code for create
* makes wait_for_destroy happen on shutdown instead of undefine
* allows for destruction of an instance while leaving the domain
* uses reset for hard reboot instead of create/destroy
* makes resume_host_state use new methods instead of hard_reboot
* makes rescue/unrescue not use hard reboot to recreate domain

Change-Id: I2072f93ad6c889d534b04009671147af653048e7

这个commit至少可以分为两个独立的修改:

  • “hard_reboot”方法改为使用心得”reset”API
  • 内部driver不再使用“hard_reboot”

那么问题在哪里呢？首先这些修改没有明确说明为什么这些代码需要同时修改。第一个commit能够包含哪些停止调用”hard_reboot”的代码。第二个提交能够重写”hard_reboot”的实现。

由于重构时在很多代码使用libvirt “reset”方法，审查者没有发现这引入一个新的libvirt API版本。这个commit后面很快就被发现是一个“罪人”，但由于其夹杂着大量无关修改导致无法进行关键性revert。

例2

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
commit e0540dfed1c1276106105aea8d5765356961ef3d
Author: [removed]
Date: Wed May 16 15:17:53 2012 +0400

blueprint lvm-disk-images

Add ability to use LVM volumes for VM disks.

Implements LVM disks support for libvirt driver.

VM disks will be stored on LVM volumes in volume group
specified by `libvirt_images_volume_group` option.
Another option `libvirt_local_images_type` specify which storage
type will be used. Supported values are `raw`, `lvm`, `qcow2`,
`default`. If `libvirt_local_images_type` = `default`, usual
logic with `use_cow_images` flag is used.
Boolean option `libvirt_sparse_logical_volumes` controls which type
of logical volumes will be created (sparsed with virtualsize or
usual logical volumes with full space allocation). Default value
for this option is `False`.

Commit introduce three classes: `Raw`, `Qcow2` and `Lvm`. They contain
image creation logic, that was stored in
`LibvirtConnection._cache_image` and `libvirt_info` methods,
that produce right `LibvirtGuestConfigDisk` configurations for
libvirt. `Backend` class choose which image type to use.

Change-Id: I0d01cb7d2fd67de2565b8d45d34f7846ad4112c2

表面上看这引入了一个新的特性，使用单独commit也是合理的。但仔细看这个patch的话，它明显混杂了很多代码重构和新的LVM特性代码，这导致很难识别在支持QCow2/Raw镜像时可能的回退。该commit至少需要分为4个单独的commits:

  1. Replace the ‘use_cow_images’ config FLAG with the new FLAG ‘libvirt_local_images_type’, with back-compat code for support of legacy ‘use_cow_images’ FLAG
  2. Creation of internal “Image” class and subclasses for Raw & QCow2 image type impls.
  3. Refactor libvirt driver to replace raw/qcow2 image management code, with calls to the new “Image” class APIs
  4. Introduce the new “LVM” Image class implementation

正确示范

例子1

1
2
3
4
5
6
7
8
9
10
11
commit 3114a97ba188895daff4a3d337b2c73855d4632d
Author: [removed]
Date: Mon Jun 11 17:16:10 2012 +0100

Update default policies for KVM guest PIT & RTC timers

commit 573ada525b8a7384398a8d7d5f094f343555df56
Author: [removed]
Date: Tue May 1 17:09:32 2012 +0100

Add support for configuring libvirt VM clock and timers

这两个修改提供配置KVM guest timer的支持，而引入创建libvirt XML 配置的API明显区别于使用新的API的KVM guest创建策略。

例子2

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
commit 62bea64940cf629829e2945255cc34903f310115
Author: [removed]
Date: Fri Jun 1 14:49:42 2012 -0400

Add a comment to rpc.queue_get_for().

Change-Id: Ifa7d648e9b33ad2416236dc6966527c257baaf88

commit cf2b87347cd801112f89552a78efabb92a63bac6
Author: [removed]
Date: Wed May 30 14:57:03 2012 -0400

Add shared_storage_test methods to compute rpcapi.
...snip...
Add get_instance_disk_info to the compute rpcapi.
...snip...
Add remove_volume_connection to the compute rpcapi.
...snip...
Add compare_cpu to the compute rpcapi.
...snip...
Add get_console_topic() to the compute rpcapi.
...snip...
Add refresh_provider_fw_rules() to compute rpcapi.
...many more commits...

这个commit序列重构了整个nova的RPC API层来允许可插拔消息实现。对于这种大的函数代码修改，只有分为一系列的单个commits才能真正有利于做代码审查，及便于跟踪/识别过程代码回退。

commit message中的信息

同代码修改同等重要的是commit消息描述。写commit消息时，这些事情需要牢记:

  1. 不要假定审查者理解根本问题是什么。当阅读bug报告时，在很多的反复评论之后，通常会变得混轮，commit消息需要清楚说明原始问题是什么。bug仅仅是识别问题的所感兴趣的历史背景，要尽可能使审查patch的时候就能确定其正确性而无需阅读bug提票。

  2. 不要假定审查者能够访问外部网络或者服务。我们不能保证他们能够访问在线bug跟踪系统，分布式SCM核心在于无须“在线”就能获取所有所需信息，commit消息应该整个是自包含的。

  3. 不要假定代码是不言自明的。对某人它是不言自明的，但是对于另一个人就可能是混乱不堪的。一定要记录原始问题是什么以及其如何解决（除了明显的打字错误和空白代码）。

  4. 描述为何要做这么改。一个普遍性错误在于仅仅记录代码怎么改的，而没有说明开发者选择这么改。要用尽量描述整个代码架构，特别是对于大的修改更要详细记录背后的目的/动机。

  5. 阅读commit消息来看它是否暗示提升了代码结构。当描述一个大的提交消息时，通常很明显需要分为多个commit。不要害怕回退及rebase后再修改后分成多个commit。

  6. 确保包含需要review什么的充分信息。当gerrit发送新patch提交的邮件提示时会包含小量信息，原则上是commit消息及所修改的文件列表。假如patch内容很多，那么期望所有reviewer详细检查patch是不合理的。commit消息需要有充分的信息来提示代码审查者需要review这个patch的哪些内容。

  7. commit消息的第一行是最重要的。在GIT commits中第一行有特殊含义。它用于邮件主题，git annotate消息，gitk viewer annotations,…第一行。同总结修改本身一样，也需要详细考虑这部分代码的影响。比如说，如果它影响到libvirt driver,那么在第一行提到”libvirt”

  8. 描述当前代码的限制。如果代码未来还有提升空间，或者任何已知的限制都需要在commit消息中提到。这向审查者展示一个更为广阔的蓝图以及长短期之间的权衡。

基本规则如下:
commit消息必须包含整个理解&审查patch正确性的信息。更少的不是更多的，更多的才是更多的

包含外部引用

commit消息主要用于人类解释，但是通常会有给机器用的元数据。虽然GIT记录patch的作者&committer，通常情况下最好还是如同很多开源工程一样包含”Signed-off-by”tag。此外，patch上还能有人的认证信息比如说:

  • ‘Reviewed-by: …some name.. <…email…>’ 尽管Gerrit会跟踪正式的review流程，一些patch会在提交到社区之前被其他人review
  • ‘Suggested-by: …some name.. <…email…>’ patch作者之外的人建议代码增强或者设计影响
  • ‘Reported-by: …some name.. <…email…>’ 某人报告bug被修改但写一个bug report

错误示范

例1

1
2
3
4
5
6
7
commit 468e64d019f51d364afb30b0eed2ad09483e0b98
Author: [removed]
Date: Mon Jun 18 16:07:37 2012 -0400

Fix missing import in compute/utils.py

Fixes bug 1014829

问题：这没有提到什么被import以及为何需要import这些。当然虽然信息在bug tracker中有记录，但也应当拷贝到commit消息中，使得其能提供一个自包含描述，比如:

1
2
3
Add missing import of 'exception' in compute/utils.py

nova/compute/utils.py makes a reference to exception.NotFound, however exception has not been imported.

例2

1
2
3
4
5
6
7
8
9
10
author: [removed]
Date: Fri Jun 15 11:12:45 2012 -0600

Present correct ec2id format for volumes and snaps

Fixes bug 1013765

* Add template argument to ec2utils.id_to_ec2_id() calls

Change-Id: I5e574f8e60d091ef8862ad814e2c8ab993daa366

问题：没有提到当前的格式及新的格式是什么。这个信息也在bug tracker中，且也需要包含到commit message中。另外，这个bug还修改了一个更早修改的回退，但它并没有提到更早的修改是什么. commit应该改成:

1
2
3
4
5
6
7
8
9
10
Present correct ec2id format for volumes and snaps

During the volume uuid migration, done by changeset XXXXXXX,
ec2 id formats for volumes and snapshots was dropped and is
now using the default instance format (i-xxxxx). These need
to be changed back to vol-xxx and snap-xxxx.

Adds a template argument to ec2utils.id_to_ec2_id() calls

Fixes bug 1013765

例3

1
2
3
4
5
6
7
8
9
commit f28731c1941e57b776b519783b0337e52e1484ab
Author: [removed]
Date: Wed Jun 13 10:11:04 2012 -0400

Add libvirt min version check.

Fixes LP Bug #1012689.

Change-Id: I91c0b7c41804b2b25026cbe672b9210c305dc29b

这个commit只提到它做了什么，而没有提到为什么这么做。它应当提到更早的修改引入一个新的最小libvirt version，及当check失败时会发生什么。

1
2
3
4
5
6
7
8
9
10
11
Add libvirt version check, min 0.9.7

The commit XXXXXXXX introduced use of the 'reset' API
which is only available in libvirt 0.9.7 or newer. Add a check
performed at startup of the compute server against the libvirt
connection version. If the version check fails the compute
service will shutdown.

Fixes LP Bug #1012689.

Change-Id: I91c0b7c41804b2b25026cbe672b9210c305dc29b

正确示范

例1

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
commit 3114a97ba188895daff4a3d337b2c73855d4632d
Author: [removed]
Date: Mon Jun 11 17:16:10 2012 +0100

Update default policies for KVM guest PIT & RTC timers

The default policies for the KVM guest PIT and RTC timers
are not very good at maintaining reliable time in guest
operating systems. In particular Windows 7 guests will
often crash with the default KVM timer policies, and old
Linux guests will have very bad time drift

Set the PIT such that missed ticks are injected at the
normal rate, ie they are delayed

Set the RTC such that missed ticks are injected at a
higher rate to "catch up"

This corresponds to the following libvirt XML

<clock offset='utc'>
<timer name='pit' tickpolicy='delay'/>
<timer name='rtc' tickpolicy='catchup'/>
</clock>

And the following KVM options

-no-kvm-pit-reinjection
-rtc base=utc,driftfix=slew

This should provide a default configuration that works
acceptably for most OS types. In the future this will
likely need to be made configurable per-guest OS type.

Fixes LP bug #1011848
Change-Id: Iafb0e2192b5f3c05b6395ffdfa14f86a98ce3d1f

例子中的commit message 提到的:

  • 描述了问题场景是什么
  • 描述了fix目的
  • 描述修正的大致架构
  • 说明了修正的局限性