From: Nicolas Pitre <nico@fluxnic.net>
Date: Wed, 17 Feb 2010 02:41:20 GMT
Subject: Re: [PATCH] fix threaded grep for machines with only one cpu
Message-ID: <alpine.LFD.2.00.1002162140110.1946@xanadu.home>
In-Reply-To: <7vvddwodox.fsf@alter.siamese.dyndns.org>

On Tue, 16 Feb 2010, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> The program can decide at runtime not to use threading even if the support
> >> is compiled in.  In such a case, mutexes are not necessary and left
> >> uninitialized.  But the code incorrectly tried to take and release the
> >> read_sha1_mutex unconditionally.
> >> 
> >> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >> Acked-by: Fredrik Kuivinen <frekui@gmail.com>
> >> ---
> >
> > Yes, this one looks much, much nicer.
> 
> The structure may be much nicer, but one remaining thing is that I do not
> think foo_locked() is a good name; IIRC, kernel folks use _locked() suffix
> when the caller is expected to already hold the lock.  So a typical naming
> convention goes like this:
> 
> 	foo()
>         {
>         	lock();
>                 foo_locked();
>                 unlock();
>         }
> 
> but what the patch did was the other way around:
> 
> 	read_sha1_file_locked()
>         {
>         	lock();
>                 read_sha1_file();
>                 unlock();
> 	}
> 
> which is probably against the convention many readers of our codebase are
> already familiar with.
> 
> We need a better name to unconfuse people, I think.

lock_and_read_sha1_file()


Nicolas