svn commit: r313260 - head/sys/kern

Steven Hartland steven.hartland at multiplay.co.uk
Sun Feb 5 02:16:14 UTC 2017


Hi Mateusz could you improve on the commit message as it currently 
describes what is changed, which can be obtained from the diff, but not why?

I hope on one feels like I'm trying to teach them to suck eggs, as I 
know everyone here has a wealth of experience, but I strongly believe 
commit messages are a very important way of improving the overall 
quality of the code base by sharing with others the reason for changes, 
which they can then learn from. I know I for one love picking up new 
nuggets of knowledge from others in this way.

Also I believe this is area the project as a whole can improve on, so I 
don't mean to single out anyone here.

Anyway I hope people find this useful:

When I write a commit message I try to stick to the following rules 
which I believe helps to bring clarity for others about my actions.
1. First line is a brief summary of the out come of the change e.g.
Fixed compiler warnings in nvmecontrol on 32bit platforms
2. Follow up paragraphs expand on #1 if needed including details about 
not just what but why the change was made e.g.
Use ssize_t instead of uint32_t to prevent warnings about a comparison 
with different signs. Due to the promotion rules, this would only  
happen on 32-bit platforms.
3. When writing #2 include details that would not be obvious to 
non-experts in the particular area.

#2 and #3 are really important to sharing knowledge that others may not 
know, its quite relevant to this commit msg, as while it may be obvious 
to you and others familiar with the atomic ops, to the rest of us we're 
just wondering why make this change?

N.B. The example is based on Warner's recent commit purely as an 
example, which had a good why, just missing the brief summary.

While on this subject are there any official guidelines to writing 
commit messages, if no should we create some?

On 05/02/2017 01:40, Mateusz Guzik wrote:
> Author: mjg
> Date: Sun Feb  5 01:40:27 2017
> New Revision: 313260
> URL: https://svnweb.freebsd.org/changeset/base/313260
>
> Log:
>    fd: switch fget_unlocked to atomic_fcmpset
>
> Modified:
>    head/sys/kern/kern_descrip.c
>
> Modified: head/sys/kern/kern_descrip.c
> ==============================================================================
> --- head/sys/kern/kern_descrip.c	Sun Feb  5 01:20:39 2017	(r313259)
> +++ head/sys/kern/kern_descrip.c	Sun Feb  5 01:40:27 2017	(r313260)
> @@ -2569,8 +2569,8 @@ fget_unlocked(struct filedesc *fdp, int
>   		if (error != 0)
>   			return (error);
>   #endif
> -	retry:
>   		count = fp->f_count;
> +	retry:
>   		if (count == 0) {
>   			/*
>   			 * Force a reload. Other thread could reallocate the
> @@ -2584,7 +2584,7 @@ fget_unlocked(struct filedesc *fdp, int
>   		 * Use an acquire barrier to force re-reading of fdt so it is
>   		 * refreshed for verification.
>   		 */
> -		if (atomic_cmpset_acq_int(&fp->f_count, count, count + 1) == 0)
> +		if (atomic_fcmpset_acq_int(&fp->f_count, &count, count + 1) == 0)
>   			goto retry;
>   		fdt = fdp->fd_files;
>   #ifdef	CAPABILITIES
>



More information about the svn-src-all mailing list