Le 16/01/13 14:06, Sébastien Santoro a écrit :
Should it be a formality to expedite in 30 seconds or an informative valuable text describing the change, crafted with care and proofread before submission or merge?
To me the commit summary introduce the patch to the reviewers and should also serve as a base for later debugging. It is always good to explain there the problem you attempt to solve or the feature being implemented, then explain your design choice and finally explain what you have tested / what people should be careful about.
Of course I do not apply that principle to myself :-] Some of the repositories on which I am the sole contributor and self reviewing, have very short commit message. But I do try to add extensive comments in the code.
A random example:
We had a trivial configuration issue in our php.ini file. The apc.shm_size parameter was missing a suffix to the amount of memory that need to be added.
The patch is:
files/php/apc.ini -apc.shm_size=240 +apc.shm_size=240M
I could have written:
'added M to apc mem size'
Instead I wrote:
------------------------ php.ini apc.shm_size requires
The apc.shm_size is an integer determining the size of each memory segment in MB. When missing the M suffix, PHP send us a warning in syslog:
php: PHP Warning: PHP Startup: apc.shm_size now uses M/G suffixes, please update your ini files in Unknown on line 0
Upstream documentation: http://php.net/manual/en/apc.configuration.php#ini.apc.shm-size
Having a change like 'fix preg call' is a -1 for me. I don't even bother reading the code in such a case. ------------------------
Yeah there are probably typo above, but at least that: - explain the change (I add M to apc.shm_size) - expose the issue (there is an annoying PHP warning) - reference doc for easy lookup for the reviewer
Done.
The PHP code base has usually very short summaries cause they simply reference the bug report. Example:
Bug #62593 Added test for change
The git project usually has very nice commit message. Have a look at http://repo.or.cz/w/git.git/commit/b72a1904aefb9e27014a11790f3af4dc90b38e8d
That optimize some code path, the summary show up command line output before and after the patch :-]
So yeah, we need extensive commit message.
If someone don't bother fixing a typo (which is like 1 minute of work), I will not bother doing it for them nor approving their change. My time is better invested in reviewing another patch.