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.
--
Antoine "hashar" Musso