Re: [PATCH] fix threaded grep for machines with only one cpu

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 17, 2010, 01:26 UTC
Message-ID: <7vvddwodox.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.1002170200370.20986@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 10 quoted lines

>> 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.