On Tue, 16 Feb 2010, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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@xxxxxxxxx>
> >> Acked-by: Fredrik Kuivinen <frekui@xxxxxxxxx>
> >> ---
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html