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
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
|
## Git Commit Good Practice
The following document is based on experience doing code development, bug troubleshooting and code review across a number of projects using Git. The document is mostly borrowed from [Open Stack](https://wiki.openstack.org/wiki/GitCommitMessages), but made more meaningful in the context of GlusterFS project.
This topic can be split into two areas of concern
* The structured set/split of the code changes
* The information provided in the commit message
### Executive Summary
The points and examples that will be raised in this document ought to clearly demonstrate the value in splitting up changes into a sequence of individual commits, and the importance in writing good commit messages to go along with them. If these guidelines were widely applied it would result in a significant improvement in the quality of the GlusterFS Git history. Both a carrot & stick will be required to effect changes. This document intends to be the carrot by alerting people to the benefits, while anyone doing Gerrit code review can act as the stick ;-P
In other words, when reviewing a change in Gerrit:
* Do not simply look at the correctness of the code.
* Review the commit message itself and request improvements to its content.
* Look out for commits which are mixing multiple logical changes and require the submitter to split them into separate commits.
* Ensure whitespace changes are not mixed in with functional changes.
* Ensure no-op code refactoring is done separately from functional changes.
And so on.
It might be mentioned that Gerrit's handling of patch series is not entirely perfect. Let that not become a valid reason to avoid creating patch series. The tools being used should be subservient to developers needs, and since they are open source they can be fixed / improved. Software source code is "read mostly, write occassionally" and thus the most important criteria is to improve the long term maintainability by the large pool of developers in the community, and not to sacrifice too much for the sake of the single author who may never touch the code again.
And now the long detailed guidelines & examples of good & bad practice
### Structural split of changes
The cardinal rule for creating good commits is to ensure there is only one "logical change" per commit. There are many reasons why this is an important rule:
* The smaller the amount of code being changed, the quicker & easier it is to review & identify potential flaws.
* If a change is found to be flawed later, it may be necessary to revert the broken commit. This is much easier to do if there are not other unrelated code changes entangled with the original commit.
* When troubleshooting problems using Git's bisect capability, small well defined changes will aid in isolating exactly where the code problem was introduced.
* When browsing history using Git annotate/blame, small well defined changes also aid in isolating exactly where & why a piece of code came from.
#### Things to avoid when creating commits
With the above points in mind, there are some commonly encountered examples of bad things to avoid
* Mixing whitespace changes with functional code changes.
|
