Re: [PATCH] git grep: be careful to use mutices only when they are initialized

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2011, 20:02 UTC
Message-ID: <7v39ef34in.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info>

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

Show 23 quoted lines

> 	I looked around a bit but ran out of time to identify the reason why
> 	this was not caught earlier.
>
>  builtin/grep.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 92eeada..e94c5fe 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;
>  /* Used to serialize calls to read_sha1_file. */
>  static pthread_mutex_t read_sha1_mutex;
>  
> -#define grep_lock() pthread_mutex_lock(&grep_mutex)
> -#define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> +#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)
> +#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))
> +#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))
> +#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))
> +#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))

I think, from a quick glance, this is a good first step.

The remainder of this message are hints and random thoughts on potential follow-up patches that may want to build on top of this patch for further clean-ups (not specifically meant for Dscho but for other people on both mailing lists).

 - The patch makes the check for use_threads in lock_and_read_sha1_file()
   redundant. The other user of read_sha1_lock/unlock in grep_object() can
   take advantage of this change (see below).

 - It makes me wonder if it is simpler to initialize mutexes even in
   !use_threads case.

 - Wouldn't the result be more readable to make these into static inline
   functions?

 - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(),
   and possibly cmd_grep() functions and let the compiler optimize things
   away under NO_PTHREADS compilation?

diff --git a/builtin/grep.c b/builtin/grep.c
index 7d0779f..60daa85 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -354,13 +354,9 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type
 {
 	void *data;
 
-	if (use_threads) {
-		read_sha1_lock();
-		data = read_sha1_file(sha1, type, size);
-		read_sha1_unlock();
-	} else {
-		data = read_sha1_file(sha1, type, size);
-	}
+	read_sha1_lock();
+	data = read_sha1_file(sha1, type, size);
+	read_sha1_unlock();
 	return data;
 }