[PATCH 00/10] Reduce #ifdef NO_PTHREADS

[PATCH 00/10] Reduce #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

People seemed to support the idea of removing these #ifdef NO_PTHREADS [1]
so this is a complete series. I left the #ifdef in run-command.c and
transport-helper.c because those code looked complicated so perhaps we
could clean them up later. Even these updated files could be updated
more, I think, to reduce some code duplication, but I tried to keep
the change here minimal.

[1] https://public-inbox.org/git/20181018180522.17642-1-pclouds@gmail.com/

Nguyễn Thái Ngọc Duy (10):
  thread-utils: macros to unconditionally compile pthreads API
  index-pack: remove #ifdef NO_PTHREADS
  name-hash.c: remove #ifdef NO_PTHREADS
  attr.c: remove #ifdef NO_PTHREADS
  send-pack.c: remove #ifdef NO_PTHREADS
  grep: remove #ifdef NO_PTHREADS
  preload-index.c: remove #ifdef NO_PTHREADS
  pack-objects: remove #ifdef NO_PTHREADS
  read-cache.c: remove #ifdef NO_PTHREADS
  Clean up pthread_create() error handling

 Makefile               |  2 +-
 attr.c                 | 14 ---------
 builtin/grep.c         | 58 +++++++++++++----------------------
 builtin/index-pack.c   | 68 +++++++++++-------------------------------
 builtin/pack-objects.c | 26 ++--------------
 grep.c                 |  6 ----
 grep.h                 |  6 ----
 name-hash.c            | 38 +++++++++--------------
 pack-objects.h         |  6 ----
 preload-index.c        | 23 +++++---------
 read-cache.c           | 49 +++++++++++-------------------
 run-command.c          |  2 +-
 send-pack.c            |  8 ++---
 thread-utils.c         | 48 +++++++++++++++++++++++++++++
 thread-utils.h         | 47 +++++++++++++++++++++++++++--
 15 files changed, 178 insertions(+), 223 deletions(-)

-- 
2.19.1.647.g708186aaf9

[PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

[Nguyễn Thái Ngọc Duy]

When built with NO_PTHREADS, the macros are used make the code build
even though pthreads header and library may be missing. The code can
still have different code paths for no threads support with
HAVE_THREADS variable.

There are of course impacts on no-pthreads builds:

- data structure may get slightly bigger because all the mutexes and
  pthread_t are present (as an int)

- code execution is not impacted much. Locking (in hot path) is
  no-op. Other wrapper function calls really should not matter much.

- the binary size grows bigger because of threaded code. But at least
  on Linux this does not matter, if some code is not executed, it's
  not mapped in memory.

This is a preparation step to remove "#ifdef NO_PTHREADS" in the code
mostly because of maintainability. As Jeff put it

> it's probably OK to stop thinking of it as "non-threaded platforms
> are the default and must pay zero cost" and more as "threaded
> platforms are the default, and non-threaded ones are OK to pay a
> small cost as long as they still work".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile       |  2 +-
 thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 thread-utils.h | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index b08d5ea258..321540a736 100644
--- a/Makefile
+++ b/Makefile
@@ -991,6 +991,7 @@ LIB_OBJS += sub-process.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
+LIB_OBJS += thread-utils.o
 LIB_OBJS += tmp-objdir.o
 LIB_OBJS += trace.o
 LIB_OBJS += trailer.o
@@ -1674,7 +1675,6 @@ ifdef NO_PTHREADS
 else
 	BASIC_CFLAGS += $(PTHREAD_CFLAGS)
 	EXTLIBS += $(PTHREAD_LIBS)
-	LIB_OBJS += thread-utils.o
 endif
 
 ifdef HAVE_PATHS_H
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0743..5329845691 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -20,6 +20,9 @@
 
 int online_cpus(void)
 {
+#ifdef NO_PTHREADS
+	return 1;
+#else
 #ifdef _SC_NPROCESSORS_ONLN
 	long ncpus;
 #endif
@@ -59,10 +62,12 @@ int online_cpus(void)
 #endif
 
 	return 1;
+#endif
 }
 
 int init_recursive_mutex(pthread_mutex_t *m)
 {
+#ifndef NO_PTHREADS
 	pthread_mutexattr_t a;
 	int ret;
 
@@ -74,4 +79,47 @@ int init_recursive_mutex(pthread_mutex_t *m)
 		pthread_mutexattr_destroy(&a);
 	}
 	return ret;
+#else
+	return 0;
+#endif
+}
+
+#ifdef NO_PTHREADS
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+int dummy_pthread_init(void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis or it may realize that functions like
+	 * pthread_mutex_init() is no-op, which means the (static)
+	 * variable is not used/initialized at all and trigger
+	 * -Wunused-variable
+	 */
+	return ENOSYS;
 }
+
+int dummy_pthread_join(pthread_t pthread, void **retval)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d190..a12850e747 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -4,12 +4,53 @@
 #ifndef NO_PTHREADS
 #include <pthread.h>
 
-extern int online_cpus(void);
-extern int init_recursive_mutex(pthread_mutex_t*);
+#define HAVE_THREADS 1
 
 #else
 
-#define online_cpus() 1
+#define HAVE_THREADS 0
+
+/*
+ * macros instead of typedefs because pthread definitions may have
+ * been pulled in by some system dependencies even though the user
+ * wants to disable pthread.
+ */
+#define pthread_t int
+#define pthread_mutex_t int
+#define pthread_cond_t int
+
+#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
+#define pthread_mutex_lock(mutex)
+#define pthread_mutex_unlock(mutex)
+#define pthread_mutex_destroy(mutex)
+
+#define pthread_cond_init(cond, attr) dummy_pthread_init(cond)
+#define pthread_cond_wait(cond, mutex)
+#define pthread_cond_signal(cond)
+#define pthread_cond_broadcast(cond)
+#define pthread_cond_destroy(cond)
+
+#define pthread_key_create(key, attr) dummy_pthread_init(key)
+#define pthread_key_delete(key)
+
+#define pthread_create(thread, attr, fn, data) \
+	dummy_pthread_create(thread, attr, fn, data)
+#define pthread_join(thread, retval) \
+	dummy_pthread_join(thread, retval)
+
+#define pthread_setspecific(key, data)
+#define pthread_getspecific(key) NULL
+
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data);
+int dummy_pthread_join(pthread_t pthread, void **retval);
+
+int dummy_pthread_init(void *);
 
 #endif
+
+int online_cpus(void);
+int init_recursive_mutex(pthread_mutex_t*);
+
+
 #endif /* THREAD_COMPAT_H */
-- 
2.19.1.647.g708186aaf9

[PATCH 02/10] index-pack: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 68 ++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 50 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da2..bbd66ca025 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -42,9 +42,7 @@ struct base_data {
 };
 
 struct thread_local {
-#ifndef NO_PTHREADS
 	pthread_t thread;
-#endif
 	struct base_data *base_cache;
 	size_t base_cache_used;
 	int pack_fd;
@@ -98,8 +96,6 @@ static uint32_t input_crc32;
 static int input_fd, output_fd;
 static const char *curr_pack;
 
-#ifndef NO_PTHREADS
-
 static struct thread_local *thread_data;
 static int nr_dispatched;
 static int threads_active;
@@ -179,26 +175,6 @@ static void cleanup_thread(void)
 	free(thread_data);
 }
 
-#else
-
-#define read_lock()
-#define read_unlock()
-
-#define counter_lock()
-#define counter_unlock()
-
-#define work_lock()
-#define work_unlock()
-
-#define deepest_delta_lock()
-#define deepest_delta_unlock()
-
-#define type_cas_lock()
-#define type_cas_unlock()
-
-#endif
-
-
 static int mark_link(struct object *obj, int type, void *data, struct fsck_options *options)
 {
 	if (!obj)
@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)
 
 static inline struct thread_local *get_thread_data(void)
 {
-#ifndef NO_PTHREADS
-	if (threads_active)
-		return pthread_getspecific(key);
-	assert(!threads_active &&
-	       "This should only be reached when all threads are gone");
-#endif
+	if (HAVE_THREADS) {
+		if (threads_active)
+			return pthread_getspecific(key);
+		assert(!threads_active &&
+		       "This should only be reached when all threads are gone");
+	}
 	return &nothread_data;
 }
 
-#ifndef NO_PTHREADS
 static void set_thread_data(struct thread_local *data)
 {
 	if (threads_active)
 		pthread_setspecific(key, data);
 }
-#endif
 
 static struct base_data *alloc_base_data(void)
 {
@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)
 	find_unresolved_deltas(base_obj);
 }
 
-#ifndef NO_PTHREADS
 static void *threaded_second_pass(void *data)
 {
 	set_thread_data(data);
@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data)
 	}
 	return NULL;
 }
-#endif
 
 /*
  * First pass:
@@ -1213,7 +1185,6 @@ static void resolve_deltas(void)
 		progress = start_progress(_("Resolving deltas"),
 					  nr_ref_deltas + nr_ofs_deltas);
 
-#ifndef NO_PTHREADS
 	nr_dispatched = 0;
 	if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
 		init_thread();
@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)
 		cleanup_thread();
 		return;
 	}
-#endif
 
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
@@ -1531,11 +1501,11 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
 		if (nr_threads < 0)
 			die(_("invalid number of threads specified (%d)"),
 			    nr_threads);
-#ifdef NO_PTHREADS
-		if (nr_threads != 1)
-			warning(_("no threads support, ignoring %s"), k);
-		nr_threads = 1;
-#endif
+		if (!HAVE_THREADS) {
+			if (nr_threads != 1)
+				warning(_("no threads support, ignoring %s"), k);
+			nr_threads = 1;
+		}
 		return 0;
 	}
 	return git_default_config(k, v, cb);
@@ -1723,12 +1693,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				nr_threads = strtoul(arg+10, &end, 0);
 				if (!arg[10] || *end || nr_threads < 0)
 					usage(index_pack_usage);
-#ifdef NO_PTHREADS
-				if (nr_threads != 1)
-					warning(_("no threads support, "
-						  "ignoring %s"), arg);
-				nr_threads = 1;
-#endif
+				if (!HAVE_THREADS) {
+					if (nr_threads != 1)
+						warning(_("no threads support, "
+							  "ignoring %s"), arg);
+					nr_threads = 1;
+				}
 			} else if (starts_with(arg, "--pack_header=")) {
 				struct pack_header *hdr;
 				char *c;
@@ -1791,14 +1761,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (strict)
 		opts.flags |= WRITE_IDX_STRICT;
 
-#ifndef NO_PTHREADS
-	if (!nr_threads) {
+	if (HAVE_THREADS && !nr_threads) {
 		nr_threads = online_cpus();
 		/* An experiment showed that more threads does not mean faster */
 		if (nr_threads > 3)
 			nr_threads = 3;
 	}
-#endif
 
 	curr_pack = open_pack_file(pack_name);
 	parse_pack_header();
-- 
2.19.1.647.g708186aaf9

[PATCH 03/10] name-hash.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index 1fcda73cb3..b3c9ac791d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -7,6 +7,7 @@
  */
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "thread-utils.h"
 
 struct dir_entry {
 	struct hashmap_entry ent;
@@ -131,22 +132,6 @@ static int cache_entry_cmp(const void *unused_cmp_data,
 static int lazy_try_threaded = 1;
 static int lazy_nr_dir_threads;
 
-#ifdef NO_PTHREADS
-
-static inline int lookup_lazy_params(struct index_state *istate)
-{
-	return 0;
-}
-
-static inline void threaded_lazy_init_name_hash(
-	struct index_state *istate)
-{
-}
-
-#else
-
-#include "thread-utils.h"
-
 /*
  * Set a minimum number of cache_entries that we will handle per
  * thread and use that to decide how many threads to run (upto
@@ -516,6 +501,9 @@ static void threaded_lazy_init_name_hash(
 	struct lazy_dir_thread_data *td_dir;
 	struct lazy_name_thread_data *td_name;
 
+	if (!HAVE_THREADS)
+		return;
+
 	k_start = 0;
 	nr_each = DIV_ROUND_UP(istate->cache_nr, lazy_nr_dir_threads);
 
@@ -574,8 +562,6 @@ static void threaded_lazy_init_name_hash(
 	free(lazy_entries);
 }
 
-#endif
-
 static void lazy_init_name_hash(struct index_state *istate)
 {
 
-- 
2.19.1.647.g708186aaf9

[PATCH 04/10] attr.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/attr.c b/attr.c
index 60d284796d..eaece6658d 100644
--- a/attr.c
+++ b/attr.c
@@ -41,23 +41,17 @@ const char *git_attr_name(const struct git_attr *attr)
 
 struct attr_hashmap {
 	struct hashmap map;
-#ifndef NO_PTHREADS
 	pthread_mutex_t mutex;
-#endif
 };
 
 static inline void hashmap_lock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&map->mutex);
-#endif
 }
 
 static inline void hashmap_unlock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&map->mutex);
-#endif
 }
 
 /*
@@ -498,23 +492,17 @@ static struct check_vector {
 	size_t nr;
 	size_t alloc;
 	struct attr_check **checks;
-#ifndef NO_PTHREADS
 	pthread_mutex_t mutex;
-#endif
 } check_vector;
 
 static inline void vector_lock(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&check_vector.mutex);
-#endif
 }
 
 static inline void vector_unlock(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&check_vector.mutex);
-#endif
 }
 
 static void check_vector_add(struct attr_check *c)
@@ -1181,8 +1169,6 @@ void git_all_attrs(const struct index_state *istate,
 
 void attr_start(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
 	pthread_mutex_init(&check_vector.mutex, NULL);
-#endif
 }
-- 
2.19.1.647.g708186aaf9

[PATCH 05/10] send-pack.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

While at there correct "#include cache.h" position. It must be one of
the first includes.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 send-pack.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index e920ca57df..fa7dc44b36 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "config.h"
 #include "commit.h"
 #include "refs.h"
@@ -14,7 +15,7 @@
 #include "version.h"
 #include "sha1-array.h"
 #include "gpg-interface.h"
-#include "cache.h"
+#include "thread-utils.h"
 
 int option_parse_push_signed(const struct option *opt,
 			     const char *arg, int unset)
@@ -203,9 +204,8 @@ static int receive_status(int in, struct ref *refs)
 static int sideband_demux(int in, int out, void *data)
 {
 	int *fd = data, ret;
-#ifdef NO_PTHREADS
-	close(fd[1]);
-#endif
+	if (!HAVE_THREADS)
+		close(fd[1]);
 	ret = recv_sideband("send-pack", fd[0], out);
 	close(out);
 	return ret;
-- 
2.19.1.647.g708186aaf9

[PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 58 +++++++++++++++++---------------------------------
 grep.c         |  6 ------
 grep.h         |  6 ------
 3 files changed, 20 insertions(+), 50 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..29221e1003 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@ static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static int num_threads;
 
-#ifndef NO_PTHREADS
 static pthread_t *threads;
 
 /* We use one producer thread and THREADS consumer
@@ -265,13 +264,6 @@ static int wait_all(void)
 
 	return hit;
 }
-#else /* !NO_PTHREADS */
-
-static int wait_all(void)
-{
-	return 0;
-}
-#endif
 
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
@@ -284,8 +276,7 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 		if (num_threads < 0)
 			die(_("invalid number of threads specified (%d) for %s"),
 			    num_threads, var);
-#ifdef NO_PTHREADS
-		else if (num_threads && num_threads != 1) {
+		else if (!HAVE_THREADS && num_threads && num_threads != 1) {
 			/*
 			 * TRANSLATORS: %s is the configuration
 			 * variable for tweaking threads, currently
@@ -294,7 +285,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 			warning(_("no threads support, ignoring %s"), var);
 			num_threads = 0;
 		}
-#endif
 	}
 
 	if (!strcmp(var, "submodule.recurse"))
@@ -330,17 +320,14 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 	grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
 	strbuf_release(&pathbuf);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -363,17 +350,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
 	strbuf_release(&buf);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -1038,20 +1022,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	pathspec.recursive = 1;
 	pathspec.recurse_submodules = !!recurse_submodules;
 
-#ifndef NO_PTHREADS
-	if (list.nr || cached || show_in_pager)
-		num_threads = 0;
-	else if (num_threads == 0)
-		num_threads = GREP_NUM_THREADS_DEFAULT;
-	else if (num_threads < 0)
-		die(_("invalid number of threads specified (%d)"), num_threads);
-	if (num_threads == 1)
+	if (HAVE_THREADS) {
+		if (list.nr || cached || show_in_pager)
+			num_threads = 0;
+		else if (num_threads == 0)
+			num_threads = GREP_NUM_THREADS_DEFAULT;
+		else if (num_threads < 0)
+			die(_("invalid number of threads specified (%d)"), num_threads);
+		if (num_threads == 1)
+			num_threads = 0;
+	} else {
+		if (num_threads)
+			warning(_("no threads support, ignoring --threads"));
 		num_threads = 0;
-#else
-	if (num_threads)
-		warning(_("no threads support, ignoring --threads"));
-	num_threads = 0;
-#endif
+	}
 
 	if (!num_threads)
 		/*
@@ -1062,15 +1046,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		 */
 		compile_grep_patterns(&opt);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
 		    && (opt.pre_context || opt.post_context ||
 			opt.file_break || opt.funcbody))
 			skip_first_line = 1;
 		start_threads(&opt);
 	}
-#endif
 
 	if (show_in_pager && (cached || list.nr))
 		die(_("--open-files-in-pager only works on the worktree"));
@@ -1121,7 +1103,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		hit = grep_objects(&opt, &pathspec, &list);
 	}
 
-	if (num_threads)
+	if (HAVE_THREADS && num_threads)
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
diff --git a/grep.c b/grep.c
index f6bd89e40b..4db1510d16 100644
--- a/grep.c
+++ b/grep.c
@@ -1513,7 +1513,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	}
 }
 
-#ifndef NO_PTHREADS
 int grep_use_locks;
 
 /*
@@ -1539,11 +1538,6 @@ static inline void grep_attr_unlock(void)
  */
 pthread_mutex_t grep_read_mutex;
 
-#else
-#define grep_attr_lock()
-#define grep_attr_unlock()
-#endif
-
 static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
 {
 	xdemitconf_t *xecfg = opt->priv;
diff --git a/grep.h b/grep.h
index 1a57d12b90..fb04893721 100644
--- a/grep.h
+++ b/grep.h
@@ -229,7 +229,6 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs);
 extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
 extern int grep_threads_ok(const struct grep_opt *opt);
 
-#ifndef NO_PTHREADS
 /*
  * Mutex used around access to the attributes machinery if
  * opt->use_threads.  Must be initialized/destroyed by callers!
@@ -250,9 +249,4 @@ static inline void grep_read_unlock(void)
 		pthread_mutex_unlock(&grep_read_mutex);
 }
 
-#else
-#define grep_read_lock()
-#define grep_read_unlock()
-#endif
-
 #endif
-- 
2.19.1.647.g708186aaf9

[PATCH 07/10] preload-index.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 preload-index.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,17 +7,7 @@
 #include "fsmonitor.h"
 #include "config.h"
 #include "progress.h"
-
-#ifdef NO_PTHREADS
-static void preload_index(struct index_state *index,
-			  const struct pathspec *pathspec,
-			  unsigned int refresh_flags)
-{
-	; /* nothing */
-}
-#else
-
-#include <pthread.h>
+#include "thread-utils.h"
 
 /*
  * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,
 	struct thread_data data[MAX_PARALLEL];
 	struct progress_data pd;
 
-	if (!core_preload_index)
+	if (!HAVE_THREADS || !core_preload_index)
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,
 
 	trace_performance_leave("preload index");
 }
-#endif
 
 int read_index_preload(struct index_state *index,
 		       const struct pathspec *pathspec,
-- 
2.19.1.647.g708186aaf9

[PATCH 08/10] pack-objects: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 26 ++------------------------
 pack-objects.h         |  6 ------
 2 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..12edd6da16 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1953,8 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 	return 0;
 }
 
-#ifndef NO_PTHREADS
-
 /* Protect access to object database */
 static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
@@ -1979,16 +1977,6 @@ static pthread_mutex_t progress_mutex;
  * ahead in the list because they can be stolen and would need
  * progress_mutex for protection.
  */
-#else
-
-#define read_lock()		(void)0
-#define read_unlock()		(void)0
-#define cache_lock()		(void)0
-#define cache_unlock()		(void)0
-#define progress_lock()		(void)0
-#define progress_unlock()	(void)0
-
-#endif
 
 /*
  * Return the size of the object without doing any delta
@@ -2347,8 +2335,6 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 	free(array);
 }
 
-#ifndef NO_PTHREADS
-
 static void try_to_free_from_threads(size_t size)
 {
 	read_lock();
@@ -2578,10 +2564,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	free(p);
 }
 
-#else
-#define ll_find_deltas(l, s, w, d, p)	find_deltas(l, &s, w, d, p)
-#endif
-
 static void add_tag_chain(const struct object_id *oid)
 {
 	struct tag *tag;
@@ -2734,12 +2716,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 		if (delta_search_threads < 0)
 			die(_("invalid number of threads specified (%d)"),
 			    delta_search_threads);
-#ifdef NO_PTHREADS
-		if (delta_search_threads != 1) {
+		if (!HAVE_THREADS && delta_search_threads != 1) {
 			warning(_("no threads support, ignoring %s"), k);
 			delta_search_threads = 0;
 		}
-#endif
 		return 0;
 	}
 	if (!strcmp(k, "pack.indexversion")) {
@@ -3402,10 +3382,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!delta_search_threads)	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
 
-#ifdef NO_PTHREADS
-	if (delta_search_threads != 1)
+	if (!HAVE_THREADS && delta_search_threads != 1)
 		warning(_("no threads support, ignoring --threads"));
-#endif
 	if (!pack_to_stdout && !pack_size_limit)
 		pack_size_limit = pack_size_limit_cfg;
 	if (pack_to_stdout && pack_size_limit)
diff --git a/pack-objects.h b/pack-objects.h
index 2ca39cfcfe..3a42727c7d 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -145,9 +145,7 @@ struct packing_data {
 	struct packed_git **in_pack_by_idx;
 	struct packed_git **in_pack;
 
-#ifndef NO_PTHREADS
 	pthread_mutex_t lock;
-#endif
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -169,15 +167,11 @@ void prepare_packing_data(struct packing_data *pdata);
 
 static inline void packing_data_lock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&pdata->lock);
-#endif
 }
 static inline void packing_data_unlock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&pdata->lock);
-#endif
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
-- 
2.19.1.647.g708186aaf9

[PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 49 ++++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index d57958233e..ba870bc3fd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
 	struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
 	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
 	return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 
 	return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	size_t mmap_size;
 	struct load_index_extensions p;
 	size_t extension_offset = 0;
-#ifndef NO_PTHREADS
 	int nr_threads, cpus;
 	struct index_entry_offset_table *ieot = NULL;
-#endif
 
 	if (istate->initialized)
 		return istate->cache_nr;
@@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS) {
+		nr_threads = git_config_get_index_threads();
 
-	/* TODO: does creating more threads than cores help? */
-	if (!nr_threads) {
-		nr_threads = istate->cache_nr / THREAD_COST;
-		cpus = online_cpus();
-		if (nr_threads > cpus)
-			nr_threads = cpus;
+		/* TODO: does creating more threads than cores help? */
+		if (!nr_threads) {
+			nr_threads = istate->cache_nr / THREAD_COST;
+			cpus = online_cpus();
+			if (nr_threads > cpus)
+				nr_threads = cpus;
+		}
+	} else {
+		nr_threads = 1;
 	}
 
 	if (nr_threads > 1) {
@@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
 	}
-#else
-	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-#endif
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-#ifndef NO_PTHREADS
-	if (extension_offset) {
+	if (HAVE_THREADS && extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
 	}
-#endif
 	if (!extension_offset) {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
@@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS)
+		nr_threads = git_config_get_index_threads();
+	else
+		nr_threads = 1;
+
 	if (nr_threads != 1) {
 		int ieot_blocks, cpus;
 
@@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
 		}
 	}
-#endif
 
 	offset = lseek(newfd, 0, SEEK_CUR);
 	if (offset < 0) {
@@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-#ifndef NO_PTHREADS
-	if (ieot) {
+	if (HAVE_THREADS && ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
@@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		if (err)
 			return -1;
 	}
-#endif
 
 	if (!strip_extensions && istate->split_index) {
 		struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
 	strbuf_add(sb, hash, the_hash_algo->rawsz);
 }
 
-#ifndef NO_PTHREADS
 #define IEOT_VERSION	(1)
 
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
@@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
 	       strbuf_add(sb, &buffer, sizeof(uint32_t));
        }
 }
-#endif
-- 
2.19.1.647.g708186aaf9

[PATCH 10/10] Clean up pthread_create() error handling

[Nguyễn Thái Ngọc Duy]

Normally pthread_create() rarely fails and sometimes error handling
can be neglected. But with new pthreads wrapper, pthread_create() will
return ENOSYS on a system without thread support.

Threaded code _is_ protected by HAVE_THREADS and pthread_create()
should never run in the first place. But the situation could change in
the future and bugs may sneak in. Make sure that all pthread_create()
checks and handles the result.

While at there, mark these strings for translation if they aren't.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c     | 16 ++++++++++------
 preload-index.c |  8 ++++++--
 run-command.c   |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index b3c9ac791d..623ca6923a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -494,6 +494,7 @@ static inline void lazy_update_dir_ref_counts(
 static void threaded_lazy_init_name_hash(
 	struct index_state *istate)
 {
+	int err;
 	int nr_each;
 	int k_start;
 	int t;
@@ -526,8 +527,9 @@ static void threaded_lazy_init_name_hash(
 		if (k_start > istate->cache_nr)
 			k_start = istate->cache_nr;
 		td_dir_t->k_end = k_start;
-		if (pthread_create(&td_dir_t->pthread, NULL, lazy_dir_thread_proc, td_dir_t))
-			die("unable to create lazy_dir_thread");
+		err = pthread_create(&td_dir_t->pthread, NULL, lazy_dir_thread_proc, td_dir_t);
+		if (err)
+			die(_("unable to create lazy_dir thread: %s"), strerror(err));
 	}
 	for (t = 0; t < lazy_nr_dir_threads; t++) {
 		struct lazy_dir_thread_data *td_dir_t = td_dir + t;
@@ -547,13 +549,15 @@ static void threaded_lazy_init_name_hash(
 	 */
 	td_name->istate = istate;
 	td_name->lazy_entries = lazy_entries;
-	if (pthread_create(&td_name->pthread, NULL, lazy_name_thread_proc, td_name))
-		die("unable to create lazy_name_thread");
+	err = pthread_create(&td_name->pthread, NULL, lazy_name_thread_proc, td_name);
+	if (err)
+		die(_("unable to create lazy_name thread: %s"), strerror(err));
 
 	lazy_update_dir_ref_counts(istate, lazy_entries);
 
-	if (pthread_join(td_name->pthread, NULL))
-		die("unable to join lazy_name_thread");
+	err = pthread_join(td_name->pthread, NULL);
+	if (err)
+		die(_("unable to join lazy_name thread: %s"), strerror(err));
 
 	cleanup_dir_mutex();
 
diff --git a/preload-index.c b/preload-index.c
index 0e24886aca..ddca1c216e 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -121,6 +121,8 @@ static void preload_index(struct index_state *index,
 
 	for (i = 0; i < threads; i++) {
 		struct thread_data *p = data+i;
+		int err;
+
 		p->index = index;
 		if (pathspec)
 			copy_pathspec(&p->pathspec, pathspec);
@@ -129,8 +131,10 @@ static void preload_index(struct index_state *index,
 		if (pd.progress)
 			p->progress = &pd;
 		offset += work;
-		if (pthread_create(&p->pthread, NULL, preload_thread, p))
-			die("unable to create threaded lstat");
+		err = pthread_create(&p->pthread, NULL, preload_thread, p);
+
+		if (err)
+			die(_("unable to create threaded lstat: %s"), strerror(err));
 	}
 	for (i = 0; i < threads; i++) {
 		struct thread_data *p = data+i;
diff --git a/run-command.c b/run-command.c
index 84b883c213..26154ba257 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1213,7 +1213,7 @@ int start_async(struct async *async)
 	{
 		int err = pthread_create(&async->tid, NULL, run_thread, async);
 		if (err) {
-			error_errno("cannot create thread");
+			error(_("cannot create async thread: %s"), strerror(err));
 			goto error;
 		}
 	}
-- 
2.19.1.647.g708186aaf9

Re: [PATCH 00/10] Reduce #ifdef NO_PTHREADS

[Jeff King]

On Sat, Oct 27, 2018 at 09:09:53AM +0200, Nguyễn Thái Ngọc Duy wrote:

> People seemed to support the idea of removing these #ifdef NO_PTHREADS [1]
> so this is a complete series. I left the #ifdef in run-command.c and
> transport-helper.c because those code looked complicated so perhaps we
> could clean them up later. Even these updated files could be updated
> more, I think, to reduce some code duplication, but I tried to keep
> the change here minimal.

Some of the bits in run-command.c can probably be converted, but all of
the "struct async" bits can't ever be. Because there the fallback is not
"OK, let's just run a single thread" but a whole separate forking
mechanism.

-Peff

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

[Jeff King]

On Sat, Oct 27, 2018 at 09:09:54AM +0200, Nguyễn Thái Ngọc Duy wrote:

> +/*
> + * macros instead of typedefs because pthread definitions may have
> + * been pulled in by some system dependencies even though the user
> + * wants to disable pthread.
> + */
> +#define pthread_t int
> +#define pthread_mutex_t int
> +#define pthread_cond_t int
> +
> +#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
> +#define pthread_mutex_lock(mutex)
> +#define pthread_mutex_unlock(mutex)
> +#define pthread_mutex_destroy(mutex)

OK, we only need to do the dummy init to shut up the compiler, and the
rest really can just become noops. Makes sense.

> +#define pthread_cond_init(cond, attr) dummy_pthread_init(cond)
> +#define pthread_cond_wait(cond, mutex)
> +#define pthread_cond_signal(cond)
> +#define pthread_cond_broadcast(cond)
> +#define pthread_cond_destroy(cond)

And this is the same.

> +#define pthread_key_create(key, attr) dummy_pthread_init(key)
> +#define pthread_key_delete(key)

We don't need keys because we know we have only one key: the main
thread. Makes sense. But...

> +#define pthread_setspecific(key, data)
> +#define pthread_getspecific(key) NULL

We expect to be able to store a void pointer here and get it back, which
should work even for a single thread. Do we need something like:

  extern void *pthread_specific_data;

  #define pthread_setspecific(key, data) do { \
	pthread_specific_data = data; \
  } while(0)

  void pthread_getspecific(key) pthread_specific_data

> +#define pthread_create(thread, attr, fn, data) \
> +	dummy_pthread_create(thread, attr, fn, data)

I wondered if this should actually run "fn", but I guess there is not
much point. At this point the caller expects the main thread to keep
going, so this is already an error, and reporting ENOSYS is probably the
best thing to do.

-Peff

Re: [PATCH 02/10] index-pack: remove #ifdef NO_PTHREADS

[Jeff King]

On Sat, Oct 27, 2018 at 09:09:55AM +0200, Nguyễn Thái Ngọc Duy wrote:

>  static inline struct thread_local *get_thread_data(void)
>  {
> -#ifndef NO_PTHREADS
> -	if (threads_active)
> -		return pthread_getspecific(key);
> -	assert(!threads_active &&
> -	       "This should only be reached when all threads are gone");
> -#endif
> +	if (HAVE_THREADS) {
> +		if (threads_active)
> +			return pthread_getspecific(key);
> +		assert(!threads_active &&
> +		       "This should only be reached when all threads are gone");
> +	}
>  	return &nothread_data;
>  }
>  
> -#ifndef NO_PTHREADS
>  static void set_thread_data(struct thread_local *data)
>  {
>  	if (threads_active)
>  		pthread_setspecific(key, data);
>  }
> -#endif

If we do the getspecific/setspecific thing I mentioned for the previous
patch, then I think helpers like this can just go away.

-Peff

Re: [PATCH 05/10] send-pack.c: remove #ifdef NO_PTHREADS

[Jeff King]

On Sat, Oct 27, 2018 at 09:09:58AM +0200, Nguyễn Thái Ngọc Duy wrote:

> While at there correct "#include cache.h" position. It must be one of
> the first includes.
> [...]
> @@ -1,4 +1,5 @@
>  #include "builtin.h"
> +#include "cache.h"

I think it's actually fine as it is. The rule is not about cache.h in
particular, but that the first include must be one of the special ones.
And builtin.h is such a special one. After that, there are no ordering
rules.

> @@ -203,9 +204,8 @@ static int receive_status(int in, struct ref *refs)
>  static int sideband_demux(int in, int out, void *data)
>  {
>  	int *fd = data, ret;
> -#ifdef NO_PTHREADS
> -	close(fd[1]);
> -#endif
> +	if (!HAVE_THREADS)
> +		close(fd[1]);
>  	ret = recv_sideband("send-pack", fd[0], out);
>  	close(out);
>  	return ret;

This one is a very interesting case. Your conversion here isn't wrong,
but what we actually want to know is: is "struct async" implemented as a
separate process or not.

And "struct async" will continue to switch on NO_PTHREADS, as it can
never use these dummy bits. I can't think of a way that HAVE_THREADS
would ever diverge from how "struct async" works, but my gut feeling is
that we probably ought to have a separate variable for the async code to
tell how it works. Even if it ends up being the same as HAVE_THREADS,
it helps untangle the two cases.

-Peff

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

[Duy Nguyen]

On Sat, Oct 27, 2018 at 9:31 AM Jeff King <peff@peff.net> wrote:
> > +#define pthread_setspecific(key, data)
> > +#define pthread_getspecific(key) NULL
>
> We expect to be able to store a void pointer here and get it back, which
> should work even for a single thread. Do we need something like:
>
>   extern void *pthread_specific_data;
>
>   #define pthread_setspecific(key, data) do { \
>         pthread_specific_data = data; \
>   } while(0)
>
>   void pthread_getspecific(key) pthread_specific_data

The data is per key though so a correct implementation may involve a
hashmap or a list. It does simplify index-pack which has to fall back
to nothread_data when pthreads is not available. But with index-pack
being the only call site that can take advantage of this
(run-command.c probably will use real pthreads library anyway), I'm
not sure if it's worth really implementing these functions.
-- 
Duy

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Jeff King]

On Sat, Oct 27, 2018 at 09:09:59AM +0200, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/builtin/grep.c b/builtin/grep.c
> index d8508ddf79..29221e1003 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -34,7 +34,6 @@ static int recurse_submodules;
>  #define GREP_NUM_THREADS_DEFAULT 8
>  static int num_threads;
>  
> -#ifndef NO_PTHREADS
>  static pthread_t *threads;
>  
>  /* We use one producer thread and THREADS consumer
> @@ -265,13 +264,6 @@ static int wait_all(void)
>  
>  	return hit;
>  }
> -#else /* !NO_PTHREADS */
> -
> -static int wait_all(void)
> -{
> -	return 0;
> -}
> -#endif

Cases like this are kind of weird. I'd expect to see wait_all() return
immediately when !HAVE_THREADS. But we don't need to, because later we
do this:

> -	if (num_threads)
> +	if (HAVE_THREADS && num_threads)
>  		hit |= wait_all();

Which I think works, but it feels like we're introducing some subtle
dependencies about which functions get called in which cases. I'd hoped
in general that the non-threaded code paths could mostly just collapse
down into the same as the "threads == 1" cases, and we wouldn't have to
ask "are threads even supported" in a lot of places.

I dunno. My comment isn't really an objection exactly, but mostly just
an indication that I'm still thinking through what the best approach is,
and what end state we want to end up in.

-Peff

Re: [PATCH 00/10] Reduce #ifdef NO_PTHREADS

[Jeff King]

On Sat, Oct 27, 2018 at 09:09:53AM +0200, Nguyễn Thái Ngọc Duy wrote:

> People seemed to support the idea of removing these #ifdef NO_PTHREADS [1]
> so this is a complete series. I left the #ifdef in run-command.c and
> transport-helper.c because those code looked complicated so perhaps we
> could clean them up later. Even these updated files could be updated
> more, I think, to reduce some code duplication, but I tried to keep
> the change here minimal.
> 
> [1] https://public-inbox.org/git/20181018180522.17642-1-pclouds@gmail.com/
> 
> Nguyễn Thái Ngọc Duy (10):
>   thread-utils: macros to unconditionally compile pthreads API
>   index-pack: remove #ifdef NO_PTHREADS
>   name-hash.c: remove #ifdef NO_PTHREADS
>   attr.c: remove #ifdef NO_PTHREADS
>   send-pack.c: remove #ifdef NO_PTHREADS
>   grep: remove #ifdef NO_PTHREADS
>   preload-index.c: remove #ifdef NO_PTHREADS
>   pack-objects: remove #ifdef NO_PTHREADS
>   read-cache.c: remove #ifdef NO_PTHREADS
>   Clean up pthread_create() error handling

Compiling with NO_PTHREADS=1, I get (with gcc 8.2.0):

read-cache.c: In function ‘do_read_index’:
read-cache.c:1820:4: error: ‘copy_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
    memcpy(ce->name, previous_ce->name, copy_len);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
read-cache.c:1749:9: note: ‘copy_len’ was declared here
  size_t copy_len;

which seems wrong to me. It's always tied to expand_name_field being
true. And curiously an earlier use doesn't trigger the warning. I wonder
if some kind of tricky pointer aliasing in the intervening code makes it
think that expand_name_field could change, but I sure don't see it.

I ran the tests under ASan/UBSan, since this series seems like it has a
good chance of accidentally causing issues there, but didn't come up
with any problems (except for the ones already fixed on pu by 8628ace269
(commit-reach: fix cast in compare_commits_by_gen(), 2018-10-01)).

-Peff

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

[Jeff King]

On Sat, Oct 27, 2018 at 09:40:13AM +0200, Duy Nguyen wrote:

> > We expect to be able to store a void pointer here and get it back, which
> > should work even for a single thread. Do we need something like:
> >
> >   extern void *pthread_specific_data;
> >
> >   #define pthread_setspecific(key, data) do { \
> >         pthread_specific_data = data; \
> >   } while(0)
> >
> >   void pthread_getspecific(key) pthread_specific_data
> 
> The data is per key though so a correct implementation may involve a
> hashmap or a list.

Ah, yeah, you're right, I was mixing up the thread id and the key in my
head. I think it would just be an array of void pointers, with
pthread_key_create() returning an static index.

> It does simplify index-pack which has to fall back to nothread_data
> when pthreads is not available. But with index-pack being the only
> call site that can take advantage of this (run-command.c probably will
> use real pthreads library anyway), I'm not sure if it's worth really
> implementing these functions.

Yeah, you might be right.

-Peff

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

[Duy Nguyen]

On Sat, Oct 27, 2018 at 10:15 AM Jeff King <peff@peff.net> wrote:
>
> On Sat, Oct 27, 2018 at 09:40:13AM +0200, Duy Nguyen wrote:
>
> > > We expect to be able to store a void pointer here and get it back, which
> > > should work even for a single thread. Do we need something like:
> > >
> > >   extern void *pthread_specific_data;
> > >
> > >   #define pthread_setspecific(key, data) do { \
> > >         pthread_specific_data = data; \
> > >   } while(0)
> > >
> > >   void pthread_getspecific(key) pthread_specific_data
> >
> > The data is per key though so a correct implementation may involve a
> > hashmap or a list.
>
> Ah, yeah, you're right, I was mixing up the thread id and the key in my
> head. I think it would just be an array of void pointers, with
> pthread_key_create() returning an static index.

We could redefine pthread_key_t as "void *" though, then
_setspecific(key, data) is key = data; But there will be more changes
in index-pack.c to take advantage of it. I took a stab but couldn't
figure out fast enough where _setspecific(.., &nothread_data) should
be called in NO_PTHREADS mode (to simplify get_thread_data() to just a
wrapper of _getspecific) and got bored. It could be a micro project
for someone really wants to learn about index-pack.c
-- 
Duy

Re: [PATCH 00/10] Reduce #ifdef NO_PTHREADS

[Duy Nguyen]

On Sat, Oct 27, 2018 at 10:13 AM Jeff King <peff@peff.net> wrote:
>
> On Sat, Oct 27, 2018 at 09:09:53AM +0200, Nguyễn Thái Ngọc Duy wrote:
>
> > People seemed to support the idea of removing these #ifdef NO_PTHREADS [1]
> > so this is a complete series. I left the #ifdef in run-command.c and
> > transport-helper.c because those code looked complicated so perhaps we
> > could clean them up later. Even these updated files could be updated
> > more, I think, to reduce some code duplication, but I tried to keep
> > the change here minimal.
> >
> > [1] https://public-inbox.org/git/20181018180522.17642-1-pclouds@gmail.com/
> >
> > Nguyễn Thái Ngọc Duy (10):
> >   thread-utils: macros to unconditionally compile pthreads API
> >   index-pack: remove #ifdef NO_PTHREADS
> >   name-hash.c: remove #ifdef NO_PTHREADS
> >   attr.c: remove #ifdef NO_PTHREADS
> >   send-pack.c: remove #ifdef NO_PTHREADS
> >   grep: remove #ifdef NO_PTHREADS
> >   preload-index.c: remove #ifdef NO_PTHREADS
> >   pack-objects: remove #ifdef NO_PTHREADS
> >   read-cache.c: remove #ifdef NO_PTHREADS
> >   Clean up pthread_create() error handling
>
> Compiling with NO_PTHREADS=1, I get (with gcc 8.2.0):
>
> read-cache.c: In function ‘do_read_index’:
> read-cache.c:1820:4: error: ‘copy_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     memcpy(ce->name, previous_ce->name, copy_len);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> read-cache.c:1749:9: note: ‘copy_len’ was declared here
>   size_t copy_len;
>
> which seems wrong to me. It's always tied to expand_name_field being
> true. And curiously an earlier use doesn't trigger the warning. I wonder
> if some kind of tricky pointer aliasing in the intervening code makes it
> think that expand_name_field could change, but I sure don't see it.

I couldn't reproduce it with gcc 8.2.0 on gentoo (I tried all
optimization levels just in case). I agree the warning is wrong. But I
will initialize copy_len anyway to avoid this.
-- 
Duy

[PATCH v2 00/10] Reduce #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

The send-pack.c patch is dropped since it's tied to async code and
like transport-helper.c (or more cleanups in index-pack.c) could be
left for later.

I added one more patch to shut up -Wmaybe-uninitialized but since I
could not reproduce it, Jeff would need to verify if it works for him.

Nguyễn Thái Ngọc Duy (10):
  thread-utils: macros to unconditionally compile pthreads API
  index-pack: remove #ifdef NO_PTHREADS
  name-hash.c: remove #ifdef NO_PTHREADS
  attr.c: remove #ifdef NO_PTHREADS
  grep: remove #ifdef NO_PTHREADS
  preload-index.c: remove #ifdef NO_PTHREADS
  pack-objects: remove #ifdef NO_PTHREADS
  read-cache.c: remove #ifdef NO_PTHREADS
  Clean up pthread_create() error handling
  read-cache.c: initialize copy_len to shut up gcc 8

 Makefile               |  2 +-
 attr.c                 | 14 ---------
 builtin/grep.c         | 59 ++++++++++++++----------------------
 builtin/index-pack.c   | 68 +++++++++++-------------------------------
 builtin/pack-objects.c | 26 ++--------------
 grep.c                 |  6 ----
 grep.h                 |  6 ----
 name-hash.c            | 38 +++++++++--------------
 pack-objects.h         |  6 ----
 preload-index.c        | 23 +++++---------
 read-cache.c           | 53 ++++++++++++--------------------
 run-command.c          |  2 +-
 thread-utils.c         | 48 +++++++++++++++++++++++++++++
 thread-utils.h         | 48 +++++++++++++++++++++++++++--
 14 files changed, 178 insertions(+), 221 deletions(-)

Range-diff against v1:
 1:  fd4926a83a !  1:  2791c06629 thread-utils: macros to unconditionally compile pthreads API
    @@ -146,6 +146,7 @@
     +#define pthread_t int
     +#define pthread_mutex_t int
     +#define pthread_cond_t int
    ++#define pthread_key_t int
     +
     +#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
     +#define pthread_mutex_lock(mutex)
 2:  47b1f63fdd =  2:  61b3efceee index-pack: remove #ifdef NO_PTHREADS
 3:  a842b27e1e =  3:  93aa33eaa6 name-hash.c: remove #ifdef NO_PTHREADS
 4:  c0939283c3 =  4:  5d9441d8e2 attr.c: remove #ifdef NO_PTHREADS
 5:  6c6f3a0d28 <  -:  ---------- send-pack.c: remove #ifdef NO_PTHREADS
 6:  9f6e259fb4 !  5:  834b19d5c6 grep: remove #ifdef NO_PTHREADS
    @@ -13,6 +13,16 @@
      static pthread_t *threads;
      
      /* We use one producer thread and THREADS consumer
    +@@
    + 	int hit = 0;
    + 	int i;
    + 
    ++	if (!HAVE_THREADS)
    ++		return 0;
    ++
    + 	grep_lock();
    + 	all_work_added = 1;
    + 
     @@
      
      	return hit;
    @@ -136,15 +146,6 @@
      
      	if (show_in_pager && (cached || list.nr))
      		die(_("--open-files-in-pager only works on the worktree"));
    -@@
    - 		hit = grep_objects(&opt, &pathspec, &list);
    - 	}
    - 
    --	if (num_threads)
    -+	if (HAVE_THREADS && num_threads)
    - 		hit |= wait_all();
    - 	if (hit && show_in_pager)
    - 		run_pager(&opt, prefix);
     
      diff --git a/grep.c b/grep.c
      --- a/grep.c
 7:  97fb2fdba4 =  6:  10969f86ae preload-index.c: remove #ifdef NO_PTHREADS
 8:  b8aeb2776b =  7:  7e83190364 pack-objects: remove #ifdef NO_PTHREADS
 9:  40a9bfb8f2 =  8:  404e9d9041 read-cache.c: remove #ifdef NO_PTHREADS
10:  0bb0c667ff =  9:  046008b4ef Clean up pthread_create() error handling
 -:  ---------- > 10:  2ad9554687 read-cache.c: initialize copy_len to shut up gcc 8
-- 
2.19.1.647.g708186aaf9

[PATCH v2 01/10] thread-utils: macros to unconditionally compile pthreads API

[Nguyễn Thái Ngọc Duy]

When built with NO_PTHREADS, the macros are used make the code build
even though pthreads header and library may be missing. The code can
still have different code paths for no threads support with
HAVE_THREADS variable.

There are of course impacts on no-pthreads builds:

- data structure may get slightly bigger because all the mutexes and
  pthread_t are present (as an int)

- code execution is not impacted much. Locking (in hot path) is
  no-op. Other wrapper function calls really should not matter much.

- the binary size grows bigger because of threaded code. But at least
  on Linux this does not matter, if some code is not executed, it's
  not mapped in memory.

This is a preparation step to remove "#ifdef NO_PTHREADS" in the code
mostly because of maintainability. As Jeff put it

> it's probably OK to stop thinking of it as "non-threaded platforms
> are the default and must pay zero cost" and more as "threaded
> platforms are the default, and non-threaded ones are OK to pay a
> small cost as long as they still work".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile       |  2 +-
 thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 thread-utils.h | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index b08d5ea258..321540a736 100644
--- a/Makefile
+++ b/Makefile
@@ -991,6 +991,7 @@ LIB_OBJS += sub-process.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
+LIB_OBJS += thread-utils.o
 LIB_OBJS += tmp-objdir.o
 LIB_OBJS += trace.o
 LIB_OBJS += trailer.o
@@ -1674,7 +1675,6 @@ ifdef NO_PTHREADS
 else
 	BASIC_CFLAGS += $(PTHREAD_CFLAGS)
 	EXTLIBS += $(PTHREAD_LIBS)
-	LIB_OBJS += thread-utils.o
 endif
 
 ifdef HAVE_PATHS_H
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0743..5329845691 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -20,6 +20,9 @@
 
 int online_cpus(void)
 {
+#ifdef NO_PTHREADS
+	return 1;
+#else
 #ifdef _SC_NPROCESSORS_ONLN
 	long ncpus;
 #endif
@@ -59,10 +62,12 @@ int online_cpus(void)
 #endif
 
 	return 1;
+#endif
 }
 
 int init_recursive_mutex(pthread_mutex_t *m)
 {
+#ifndef NO_PTHREADS
 	pthread_mutexattr_t a;
 	int ret;
 
@@ -74,4 +79,47 @@ int init_recursive_mutex(pthread_mutex_t *m)
 		pthread_mutexattr_destroy(&a);
 	}
 	return ret;
+#else
+	return 0;
+#endif
+}
+
+#ifdef NO_PTHREADS
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+int dummy_pthread_init(void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis or it may realize that functions like
+	 * pthread_mutex_init() is no-op, which means the (static)
+	 * variable is not used/initialized at all and trigger
+	 * -Wunused-variable
+	 */
+	return ENOSYS;
 }
+
+int dummy_pthread_join(pthread_t pthread, void **retval)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d190..4961487ed9 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -4,12 +4,54 @@
 #ifndef NO_PTHREADS
 #include <pthread.h>
 
-extern int online_cpus(void);
-extern int init_recursive_mutex(pthread_mutex_t*);
+#define HAVE_THREADS 1
 
 #else
 
-#define online_cpus() 1
+#define HAVE_THREADS 0
+
+/*
+ * macros instead of typedefs because pthread definitions may have
+ * been pulled in by some system dependencies even though the user
+ * wants to disable pthread.
+ */
+#define pthread_t int
+#define pthread_mutex_t int
+#define pthread_cond_t int
+#define pthread_key_t int
+
+#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
+#define pthread_mutex_lock(mutex)
+#define pthread_mutex_unlock(mutex)
+#define pthread_mutex_destroy(mutex)
+
+#define pthread_cond_init(cond, attr) dummy_pthread_init(cond)
+#define pthread_cond_wait(cond, mutex)
+#define pthread_cond_signal(cond)
+#define pthread_cond_broadcast(cond)
+#define pthread_cond_destroy(cond)
+
+#define pthread_key_create(key, attr) dummy_pthread_init(key)
+#define pthread_key_delete(key)
+
+#define pthread_create(thread, attr, fn, data) \
+	dummy_pthread_create(thread, attr, fn, data)
+#define pthread_join(thread, retval) \
+	dummy_pthread_join(thread, retval)
+
+#define pthread_setspecific(key, data)
+#define pthread_getspecific(key) NULL
+
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data);
+int dummy_pthread_join(pthread_t pthread, void **retval);
+
+int dummy_pthread_init(void *);
 
 #endif
+
+int online_cpus(void);
+int init_recursive_mutex(pthread_mutex_t*);
+
+
 #endif /* THREAD_COMPAT_H */
-- 
2.19.1.647.g708186aaf9

[PATCH v2 02/10] index-pack: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 68 ++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 50 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da2..bbd66ca025 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -42,9 +42,7 @@ struct base_data {
 };
 
 struct thread_local {
-#ifndef NO_PTHREADS
 	pthread_t thread;
-#endif
 	struct base_data *base_cache;
 	size_t base_cache_used;
 	int pack_fd;
@@ -98,8 +96,6 @@ static uint32_t input_crc32;
 static int input_fd, output_fd;
 static const char *curr_pack;
 
-#ifndef NO_PTHREADS
-
 static struct thread_local *thread_data;
 static int nr_dispatched;
 static int threads_active;
@@ -179,26 +175,6 @@ static void cleanup_thread(void)
 	free(thread_data);
 }
 
-#else
-
-#define read_lock()
-#define read_unlock()
-
-#define counter_lock()
-#define counter_unlock()
-
-#define work_lock()
-#define work_unlock()
-
-#define deepest_delta_lock()
-#define deepest_delta_unlock()
-
-#define type_cas_lock()
-#define type_cas_unlock()
-
-#endif
-
-
 static int mark_link(struct object *obj, int type, void *data, struct fsck_options *options)
 {
 	if (!obj)
@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)
 
 static inline struct thread_local *get_thread_data(void)
 {
-#ifndef NO_PTHREADS
-	if (threads_active)
-		return pthread_getspecific(key);
-	assert(!threads_active &&
-	       "This should only be reached when all threads are gone");
-#endif
+	if (HAVE_THREADS) {
+		if (threads_active)
+			return pthread_getspecific(key);
+		assert(!threads_active &&
+		       "This should only be reached when all threads are gone");
+	}
 	return &nothread_data;
 }
 
-#ifndef NO_PTHREADS
 static void set_thread_data(struct thread_local *data)
 {
 	if (threads_active)
 		pthread_setspecific(key, data);
 }
-#endif
 
 static struct base_data *alloc_base_data(void)
 {
@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)
 	find_unresolved_deltas(base_obj);
 }
 
-#ifndef NO_PTHREADS
 static void *threaded_second_pass(void *data)
 {
 	set_thread_data(data);
@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data)
 	}
 	return NULL;
 }
-#endif
 
 /*
  * First pass:
@@ -1213,7 +1185,6 @@ static void resolve_deltas(void)
 		progress = start_progress(_("Resolving deltas"),
 					  nr_ref_deltas + nr_ofs_deltas);
 
-#ifndef NO_PTHREADS
 	nr_dispatched = 0;
 	if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
 		init_thread();
@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)
 		cleanup_thread();
 		return;
 	}
-#endif
 
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
@@ -1531,11 +1501,11 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
 		if (nr_threads < 0)
 			die(_("invalid number of threads specified (%d)"),
 			    nr_threads);
-#ifdef NO_PTHREADS
-		if (nr_threads != 1)
-			warning(_("no threads support, ignoring %s"), k);
-		nr_threads = 1;
-#endif
+		if (!HAVE_THREADS) {
+			if (nr_threads != 1)
+				warning(_("no threads support, ignoring %s"), k);
+			nr_threads = 1;
+		}
 		return 0;
 	}
 	return git_default_config(k, v, cb);
@@ -1723,12 +1693,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				nr_threads = strtoul(arg+10, &end, 0);
 				if (!arg[10] || *end || nr_threads < 0)
 					usage(index_pack_usage);
-#ifdef NO_PTHREADS
-				if (nr_threads != 1)
-					warning(_("no threads support, "
-						  "ignoring %s"), arg);
-				nr_threads = 1;
-#endif
+				if (!HAVE_THREADS) {
+					if (nr_threads != 1)
+						warning(_("no threads support, "
+							  "ignoring %s"), arg);
+					nr_threads = 1;
+				}
 			} else if (starts_with(arg, "--pack_header=")) {
 				struct pack_header *hdr;
 				char *c;
@@ -1791,14 +1761,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (strict)
 		opts.flags |= WRITE_IDX_STRICT;
 
-#ifndef NO_PTHREADS
-	if (!nr_threads) {
+	if (HAVE_THREADS && !nr_threads) {
 		nr_threads = online_cpus();
 		/* An experiment showed that more threads does not mean faster */
 		if (nr_threads > 3)
 			nr_threads = 3;
 	}
-#endif
 
 	curr_pack = open_pack_file(pack_name);
 	parse_pack_header();
-- 
2.19.1.647.g708186aaf9

[PATCH v2 03/10] name-hash.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index 1fcda73cb3..b3c9ac791d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -7,6 +7,7 @@
  */
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "thread-utils.h"
 
 struct dir_entry {
 	struct hashmap_entry ent;
@@ -131,22 +132,6 @@ static int cache_entry_cmp(const void *unused_cmp_data,
 static int lazy_try_threaded = 1;
 static int lazy_nr_dir_threads;
 
-#ifdef NO_PTHREADS
-
-static inline int lookup_lazy_params(struct index_state *istate)
-{
-	return 0;
-}
-
-static inline void threaded_lazy_init_name_hash(
-	struct index_state *istate)
-{
-}
-
-#else
-
-#include "thread-utils.h"
-
 /*
  * Set a minimum number of cache_entries that we will handle per
  * thread and use that to decide how many threads to run (upto
@@ -516,6 +501,9 @@ static void threaded_lazy_init_name_hash(
 	struct lazy_dir_thread_data *td_dir;
 	struct lazy_name_thread_data *td_name;
 
+	if (!HAVE_THREADS)
+		return;
+
 	k_start = 0;
 	nr_each = DIV_ROUND_UP(istate->cache_nr, lazy_nr_dir_threads);
 
@@ -574,8 +562,6 @@ static void threaded_lazy_init_name_hash(
 	free(lazy_entries);
 }
 
-#endif
-
 static void lazy_init_name_hash(struct index_state *istate)
 {
 
-- 
2.19.1.647.g708186aaf9

[PATCH v2 04/10] attr.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/attr.c b/attr.c
index 60d284796d..eaece6658d 100644
--- a/attr.c
+++ b/attr.c
@@ -41,23 +41,17 @@ const char *git_attr_name(const struct git_attr *attr)
 
 struct attr_hashmap {
 	struct hashmap map;
-#ifndef NO_PTHREADS
 	pthread_mutex_t mutex;
-#endif
 };
 
 static inline void hashmap_lock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&map->mutex);
-#endif
 }
 
 static inline void hashmap_unlock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&map->mutex);
-#endif
 }
 
 /*
@@ -498,23 +492,17 @@ static struct check_vector {
 	size_t nr;
 	size_t alloc;
 	struct attr_check **checks;
-#ifndef NO_PTHREADS
 	pthread_mutex_t mutex;
-#endif
 } check_vector;
 
 static inline void vector_lock(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&check_vector.mutex);
-#endif
 }
 
 static inline void vector_unlock(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&check_vector.mutex);
-#endif
 }
 
 static void check_vector_add(struct attr_check *c)
@@ -1181,8 +1169,6 @@ void git_all_attrs(const struct index_state *istate,
 
 void attr_start(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
 	pthread_mutex_init(&check_vector.mutex, NULL);
-#endif
 }
-- 
2.19.1.647.g708186aaf9

[PATCH v2 05/10] grep: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 59 +++++++++++++++++++-------------------------------
 grep.c         |  6 -----
 grep.h         |  6 -----
 3 files changed, 22 insertions(+), 49 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..6dd15dbaa2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@ static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static int num_threads;
 
-#ifndef NO_PTHREADS
 static pthread_t *threads;
 
 /* We use one producer thread and THREADS consumer
@@ -234,6 +233,9 @@ static int wait_all(void)
 	int hit = 0;
 	int i;
 
+	if (!HAVE_THREADS)
+		return 0;
+
 	grep_lock();
 	all_work_added = 1;
 
@@ -265,13 +267,6 @@ static int wait_all(void)
 
 	return hit;
 }
-#else /* !NO_PTHREADS */
-
-static int wait_all(void)
-{
-	return 0;
-}
-#endif
 
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
@@ -284,8 +279,7 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 		if (num_threads < 0)
 			die(_("invalid number of threads specified (%d) for %s"),
 			    num_threads, var);
-#ifdef NO_PTHREADS
-		else if (num_threads && num_threads != 1) {
+		else if (!HAVE_THREADS && num_threads && num_threads != 1) {
 			/*
 			 * TRANSLATORS: %s is the configuration
 			 * variable for tweaking threads, currently
@@ -294,7 +288,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 			warning(_("no threads support, ignoring %s"), var);
 			num_threads = 0;
 		}
-#endif
 	}
 
 	if (!strcmp(var, "submodule.recurse"))
@@ -330,17 +323,14 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 	grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
 	strbuf_release(&pathbuf);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -363,17 +353,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
 	strbuf_release(&buf);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -1038,20 +1025,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	pathspec.recursive = 1;
 	pathspec.recurse_submodules = !!recurse_submodules;
 
-#ifndef NO_PTHREADS
-	if (list.nr || cached || show_in_pager)
-		num_threads = 0;
-	else if (num_threads == 0)
-		num_threads = GREP_NUM_THREADS_DEFAULT;
-	else if (num_threads < 0)
-		die(_("invalid number of threads specified (%d)"), num_threads);
-	if (num_threads == 1)
+	if (HAVE_THREADS) {
+		if (list.nr || cached || show_in_pager)
+			num_threads = 0;
+		else if (num_threads == 0)
+			num_threads = GREP_NUM_THREADS_DEFAULT;
+		else if (num_threads < 0)
+			die(_("invalid number of threads specified (%d)"), num_threads);
+		if (num_threads == 1)
+			num_threads = 0;
+	} else {
+		if (num_threads)
+			warning(_("no threads support, ignoring --threads"));
 		num_threads = 0;
-#else
-	if (num_threads)
-		warning(_("no threads support, ignoring --threads"));
-	num_threads = 0;
-#endif
+	}
 
 	if (!num_threads)
 		/*
@@ -1062,15 +1049,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		 */
 		compile_grep_patterns(&opt);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
 		    && (opt.pre_context || opt.post_context ||
 			opt.file_break || opt.funcbody))
 			skip_first_line = 1;
 		start_threads(&opt);
 	}
-#endif
 
 	if (show_in_pager && (cached || list.nr))
 		die(_("--open-files-in-pager only works on the worktree"));
diff --git a/grep.c b/grep.c
index f6bd89e40b..4db1510d16 100644
--- a/grep.c
+++ b/grep.c
@@ -1513,7 +1513,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	}
 }
 
-#ifndef NO_PTHREADS
 int grep_use_locks;
 
 /*
@@ -1539,11 +1538,6 @@ static inline void grep_attr_unlock(void)
  */
 pthread_mutex_t grep_read_mutex;
 
-#else
-#define grep_attr_lock()
-#define grep_attr_unlock()
-#endif
-
 static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
 {
 	xdemitconf_t *xecfg = opt->priv;
diff --git a/grep.h b/grep.h
index 1a57d12b90..fb04893721 100644
--- a/grep.h
+++ b/grep.h
@@ -229,7 +229,6 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs);
 extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
 extern int grep_threads_ok(const struct grep_opt *opt);
 
-#ifndef NO_PTHREADS
 /*
  * Mutex used around access to the attributes machinery if
  * opt->use_threads.  Must be initialized/destroyed by callers!
@@ -250,9 +249,4 @@ static inline void grep_read_unlock(void)
 		pthread_mutex_unlock(&grep_read_mutex);
 }
 
-#else
-#define grep_read_lock()
-#define grep_read_unlock()
-#endif
-
 #endif
-- 
2.19.1.647.g708186aaf9

[PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 preload-index.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,17 +7,7 @@
 #include "fsmonitor.h"
 #include "config.h"
 #include "progress.h"
-
-#ifdef NO_PTHREADS
-static void preload_index(struct index_state *index,
-			  const struct pathspec *pathspec,
-			  unsigned int refresh_flags)
-{
-	; /* nothing */
-}
-#else
-
-#include <pthread.h>
+#include "thread-utils.h"
 
 /*
  * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,
 	struct thread_data data[MAX_PARALLEL];
 	struct progress_data pd;
 
-	if (!core_preload_index)
+	if (!HAVE_THREADS || !core_preload_index)
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,
 
 	trace_performance_leave("preload index");
 }
-#endif
 
 int read_index_preload(struct index_state *index,
 		       const struct pathspec *pathspec,
-- 
2.19.1.647.g708186aaf9

[PATCH v2 07/10] pack-objects: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 26 ++------------------------
 pack-objects.h         |  6 ------
 2 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..12edd6da16 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1953,8 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 	return 0;
 }
 
-#ifndef NO_PTHREADS
-
 /* Protect access to object database */
 static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
@@ -1979,16 +1977,6 @@ static pthread_mutex_t progress_mutex;
  * ahead in the list because they can be stolen and would need
  * progress_mutex for protection.
  */
-#else
-
-#define read_lock()		(void)0
-#define read_unlock()		(void)0
-#define cache_lock()		(void)0
-#define cache_unlock()		(void)0
-#define progress_lock()		(void)0
-#define progress_unlock()	(void)0
-
-#endif
 
 /*
  * Return the size of the object without doing any delta
@@ -2347,8 +2335,6 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 	free(array);
 }
 
-#ifndef NO_PTHREADS
-
 static void try_to_free_from_threads(size_t size)
 {
 	read_lock();
@@ -2578,10 +2564,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	free(p);
 }
 
-#else
-#define ll_find_deltas(l, s, w, d, p)	find_deltas(l, &s, w, d, p)
-#endif
-
 static void add_tag_chain(const struct object_id *oid)
 {
 	struct tag *tag;
@@ -2734,12 +2716,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 		if (delta_search_threads < 0)
 			die(_("invalid number of threads specified (%d)"),
 			    delta_search_threads);
-#ifdef NO_PTHREADS
-		if (delta_search_threads != 1) {
+		if (!HAVE_THREADS && delta_search_threads != 1) {
 			warning(_("no threads support, ignoring %s"), k);
 			delta_search_threads = 0;
 		}
-#endif
 		return 0;
 	}
 	if (!strcmp(k, "pack.indexversion")) {
@@ -3402,10 +3382,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!delta_search_threads)	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
 
-#ifdef NO_PTHREADS
-	if (delta_search_threads != 1)
+	if (!HAVE_THREADS && delta_search_threads != 1)
 		warning(_("no threads support, ignoring --threads"));
-#endif
 	if (!pack_to_stdout && !pack_size_limit)
 		pack_size_limit = pack_size_limit_cfg;
 	if (pack_to_stdout && pack_size_limit)
diff --git a/pack-objects.h b/pack-objects.h
index 2ca39cfcfe..3a42727c7d 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -145,9 +145,7 @@ struct packing_data {
 	struct packed_git **in_pack_by_idx;
 	struct packed_git **in_pack;
 
-#ifndef NO_PTHREADS
 	pthread_mutex_t lock;
-#endif
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -169,15 +167,11 @@ void prepare_packing_data(struct packing_data *pdata);
 
 static inline void packing_data_lock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&pdata->lock);
-#endif
 }
 static inline void packing_data_unlock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&pdata->lock);
-#endif
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
-- 
2.19.1.647.g708186aaf9

[PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 49 ++++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index d57958233e..ba870bc3fd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
 	struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
 	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
 	return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 
 	return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	size_t mmap_size;
 	struct load_index_extensions p;
 	size_t extension_offset = 0;
-#ifndef NO_PTHREADS
 	int nr_threads, cpus;
 	struct index_entry_offset_table *ieot = NULL;
-#endif
 
 	if (istate->initialized)
 		return istate->cache_nr;
@@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS) {
+		nr_threads = git_config_get_index_threads();
 
-	/* TODO: does creating more threads than cores help? */
-	if (!nr_threads) {
-		nr_threads = istate->cache_nr / THREAD_COST;
-		cpus = online_cpus();
-		if (nr_threads > cpus)
-			nr_threads = cpus;
+		/* TODO: does creating more threads than cores help? */
+		if (!nr_threads) {
+			nr_threads = istate->cache_nr / THREAD_COST;
+			cpus = online_cpus();
+			if (nr_threads > cpus)
+				nr_threads = cpus;
+		}
+	} else {
+		nr_threads = 1;
 	}
 
 	if (nr_threads > 1) {
@@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
 	}
-#else
-	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-#endif
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-#ifndef NO_PTHREADS
-	if (extension_offset) {
+	if (HAVE_THREADS && extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
 	}
-#endif
 	if (!extension_offset) {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
@@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS)
+		nr_threads = git_config_get_index_threads();
+	else
+		nr_threads = 1;
+
 	if (nr_threads != 1) {
 		int ieot_blocks, cpus;
 
@@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
 		}
 	}
-#endif
 
 	offset = lseek(newfd, 0, SEEK_CUR);
 	if (offset < 0) {
@@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-#ifndef NO_PTHREADS
-	if (ieot) {
+	if (HAVE_THREADS && ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
@@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		if (err)
 			return -1;
 	}
-#endif
 
 	if (!strip_extensions && istate->split_index) {
 		struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
 	strbuf_add(sb, hash, the_hash_algo->rawsz);
 }
 
-#ifndef NO_PTHREADS
 #define IEOT_VERSION	(1)
 
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
@@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
 	       strbuf_add(sb, &buffer, sizeof(uint32_t));
        }
 }
-#endif
-- 
2.19.1.647.g708186aaf9

[PATCH v2 09/10] Clean up pthread_create() error handling

[Nguyễn Thái Ngọc Duy]

Normally pthread_create() rarely fails and sometimes error handling
can be neglected. But with new pthreads wrapper, pthread_create() will
return ENOSYS on a system without thread support.

Threaded code _is_ protected by HAVE_THREADS and pthread_create()
should never run in the first place. But the situation could change in
the future and bugs may sneak in. Make sure that all pthread_create()
checks and handles the result.

While at there, mark these strings for translation if they aren't.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c     | 16 ++++++++++------
 preload-index.c |  8 ++++++--
 run-command.c   |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index b3c9ac791d..623ca6923a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -494,6 +494,7 @@ static inline void lazy_update_dir_ref_counts(
 static void threaded_lazy_init_name_hash(
 	struct index_state *istate)
 {
+	int err;
 	int nr_each;
 	int k_start;
 	int t;
@@ -526,8 +527,9 @@ static void threaded_lazy_init_name_hash(
 		if (k_start > istate->cache_nr)
 			k_start = istate->cache_nr;
 		td_dir_t->k_end = k_start;
-		if (pthread_create(&td_dir_t->pthread, NULL, lazy_dir_thread_proc, td_dir_t))
-			die("unable to create lazy_dir_thread");
+		err = pthread_create(&td_dir_t->pthread, NULL, lazy_dir_thread_proc, td_dir_t);
+		if (err)
+			die(_("unable to create lazy_dir thread: %s"), strerror(err));
 	}
 	for (t = 0; t < lazy_nr_dir_threads; t++) {
 		struct lazy_dir_thread_data *td_dir_t = td_dir + t;
@@ -547,13 +549,15 @@ static void threaded_lazy_init_name_hash(
 	 */
 	td_name->istate = istate;
 	td_name->lazy_entries = lazy_entries;
-	if (pthread_create(&td_name->pthread, NULL, lazy_name_thread_proc, td_name))
-		die("unable to create lazy_name_thread");
+	err = pthread_create(&td_name->pthread, NULL, lazy_name_thread_proc, td_name);
+	if (err)
+		die(_("unable to create lazy_name thread: %s"), strerror(err));
 
 	lazy_update_dir_ref_counts(istate, lazy_entries);
 
-	if (pthread_join(td_name->pthread, NULL))
-		die("unable to join lazy_name_thread");
+	err = pthread_join(td_name->pthread, NULL);
+	if (err)
+		die(_("unable to join lazy_name thread: %s"), strerror(err));
 
 	cleanup_dir_mutex();
 
diff --git a/preload-index.c b/preload-index.c
index 0e24886aca..ddca1c216e 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -121,6 +121,8 @@ static void preload_index(struct index_state *index,
 
 	for (i = 0; i < threads; i++) {
 		struct thread_data *p = data+i;
+		int err;
+
 		p->index = index;
 		if (pathspec)
 			copy_pathspec(&p->pathspec, pathspec);
@@ -129,8 +131,10 @@ static void preload_index(struct index_state *index,
 		if (pd.progress)
 			p->progress = &pd;
 		offset += work;
-		if (pthread_create(&p->pthread, NULL, preload_thread, p))
-			die("unable to create threaded lstat");
+		err = pthread_create(&p->pthread, NULL, preload_thread, p);
+
+		if (err)
+			die(_("unable to create threaded lstat: %s"), strerror(err));
 	}
 	for (i = 0; i < threads; i++) {
 		struct thread_data *p = data+i;
diff --git a/run-command.c b/run-command.c
index 84b883c213..26154ba257 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1213,7 +1213,7 @@ int start_async(struct async *async)
 	{
 		int err = pthread_create(&async->tid, NULL, run_thread, async);
 		if (err) {
-			error_errno("cannot create thread");
+			error(_("cannot create async thread: %s"), strerror(err));
 			goto error;
 		}
 	}
-- 
2.19.1.647.g708186aaf9

[PATCH v2 10/10] read-cache.c: initialize copy_len to shut up gcc 8

[Nguyễn Thái Ngọc Duy]

It was reported that when building with NO_PTHREADS=1,
-Wmaybe-uninitialized is triggered. Just initialize the variable from
the beginning to shut the compiler up (because this warning is enabled
in config.dev)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index ba870bc3fd..4307b9a7bf 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1746,7 +1746,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 	size_t len;
 	const char *name;
 	unsigned int flags;
-	size_t copy_len;
+	size_t copy_len = 0;
 	/*
 	 * Adjacent cache entries tend to share the leading paths, so it makes
 	 * sense to only store the differences in later entries.  In the v4
@@ -1786,8 +1786,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 				die(_("malformed name field in the index, near path '%s'"),
 					previous_ce->name);
 			copy_len = previous_len - strip_len;
-		} else {
-			copy_len = 0;
 		}
 		name = (const char *)cp;
 	}
-- 
2.19.1.647.g708186aaf9

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Cases like this are kind of weird. I'd expect to see wait_all() return
> immediately when !HAVE_THREADS. But we don't need to, because later we
> do this:
>
>> -	if (num_threads)
>> +	if (HAVE_THREADS && num_threads)
>>  		hit |= wait_all();
>
> Which I think works, but it feels like we're introducing some subtle
> dependencies about which functions get called in which cases. I'd hoped
> in general that the non-threaded code paths could mostly just collapse
> down into the same as the "threads == 1" cases, and we wouldn't have to
> ask "are threads even supported" in a lot of places.

True, but the way "grep" code works with threads is already strange,
and I suspect that the subtle strangeness you feel mostly comes from
that.  The single threaded code signaled "hit" with return value of
individual functions like grep_file(), but the meaning of return
value from them is changed to "does not matter--we do not have
meaningful result yet at this point" when threading is used.

In the new world order where "threading is the norm but
single-threaded is still supported", I wonder if we would want to
drive the single threaded case the same way with the add_work(run)
interface and return the "did we see hits?" etc. from the same (or
at lesat "more similar than today's") interface, so that the flow of
data smells the same in both cases.

> I dunno. My comment isn't really an objection exactly, but mostly just
> an indication that I'm still thinking through what the best approach is,
> and what end state we want to end up in.
>
> -Peff

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Jeff King]

On Mon, Oct 29, 2018 at 11:16:39AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Cases like this are kind of weird. I'd expect to see wait_all() return
> > immediately when !HAVE_THREADS. But we don't need to, because later we
> > do this:
> >
> >> -	if (num_threads)
> >> +	if (HAVE_THREADS && num_threads)
> >>  		hit |= wait_all();
> >
> > Which I think works, but it feels like we're introducing some subtle
> > dependencies about which functions get called in which cases. I'd hoped
> > in general that the non-threaded code paths could mostly just collapse
> > down into the same as the "threads == 1" cases, and we wouldn't have to
> > ask "are threads even supported" in a lot of places.
> 
> True, but the way "grep" code works with threads is already strange,
> and I suspect that the subtle strangeness you feel mostly comes from
> that.  The single threaded code signaled "hit" with return value of
> individual functions like grep_file(), but the meaning of return
> value from them is changed to "does not matter--we do not have
> meaningful result yet at this point" when threading is used.
> 
> In the new world order where "threading is the norm but
> single-threaded is still supported", I wonder if we would want to
> drive the single threaded case the same way with the add_work(run)
> interface and return the "did we see hits?" etc. from the same (or
> at lesat "more similar than today's") interface, so that the flow of
> data smells the same in both cases.

Right, your second paragraph here is a better statement of what I was
trying to get at. ;)

But if the problem is simply that we are not quite there yet in the grep
code, I am OK with taking this as the first pass, and knowing that there
is more cleanup to be done later (though that sort of thing is IMHO very
useful in a commit message).

-Peff

Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

[Jeff King]

On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:

> -#ifndef NO_PTHREADS
> -	nr_threads = git_config_get_index_threads();
> +	if (HAVE_THREADS) {
> +		nr_threads = git_config_get_index_threads();
>  
> -	/* TODO: does creating more threads than cores help? */
> -	if (!nr_threads) {
> -		nr_threads = istate->cache_nr / THREAD_COST;
> -		cpus = online_cpus();
> -		if (nr_threads > cpus)
> -			nr_threads = cpus;
> +		/* TODO: does creating more threads than cores help? */
> +		if (!nr_threads) {
> +			nr_threads = istate->cache_nr / THREAD_COST;
> +			cpus = online_cpus();
> +			if (nr_threads > cpus)
> +				nr_threads = cpus;
> +		}
> +	} else {
> +		nr_threads = 1;
>  	}

I'd have thought we could just rely on online_cpus() returning 1 here to
avoid having to ask "do we even have thread support?". But I guess that
TODO comment implies that we might one day two 2 threads on a single
CPU.

-Peff

Re: [PATCH v2 10/10] read-cache.c: initialize copy_len to shut up gcc 8

[Jeff King]

On Sat, Oct 27, 2018 at 07:30:08PM +0200, Nguyễn Thái Ngọc Duy wrote:

> It was reported that when building with NO_PTHREADS=1,
> -Wmaybe-uninitialized is triggered. Just initialize the variable from
> the beginning to shut the compiler up (because this warning is enabled
> in config.dev)
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  read-cache.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index ba870bc3fd..4307b9a7bf 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1746,7 +1746,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>  	size_t len;
>  	const char *name;
>  	unsigned int flags;
> -	size_t copy_len;
> +	size_t copy_len = 0;
>  	/*
>  	 * Adjacent cache entries tend to share the leading paths, so it makes
>  	 * sense to only store the differences in later entries.  In the v4
> @@ -1786,8 +1786,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>  				die(_("malformed name field in the index, near path '%s'"),
>  					previous_ce->name);
>  			copy_len = previous_len - strip_len;
> -		} else {
> -			copy_len = 0;
>  		}
>  		name = (const char *)cp;
>  	}

Yes, this silences the compiler warning I saw (and is exactly the same
patch I wrote to get past it the other day ;) ).

-Peff

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Duy Nguyen]

On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
> But if the problem is simply that we are not quite there yet in the grep
> code, I am OK with taking this as the first pass, and knowing that there
> is more cleanup to be done later (though that sort of thing is IMHO very
> useful in a commit message).

Since the problem pops up now, I'm ok with updating/cleaning up all
this in this series, unless there's benefits in keeping this series
simple and merging it early (probably not?)
-- 
Duy

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Jeff King]

On Mon, Oct 29, 2018 at 05:01:41PM +0100, Duy Nguyen wrote:

> On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
> > But if the problem is simply that we are not quite there yet in the grep
> > code, I am OK with taking this as the first pass, and knowing that there
> > is more cleanup to be done later (though that sort of thing is IMHO very
> > useful in a commit message).
> 
> Since the problem pops up now, I'm ok with updating/cleaning up all
> this in this series, unless there's benefits in keeping this series
> simple and merging it early (probably not?)

Mostly I did not want to tax you. I would rather have this series and
some cleanup left over, than to not have anything. But if you are
interested in moving it further, I will not say no. :)

-Peff

Re: [PATCH 07/10] preload-index.c: remove #ifdef NO_PTHREADS

[Ben Peart]

On 10/27/2018 3:10 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   preload-index.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/preload-index.c b/preload-index.c
> index 9e7152ab14..0e24886aca 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -7,17 +7,7 @@
>   #include "fsmonitor.h"
>   #include "config.h"
>   #include "progress.h"
> -
> -#ifdef NO_PTHREADS
> -static void preload_index(struct index_state *index,
> -			  const struct pathspec *pathspec,
> -			  unsigned int refresh_flags)
> -{
> -	; /* nothing */
> -}
> -#else
> -
> -#include <pthread.h>
> +#include "thread-utils.h"
>   
>   /*
>    * Mostly randomly chosen maximum thread counts: we
> @@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,
>   	struct thread_data data[MAX_PARALLEL];
>   	struct progress_data pd;
>   
> -	if (!core_preload_index)
> +	if (!HAVE_THREADS || !core_preload_index)

I also "hoped in general that the non-threaded code paths could mostly 
just collapse into the same as the "threads == 1" cases, and we wouldn't 
have to "are threads even supported" in a lot of places."

In this case, if we cap the threads to online_cpus() the later test for 
'if (threads < 2)' would do that.  I haven't measured this specific case 
but in the other cases I have measured, having more threads than cpus 
did not result in a performance win.

>   		return;
>   
>   	threads = index->cache_nr / THREAD_COST;
> @@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,
>   
>   	trace_performance_leave("preload index");
>   }
> -#endif
>   
>   int read_index_preload(struct index_state *index,
>   		       const struct pathspec *pathspec,
> 

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

[Ben Peart]

On 10/27/2018 3:10 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   read-cache.c | 49 ++++++++++++++++++-------------------------------
>   1 file changed, 18 insertions(+), 31 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index d57958233e..ba870bc3fd 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1920,19 +1920,15 @@ struct index_entry_offset_table
>   	struct index_entry_offset entries[FLEX_ARRAY];
>   };
>   
> -#ifndef NO_PTHREADS
>   static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
>   static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
> -#endif
>   
>   static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
>   static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
>   
>   struct load_index_extensions
>   {
> -#ifndef NO_PTHREADS
>   	pthread_t pthread;
> -#endif
>   	struct index_state *istate;
>   	const char *mmap;
>   	size_t mmap_size;
> @@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
>   	return consumed;
>   }
>   
> -#ifndef NO_PTHREADS
> -
>   /*
>    * Mostly randomly chosen maximum thread counts: we
>    * cap the parallelism to online_cpus() threads, and we want
> @@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
>   
>   	return consumed;
>   }
> -#endif
>   
>   /* remember to discard_cache() before reading a different cache! */
>   int do_read_index(struct index_state *istate, const char *path, int must_exist)
> @@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>   	size_t mmap_size;
>   	struct load_index_extensions p;
>   	size_t extension_offset = 0;
> -#ifndef NO_PTHREADS
>   	int nr_threads, cpus;
>   	struct index_entry_offset_table *ieot = NULL;
> -#endif
>   
>   	if (istate->initialized)
>   		return istate->cache_nr;
> @@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>   
>   	src_offset = sizeof(*hdr);
>   
> -#ifndef NO_PTHREADS
> -	nr_threads = git_config_get_index_threads();
> +	if (HAVE_THREADS) {

In this case, nr_threads is already capped to online_cpus() below so 
this HAVE_THREADS test isn't needed.

> +		nr_threads = git_config_get_index_threads();
>   
> -	/* TODO: does creating more threads than cores help? */
> -	if (!nr_threads) {
> -		nr_threads = istate->cache_nr / THREAD_COST;
> -		cpus = online_cpus();
> -		if (nr_threads > cpus)
> -			nr_threads = cpus;
> +		/* TODO: does creating more threads than cores help? */
> +		if (!nr_threads) {
> +			nr_threads = istate->cache_nr / THREAD_COST;
> +			cpus = online_cpus();
> +			if (nr_threads > cpus)
> +				nr_threads = cpus;
> +		}
> +	} else {
> +		nr_threads = 1;
>   	}
>   
>   	if (nr_threads > 1) {
> @@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>   	} else {
>   		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
>   	}
> -#else
> -	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
> -#endif
>   
>   	istate->timestamp.sec = st.st_mtime;
>   	istate->timestamp.nsec = ST_MTIME_NSEC(st);
>   
>   	/* if we created a thread, join it otherwise load the extensions on the primary thread */
> -#ifndef NO_PTHREADS
> -	if (extension_offset) {
> +	if (HAVE_THREADS && extension_offset) {

Here extension_offset won't be set if there wasn't a thread created so 
the HAVE_THREADS test is not needed.

>   		int ret = pthread_join(p.pthread, NULL);
>   		if (ret)
>   			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
>   	}
> -#endif
>   	if (!extension_offset) {
>   		p.src_offset = src_offset;
>   		load_index_extensions(&p);
> @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
>   		return -1;
>   
> -#ifndef NO_PTHREADS
> -	nr_threads = git_config_get_index_threads();
> +	if (HAVE_THREADS)

This shouldn't be needed either.  My assumption was that if someone 
explicitly asked for multiple threads we're better off failing than 
silently ignoring their request.  The default/automatic setting will 
detect the number of cpus and behave correctly.

> +		nr_threads = git_config_get_index_threads();
> +	else
> +		nr_threads = 1;
> +
>   	if (nr_threads != 1) {
>   		int ieot_blocks, cpus;
>   
> @@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
>   		}
>   	}
> -#endif
>   
>   	offset = lseek(newfd, 0, SEEK_CUR);
>   	if (offset < 0) {
> @@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   	 * strip_extensions parameter as we need it when loading the shared
>   	 * index.
>   	 */
> -#ifndef NO_PTHREADS
> -	if (ieot) {
> +	if (HAVE_THREADS && ieot) {

Again, this one isn't needed.  If there is only 1 thread, we don't read 
the eoie or the ieot (see the code just above this) so the ieot test is 
sufficient.

>   		struct strbuf sb = STRBUF_INIT;
>   
>   		write_ieot_extension(&sb, ieot);
> @@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   		if (err)
>   			return -1;
>   	}
> -#endif
>   
>   	if (!strip_extensions && istate->split_index) {
>   		struct strbuf sb = STRBUF_INIT;
> @@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
>   	strbuf_add(sb, hash, the_hash_algo->rawsz);
>   }
>   
> -#ifndef NO_PTHREADS
>   #define IEOT_VERSION	(1)
>   
>   static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
> @@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
>   	       strbuf_add(sb, &buffer, sizeof(uint32_t));
>          }
>   }
> -#endif
> 

Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

[Ben Peart]

On 10/29/2018 10:30 AM, Jeff King wrote:
> On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:
> 
>> -#ifndef NO_PTHREADS
>> -	nr_threads = git_config_get_index_threads();
>> +	if (HAVE_THREADS) {
>> +		nr_threads = git_config_get_index_threads();
>>   
>> -	/* TODO: does creating more threads than cores help? */
>> -	if (!nr_threads) {
>> -		nr_threads = istate->cache_nr / THREAD_COST;
>> -		cpus = online_cpus();
>> -		if (nr_threads > cpus)
>> -			nr_threads = cpus;
>> +		/* TODO: does creating more threads than cores help? */
>> +		if (!nr_threads) {
>> +			nr_threads = istate->cache_nr / THREAD_COST;
>> +			cpus = online_cpus();
>> +			if (nr_threads > cpus)
>> +				nr_threads = cpus;
>> +		}
>> +	} else {
>> +		nr_threads = 1;
>>   	}
> 
> I'd have thought we could just rely on online_cpus() returning 1 here to
> avoid having to ask "do we even have thread support?". But I guess that
> TODO comment implies that we might one day two 2 threads on a single
> CPU.
> 
> -Peff
> 

See my earlier response - the HAVE_THREADS tests are not needed.

We can remove the "TODO" comment - I tested it and it wasn't faster to 
have more threads than CPUs.

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

[Ben Peart]

On 10/27/2018 1:30 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   preload-index.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/preload-index.c b/preload-index.c
> index 9e7152ab14..0e24886aca 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -7,17 +7,7 @@
>   #include "fsmonitor.h"
>   #include "config.h"
>   #include "progress.h"
> -
> -#ifdef NO_PTHREADS
> -static void preload_index(struct index_state *index,
> -			  const struct pathspec *pathspec,
> -			  unsigned int refresh_flags)
> -{
> -	; /* nothing */
> -}
> -#else
> -
> -#include <pthread.h>
> +#include "thread-utils.h"
>   
>   /*
>    * Mostly randomly chosen maximum thread counts: we
> @@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,
>   	struct thread_data data[MAX_PARALLEL];
>   	struct progress_data pd;
>   
> -	if (!core_preload_index)
> +	if (!HAVE_THREADS || !core_preload_index)
>   		return;
>   
>   	threads = index->cache_nr / THREAD_COST;
> @@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,
>   
>   	trace_performance_leave("preload index");
>   }
> -#endif
>   
>   int read_index_preload(struct index_state *index,
>   		       const struct pathspec *pathspec,
> 


In the theory that code speaks louder than comments, here is the patch I 
used when testing out the idea of getting rid of NO_PTHREADS:


  git diff head~1 preload-index.c
diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..266bc9d8d7 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,17 +7,7 @@
  #include "fsmonitor.h"
  #include "config.h"
  #include "progress.h"
-
-#ifdef NO_PTHREADS
-static void preload_index(struct index_state *index,
-                         const struct pathspec *pathspec,
-                         unsigned int refresh_flags)
-{
-       ; /* nothing */
-}
-#else
-
-#include <pthread.h>
+#include "thread-utils.h"

  /*
   * Mostly randomly chosen maximum thread counts: we
@@ -104,7 +94,7 @@ static void preload_index(struct index_state *index,
                           const struct pathspec *pathspec,
                           unsigned int refresh_flags)
  {
-       int threads, i, work, offset;
+       int cpus, threads, i, work, offset;
         struct thread_data data[MAX_PARALLEL];
         struct progress_data pd;

@@ -114,6 +104,9 @@ static void preload_index(struct index_state *index,
         threads = index->cache_nr / THREAD_COST;
         if ((index->cache_nr > 1) && (threads < 2) && 
git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
                 threads = 2;
+       cpus = online_cpus();
+       if (threads > cpus)
+               threads = cpus;
         if (threads < 2)
                 return;
         trace_performance_enter();
@@ -151,7 +144,6 @@ static void preload_index(struct index_state *index,

         trace_performance_leave("preload index");
  }
-#endif

  int read_index_preload(struct index_state *index,
                        const struct pathspec *pathspec,

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

[Duy Nguyen]

On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote:
> > @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
> >       if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
> >               return -1;
> >
> > -#ifndef NO_PTHREADS
> > -     nr_threads = git_config_get_index_threads();
> > +     if (HAVE_THREADS)
>
> This shouldn't be needed either.  My assumption was that if someone
> explicitly asked for multiple threads we're better off failing than
> silently ignoring their request.  The default/automatic setting will
> detect the number of cpus and behave correctly.

No. I can have ~/.gitconfig shared between different machines. One
with multithread support and one without. I should not have to fall
back to conditional includes in order to use the same config file on
the machine without multithread.
-- 
Duy

Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

[Ben Peart]

On 10/29/2018 10:30 AM, Jeff King wrote:
> On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:
> 
>> -#ifndef NO_PTHREADS
>> -	nr_threads = git_config_get_index_threads();
>> +	if (HAVE_THREADS) {
>> +		nr_threads = git_config_get_index_threads();
>>   
>> -	/* TODO: does creating more threads than cores help? */
>> -	if (!nr_threads) {
>> -		nr_threads = istate->cache_nr / THREAD_COST;
>> -		cpus = online_cpus();
>> -		if (nr_threads > cpus)
>> -			nr_threads = cpus;
>> +		/* TODO: does creating more threads than cores help? */
>> +		if (!nr_threads) {
>> +			nr_threads = istate->cache_nr / THREAD_COST;
>> +			cpus = online_cpus();
>> +			if (nr_threads > cpus)
>> +				nr_threads = cpus;
>> +		}
>> +	} else {
>> +		nr_threads = 1;
>>   	}
> 
> I'd have thought we could just rely on online_cpus() returning 1 here to
> avoid having to ask "do we even have thread support?". But I guess that
> TODO comment implies that we might one day two 2 threads on a single
> CPU.
> 
> -Peff
> 

And here is the patch I created when testing out the idea of removing 
NO_PTHREADS.  It doesn't require any 'HAVE_THREADS' tests.



diff --git a/read-cache.c b/read-cache.c
index 1df5c16dbc..1f088799fc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
         struct index_entry_offset entries[FLEX_ARRAY];
  };

-#ifndef NO_PTHREADS
  static struct index_entry_offset_table *read_ieot_extension(const char 
*mmap, size_t mmap_size, size_t offset
);
  static void write_ieot_extension(struct strbuf *sb, struct 
index_entry_offset_table *ieot);
-#endif

  static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
  static void write_eoie_extension(struct strbuf *sb, git_hash_ctx 
*eoie_context, size_t offset);

  struct load_index_extensions
  {
-#ifndef NO_PTHREADS
         pthread_t pthread;
-#endif
         struct index_state *istate;
         const char *mmap;
         size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct 
index_state *istate,
         return consumed;
  }

-#ifndef NO_PTHREADS
-
  /*
   * Mostly randomly chosen maximum thread counts: we
   * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long 
load_cache_entries_threaded(struct index_state *istate, con


         return consumed;
  }
-#endif

  /* remember to discard_cache() before reading a different cache! */
  int do_read_index(struct index_state *istate, const char *path, int 
must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, 
const char *path, int must_exist)

         size_t mmap_size;
         struct load_index_extensions p;
         size_t extension_offset = 0;
-#ifndef NO_PTHREADS
         int nr_threads, cpus;
         struct index_entry_offset_table *ieot = NULL;
-#endif

         if (istate->initialized)
                 return istate->cache_nr;
@@ -2181,16 +2172,12 @@ int do_read_index(struct index_state *istate, 
const char *path, int must_exist
)

         src_offset = sizeof(*hdr);

-#ifndef NO_PTHREADS
         nr_threads = git_config_get_index_threads();
-
-       /* TODO: does creating more threads than cores help? */
-       if (!nr_threads) {
+       if (!nr_threads)
                 nr_threads = istate->cache_nr / THREAD_COST;
-               cpus = online_cpus();
-               if (nr_threads > cpus)
-                       nr_threads = cpus;
-       }
+       cpus = online_cpus();
+       if (nr_threads > cpus)
+               nr_threads = cpus;

         if (nr_threads > 1) {
                 extension_offset = read_eoie_extension(mmap, mmap_size);
@@ -2219,22 +2206,16 @@ int do_read_index(struct index_state *istate, 
const char *path, int must_exist
)
         } else {
                 src_offset += load_all_cache_entries(istate, mmap, 
mmap_size, src_offset);
         }
-#else
-       src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);
-#endif

         istate->timestamp.sec = st.st_mtime;
         istate->timestamp.nsec = ST_MTIME_NSEC(st);

         /* if we created a thread, join it otherwise load the 
extensions on the primary thread */
-#ifndef NO_PTHREADS
         if (extension_offset) {
                 int ret = pthread_join(p.pthread, NULL);
                 if (ret)
                         die(_("unable to join load_index_extensions 
thread: %s"), strerror(ret));
-       }
-#endif
-       if (!extension_offset) {
+       } else {
                 p.src_offset = src_offset;
                 load_index_extensions(&p);
         }
@@ -2756,7 +2737,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

         if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
                 return -1;

-#ifndef NO_PTHREADS
         nr_threads = git_config_get_index_threads();
         if (nr_threads != 1) {
                 int ieot_blocks, cpus;
@@ -2787,7 +2767,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

                         ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
                 }
         }
-#endif

         offset = lseek(newfd, 0, SEEK_CUR);
         if (offset < 0) {
@@ -2871,7 +2850,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

          * strip_extensions parameter as we need it when loading the shared
          * index.
          */
-#ifndef NO_PTHREADS
         if (ieot) {
                 struct strbuf sb = STRBUF_INIT;

@@ -2883,7 +2861,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

                 if (err)
                         return -1;
         }
-#endif

         if (!strip_extensions && istate->split_index) {
                 struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3446,6 @@ static void write_eoie_extension(struct strbuf 
*sb, git_hash_ctx *eoie_context,

         strbuf_add(sb, hash, the_hash_algo->rawsz);
  }

-#ifndef NO_PTHREADS
  #define IEOT_VERSION   (1)

  static struct index_entry_offset_table *read_ieot_extension(const char 
*mmap, size_t mmap_size, size_t offset
)
@@ -3542,4 +3518,3 @@ static void write_ieot_extension(struct strbuf 
*sb, struct index_entry_offset_ta

                strbuf_add(sb, &buffer, sizeof(uint32_t));
         }
  }
-#endif

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

[Duy Nguyen]

On Mon, Oct 29, 2018 at 6:21 PM Ben Peart <peartben@gmail.com> wrote:
> @@ -114,6 +104,9 @@ static void preload_index(struct index_state *index,
>          threads = index->cache_nr / THREAD_COST;
>          if ((index->cache_nr > 1) && (threads < 2) &&
> git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
>                  threads = 2;
> +       cpus = online_cpus();
> +       if (threads > cpus)
> +               threads = cpus;
>          if (threads < 2)
>                  return;
>          trace_performance_enter();

Capping the number of threads to online_cpus() does not always make
sense. In this case (or at least the original use case where we stat()
over nfs) we want to issue as many requests to nfs server as possible
to reduce latency and it has nothing to do with how many cores we
have. Using more threads than cores might make sense since threads are
blocked by nfs client, but we still want to send more to the server.
-- 
Duy

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

[Ben Peart]

On 10/29/2018 1:21 PM, Duy Nguyen wrote:
> On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote:
>>> @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>>>        if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
>>>                return -1;
>>>
>>> -#ifndef NO_PTHREADS
>>> -     nr_threads = git_config_get_index_threads();
>>> +     if (HAVE_THREADS)
>>
>> This shouldn't be needed either.  My assumption was that if someone
>> explicitly asked for multiple threads we're better off failing than
>> silently ignoring their request.  The default/automatic setting will
>> detect the number of cpus and behave correctly.
> 
> No. I can have ~/.gitconfig shared between different machines. One
> with multithread support and one without. I should not have to fall
> back to conditional includes in order to use the same config file on
> the machine without multithread.
> 

If you want to share the ~/.gitconfig across machines that have 
different numbers of CPUs, it makes more sense to me to leave the 
setting at the "auto" setting rather than specifically overriding it to 
a number that won't work on at least one of your machines.  Then it all 
"just works" without the need to conditional includes.

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

[Ben Peart]

On 10/29/2018 1:26 PM, Duy Nguyen wrote:
> On Mon, Oct 29, 2018 at 6:21 PM Ben Peart <peartben@gmail.com> wrote:
>> @@ -114,6 +104,9 @@ static void preload_index(struct index_state *index,
>>           threads = index->cache_nr / THREAD_COST;
>>           if ((index->cache_nr > 1) && (threads < 2) &&
>> git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
>>                   threads = 2;
>> +       cpus = online_cpus();
>> +       if (threads > cpus)
>> +               threads = cpus;
>>           if (threads < 2)
>>                   return;
>>           trace_performance_enter();
> 
> Capping the number of threads to online_cpus() does not always make
> sense. In this case (or at least the original use case where we stat()
> over nfs) we want to issue as many requests to nfs server as possible
> to reduce latency and it has nothing to do with how many cores we
> have. Using more threads than cores might make sense since threads are
> blocked by nfs client, but we still want to send more to the server.
> 

That makes sense.  Some test will be necessary then.

I guess HAVE_THREADS is functionally the same as online_cpus() == 1. 
For some reason, I prefer the latter - probably because I'm typically 
already calling it and so it doesn't feel like a 'special' value/test I 
have to remember. I just know I need to make sure the logic works 
correctly with any number of cps greater than zero. :-)

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

[Jeff King]

On Mon, Oct 29, 2018 at 02:05:58PM -0400, Ben Peart wrote:

> On 10/29/2018 1:26 PM, Duy Nguyen wrote:
> > On Mon, Oct 29, 2018 at 6:21 PM Ben Peart <peartben@gmail.com> wrote:
> > > @@ -114,6 +104,9 @@ static void preload_index(struct index_state *index,
> > >           threads = index->cache_nr / THREAD_COST;
> > >           if ((index->cache_nr > 1) && (threads < 2) &&
> > > git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
> > >                   threads = 2;
> > > +       cpus = online_cpus();
> > > +       if (threads > cpus)
> > > +               threads = cpus;
> > >           if (threads < 2)
> > >                   return;
> > >           trace_performance_enter();
> > 
> > Capping the number of threads to online_cpus() does not always make
> > sense. In this case (or at least the original use case where we stat()
> > over nfs) we want to issue as many requests to nfs server as possible
> > to reduce latency and it has nothing to do with how many cores we
> > have. Using more threads than cores might make sense since threads are
> > blocked by nfs client, but we still want to send more to the server.
> > 
> 
> That makes sense.  Some test will be necessary then.
> 
> I guess HAVE_THREADS is functionally the same as online_cpus() == 1. For
> some reason, I prefer the latter - probably because I'm typically already
> calling it and so it doesn't feel like a 'special' value/test I have to
> remember. I just know I need to make sure the logic works correctly with any
> number of cps greater than zero. :-)

I don't think they're functionally the same. If you see online_cpus()
== 1, you are in one of two cases:

  - the system supports threads, but CPU-bound tasks should stick to a
    single thread

  - the system does not even support threading, and calling
    pthread_create() will give you ENOSYS

In the first case, if your task is _not_ CPU bound (i.e., the stat()
thing), then you want to distinguish those cases.

I'm not sure if I'm violently agreeing, but it just wasn't clear to me
from the above discussion that we all agreed that HAVE_THREADS is still
necessary in some cases.

(I do think in cases where it is not, that we would do just as well to
avoid it; if that is all you were saying, then I agree. ;) ).

-Peff

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Oct 29, 2018 at 05:01:41PM +0100, Duy Nguyen wrote:
>
>> On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
>> > But if the problem is simply that we are not quite there yet in the grep
>> > code, I am OK with taking this as the first pass, and knowing that there
>> > is more cleanup to be done later (though that sort of thing is IMHO very
>> > useful in a commit message).
>> 
>> Since the problem pops up now, I'm ok with updating/cleaning up all
>> this in this series, unless there's benefits in keeping this series
>> simple and merging it early (probably not?)
>
> Mostly I did not want to tax you. I would rather have this series and
> some cleanup left over, than to not have anything. But if you are
> interested in moving it further, I will not say no. :)

Likewise.

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> This shouldn't be needed either.  My assumption was that if someone
>> explicitly asked for multiple threads we're better off failing than
>> silently ignoring their request.  The default/automatic setting will
>> detect the number of cpus and behave correctly.
>
> No. I can have ~/.gitconfig shared between different machines. One
> with multithread support and one without. I should not have to fall
> back to conditional includes in order to use the same config file on
> the machine without multithread.

Our attitude has been to fail unsatisfyable requests from the
command line loudly to let the users' know, and tolerate having
elements from the future versions of Git in the configuration file
by ignoring them or tweaking them (silently or with a warning).

So this hunk is in line with the current practice, and we should
keep it this way.  It is a separate discussion if we want to rethink
that whole attitude, that discussion may be worth having, and I
think that discussion is wider than this single topic.

[PATCH v3 00/14] Reduce #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Changes since v2

- more cleanups in grep.c, read-cache.c and index-pack.c
- the send-pack.c changes are back, but this time I just add
  async_with_fork() to move NO_PTHREADS back in run-command.c

For grep.c and read-cache.c, changes are split in two patches. The
first one is a dumb, mechanical conversion from #ifdef to if and is
straightforward. The second one makes "no thread" use "one thread"
code path and needs more careful review.

Diff against nd/pthreads

diff --git a/builtin/grep.c b/builtin/grep.c
index 6dd15dbaa2..de3f568cee 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -69,13 +69,11 @@ static pthread_mutex_t grep_mutex;
 
 static inline void grep_lock(void)
 {
-	assert(num_threads);
 	pthread_mutex_lock(&grep_mutex);
 }
 
 static inline void grep_unlock(void)
 {
-	assert(num_threads);
 	pthread_mutex_unlock(&grep_mutex);
 }
 
@@ -234,7 +232,7 @@ static int wait_all(void)
 	int i;
 
 	if (!HAVE_THREADS)
-		return 0;
+		BUG("Never call this function unless you have started threads");
 
 	grep_lock();
 	all_work_added = 1;
@@ -279,14 +277,14 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 		if (num_threads < 0)
 			die(_("invalid number of threads specified (%d) for %s"),
 			    num_threads, var);
-		else if (!HAVE_THREADS && num_threads && num_threads != 1) {
+		else if (!HAVE_THREADS && num_threads > 1) {
 			/*
 			 * TRANSLATORS: %s is the configuration
 			 * variable for tweaking threads, currently
 			 * grep.threads
 			 */
 			warning(_("no threads support, ignoring %s"), var);
-			num_threads = 0;
+			num_threads = 1;
 		}
 	}
 
@@ -323,7 +321,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 	grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
 	strbuf_release(&pathbuf);
 
-	if (HAVE_THREADS && num_threads) {
+	if (num_threads > 1) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
@@ -353,7 +351,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
 	strbuf_release(&buf);
 
-	if (HAVE_THREADS && num_threads) {
+	if (num_threads > 1) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
@@ -1025,36 +1023,34 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	pathspec.recursive = 1;
 	pathspec.recurse_submodules = !!recurse_submodules;
 
-	if (HAVE_THREADS) {
-		if (list.nr || cached || show_in_pager)
-			num_threads = 0;
-		else if (num_threads == 0)
-			num_threads = GREP_NUM_THREADS_DEFAULT;
-		else if (num_threads < 0)
-			die(_("invalid number of threads specified (%d)"), num_threads);
-		if (num_threads == 1)
-			num_threads = 0;
+	if (list.nr || cached || show_in_pager) {
+		if (num_threads > 1)
+			warning(_("invalid option combination, ignoring --threads"));
+		num_threads = 1;
+	} else if (!HAVE_THREADS && num_threads > 1) {
+		warning(_("no threads support, ignoring --threads"));
+		num_threads = 1;
+	} else if (num_threads < 0)
+		die(_("invalid number of threads specified (%d)"), num_threads);
+	else if (num_threads == 0)
+		num_threads = HAVE_THREADS ? GREP_NUM_THREADS_DEFAULT : 1;
+
+	if (num_threads > 1) {
+		if (!HAVE_THREADS)
+			BUG("Somebody got num_threads calculation wrong!");
+		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
+		    && (opt.pre_context || opt.post_context ||
+			opt.file_break || opt.funcbody))
+			skip_first_line = 1;
+		start_threads(&opt);
 	} else {
-		if (num_threads)
-			warning(_("no threads support, ignoring --threads"));
-		num_threads = 0;
-	}
-
-	if (!num_threads)
 		/*
 		 * The compiled patterns on the main path are only
 		 * used when not using threading. Otherwise
-		 * start_threads() below calls compile_grep_patterns()
+		 * start_threads() above calls compile_grep_patterns()
 		 * for each thread.
 		 */
 		compile_grep_patterns(&opt);
-
-	if (HAVE_THREADS && num_threads) {
-		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
-		    && (opt.pre_context || opt.post_context ||
-			opt.file_break || opt.funcbody))
-			skip_first_line = 1;
-		start_threads(&opt);
 	}
 
 	if (show_in_pager && (cached || list.nr))
@@ -1106,7 +1102,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		hit = grep_objects(&opt, &pathspec, &list);
 	}
 
-	if (num_threads)
+	if (num_threads > 1)
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index bbd66ca025..682042579b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1501,9 +1501,8 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
 		if (nr_threads < 0)
 			die(_("invalid number of threads specified (%d)"),
 			    nr_threads);
-		if (!HAVE_THREADS) {
-			if (nr_threads != 1)
-				warning(_("no threads support, ignoring %s"), k);
+		if (!HAVE_THREADS && nr_threads != 1) {
+			warning(_("no threads support, ignoring %s"), k);
 			nr_threads = 1;
 		}
 		return 0;
@@ -1693,10 +1692,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				nr_threads = strtoul(arg+10, &end, 0);
 				if (!arg[10] || *end || nr_threads < 0)
 					usage(index_pack_usage);
-				if (!HAVE_THREADS) {
-					if (nr_threads != 1)
-						warning(_("no threads support, "
-							  "ignoring %s"), arg);
+				if (!HAVE_THREADS && nr_threads != 1) {
+					warning(_("no threads support, ignoring %s"), arg);
 					nr_threads = 1;
 				}
 			} else if (starts_with(arg, "--pack_header=")) {
diff --git a/read-cache.c b/read-cache.c
index 4307b9a7bf..c510f598b1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2170,20 +2170,19 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-	if (HAVE_THREADS) {
-		nr_threads = git_config_get_index_threads();
+	nr_threads = git_config_get_index_threads();
 
-		/* TODO: does creating more threads than cores help? */
-		if (!nr_threads) {
-			nr_threads = istate->cache_nr / THREAD_COST;
-			cpus = online_cpus();
-			if (nr_threads > cpus)
-				nr_threads = cpus;
-		}
-	} else {
-		nr_threads = 1;
+	/* TODO: does creating more threads than cores help? */
+	if (!nr_threads) {
+		nr_threads = istate->cache_nr / THREAD_COST;
+		cpus = online_cpus();
+		if (nr_threads > cpus)
+			nr_threads = cpus;
 	}
 
+	if (!HAVE_THREADS)
+		nr_threads = 1;
+
 	if (nr_threads > 1) {
 		extension_offset = read_eoie_extension(mmap, mmap_size);
 		if (extension_offset) {
@@ -2216,12 +2215,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-	if (HAVE_THREADS && extension_offset) {
+	if (extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
-	}
-	if (!extension_offset) {
+	} else {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
 	}
@@ -2860,7 +2858,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-	if (HAVE_THREADS && ieot) {
+	if (ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
diff --git a/run-command.c b/run-command.c
index 26154ba257..decf3239bd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1246,6 +1246,15 @@ int finish_async(struct async *async)
 #endif
 }
 
+int async_with_fork(void)
+{
+#ifdef NO_PTHREADS
+	return 1;
+#else
+	return 0;
+#endif
+}
+
 const char *find_hook(const char *name)
 {
 	static struct strbuf path = STRBUF_INIT;
diff --git a/run-command.h b/run-command.h
index 3932420ec8..68f5369fc2 100644
--- a/run-command.h
+++ b/run-command.h
@@ -1,9 +1,7 @@
 #ifndef RUN_COMMAND_H
 #define RUN_COMMAND_H
 
-#ifndef NO_PTHREADS
-#include <pthread.h>
-#endif
+#include "thread-utils.h"
 
 #include "argv-array.h"
 
@@ -143,6 +141,7 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 int in_async(void);
+int async_with_fork(void);
 void check_pipe(int err);
 
 /**
diff --git a/send-pack.c b/send-pack.c
index e920ca57df..f692686770 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -203,9 +203,8 @@ static int receive_status(int in, struct ref *refs)
 static int sideband_demux(int in, int out, void *data)
 {
 	int *fd = data, ret;
-#ifdef NO_PTHREADS
-	close(fd[1]);
-#endif
+	if (async_with_fork())
+		close(fd[1]);
 	ret = recv_sideband("send-pack", fd[0], out);
 	close(out);
 	return ret;

Nguyễn Thái Ngọc Duy (14):
  thread-utils: macros to unconditionally compile pthreads API
  run-command.h: include thread-utils.h instead of pthread.h
  send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c
  index-pack: remove #ifdef NO_PTHREADS
  name-hash.c: remove #ifdef NO_PTHREADS
  attr.c: remove #ifdef NO_PTHREADS
  grep: remove #ifdef NO_PTHREADS
  grep: clean up num_threads handling
  preload-index.c: remove #ifdef NO_PTHREADS
  pack-objects: remove #ifdef NO_PTHREADS
  read-cache.c: remove #ifdef NO_PTHREADS
  read-cache.c: reduce branching based on HAVE_THREADS
  read-cache.c: initialize copy_len to shut up gcc 8
  Clean up pthread_create() error handling

 Makefile               |  2 +-
 attr.c                 | 14 --------
 builtin/grep.c         | 79 ++++++++++++++++--------------------------
 builtin/index-pack.c   | 63 ++++++++-------------------------
 builtin/pack-objects.c | 26 ++------------
 grep.c                 |  6 ----
 grep.h                 |  6 ----
 name-hash.c            | 38 ++++++++------------
 pack-objects.h         |  6 ----
 preload-index.c        | 23 +++++-------
 read-cache.c           | 37 ++++++--------------
 run-command.c          | 11 +++++-
 run-command.h          |  5 ++-
 send-pack.c            |  5 ++-
 thread-utils.c         | 48 +++++++++++++++++++++++++
 thread-utils.h         | 48 +++++++++++++++++++++++--
 16 files changed, 186 insertions(+), 231 deletions(-)

-- 
2.19.1.1005.gac84295441

[PATCH v3 01/14] thread-utils: macros to unconditionally compile pthreads API

[Nguyễn Thái Ngọc Duy]

When built with NO_PTHREADS, the macros are used make the code build
even though pthreads header and library may be missing. The code can
still have different code paths for no threads support with
HAVE_THREADS variable.

There are of course impacts on no-pthreads builds:

- data structure may get slightly bigger because all the mutexes and
  pthread_t are present (as an int)

- code execution is not impacted much. Locking (in hot path) is
  no-op. Other wrapper function calls really should not matter much.

- the binary size grows bigger because of threaded code. But at least
  on Linux this does not matter, if some code is not executed, it's
  not mapped in memory.

This is a preparation step to remove "#ifdef NO_PTHREADS" in the code
mostly because of maintainability. As Jeff put it

> it's probably OK to stop thinking of it as "non-threaded platforms
> are the default and must pay zero cost" and more as "threaded
> platforms are the default, and non-threaded ones are OK to pay a
> small cost as long as they still work".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile       |  2 +-
 thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 thread-utils.h | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index b08d5ea258..321540a736 100644
--- a/Makefile
+++ b/Makefile
@@ -991,6 +991,7 @@ LIB_OBJS += sub-process.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
+LIB_OBJS += thread-utils.o
 LIB_OBJS += tmp-objdir.o
 LIB_OBJS += trace.o
 LIB_OBJS += trailer.o
@@ -1674,7 +1675,6 @@ ifdef NO_PTHREADS
 else
 	BASIC_CFLAGS += $(PTHREAD_CFLAGS)
 	EXTLIBS += $(PTHREAD_LIBS)
-	LIB_OBJS += thread-utils.o
 endif
 
 ifdef HAVE_PATHS_H
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0743..5329845691 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -20,6 +20,9 @@
 
 int online_cpus(void)
 {
+#ifdef NO_PTHREADS
+	return 1;
+#else
 #ifdef _SC_NPROCESSORS_ONLN
 	long ncpus;
 #endif
@@ -59,10 +62,12 @@ int online_cpus(void)
 #endif
 
 	return 1;
+#endif
 }
 
 int init_recursive_mutex(pthread_mutex_t *m)
 {
+#ifndef NO_PTHREADS
 	pthread_mutexattr_t a;
 	int ret;
 
@@ -74,4 +79,47 @@ int init_recursive_mutex(pthread_mutex_t *m)
 		pthread_mutexattr_destroy(&a);
 	}
 	return ret;
+#else
+	return 0;
+#endif
+}
+
+#ifdef NO_PTHREADS
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+int dummy_pthread_init(void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis or it may realize that functions like
+	 * pthread_mutex_init() is no-op, which means the (static)
+	 * variable is not used/initialized at all and trigger
+	 * -Wunused-variable
+	 */
+	return ENOSYS;
 }
+
+int dummy_pthread_join(pthread_t pthread, void **retval)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d190..4961487ed9 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -4,12 +4,54 @@
 #ifndef NO_PTHREADS
 #include <pthread.h>
 
-extern int online_cpus(void);
-extern int init_recursive_mutex(pthread_mutex_t*);
+#define HAVE_THREADS 1
 
 #else
 
-#define online_cpus() 1
+#define HAVE_THREADS 0
+
+/*
+ * macros instead of typedefs because pthread definitions may have
+ * been pulled in by some system dependencies even though the user
+ * wants to disable pthread.
+ */
+#define pthread_t int
+#define pthread_mutex_t int
+#define pthread_cond_t int
+#define pthread_key_t int
+
+#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
+#define pthread_mutex_lock(mutex)
+#define pthread_mutex_unlock(mutex)
+#define pthread_mutex_destroy(mutex)
+
+#define pthread_cond_init(cond, attr) dummy_pthread_init(cond)
+#define pthread_cond_wait(cond, mutex)
+#define pthread_cond_signal(cond)
+#define pthread_cond_broadcast(cond)
+#define pthread_cond_destroy(cond)
+
+#define pthread_key_create(key, attr) dummy_pthread_init(key)
+#define pthread_key_delete(key)
+
+#define pthread_create(thread, attr, fn, data) \
+	dummy_pthread_create(thread, attr, fn, data)
+#define pthread_join(thread, retval) \
+	dummy_pthread_join(thread, retval)
+
+#define pthread_setspecific(key, data)
+#define pthread_getspecific(key) NULL
+
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data);
+int dummy_pthread_join(pthread_t pthread, void **retval);
+
+int dummy_pthread_init(void *);
 
 #endif
+
+int online_cpus(void);
+int init_recursive_mutex(pthread_mutex_t*);
+
+
 #endif /* THREAD_COMPAT_H */
-- 
2.19.1.1005.gac84295441

[PATCH v3 02/14] run-command.h: include thread-utils.h instead of pthread.h

[Nguyễn Thái Ngọc Duy]

run-command.c may use threads for its async support. But instead of
including pthread.h directly, let's include thread-utils.h.

run-command.c probably never needs the dummy bits in thread-utils.h
when NO_PTHREADS is defined. But this makes sure we have consistent
HAVE_THREADS behavior everywhere. From now on outside compat/,
thread-utils.h is the only place that includes pthread.h

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 run-command.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/run-command.h b/run-command.h
index 3932420ec8..9b7f38202c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -1,9 +1,7 @@
 #ifndef RUN_COMMAND_H
 #define RUN_COMMAND_H
 
-#ifndef NO_PTHREADS
-#include <pthread.h>
-#endif
+#include "thread-utils.h"
 
 #include "argv-array.h"
 
-- 
2.19.1.1005.gac84295441

[PATCH v3 03/14] send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c

[Nguyễn Thái Ngọc Duy]

On systems that do not support multithread, start_async() is
implemented with fork(). This implementation details unfortunately
leak out at least in send-pack.c [1].

To keep the code base clean of NO_PTHREADS, move the this #ifdef back
to run-command.c. The new wrapper function async_with_fork() at least
helps suggest that this special "close()" is related to async in fork
mode.

[1] 09c9957cf7 (send-pack: avoid deadlock when pack-object dies early
    - 2011-04-25)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 run-command.c | 9 +++++++++
 run-command.h | 1 +
 send-pack.c   | 5 ++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index 84b883c213..3c3b8814df 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1246,6 +1246,15 @@ int finish_async(struct async *async)
 #endif
 }
 
+int async_with_fork(void)
+{
+#ifdef NO_PTHREADS
+	return 1;
+#else
+	return 0;
+#endif
+}
+
 const char *find_hook(const char *name)
 {
 	static struct strbuf path = STRBUF_INIT;
diff --git a/run-command.h b/run-command.h
index 9b7f38202c..68f5369fc2 100644
--- a/run-command.h
+++ b/run-command.h
@@ -141,6 +141,7 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 int in_async(void);
+int async_with_fork(void);
 void check_pipe(int err);
 
 /**
diff --git a/send-pack.c b/send-pack.c
index e920ca57df..f692686770 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -203,9 +203,8 @@ static int receive_status(int in, struct ref *refs)
 static int sideband_demux(int in, int out, void *data)
 {
 	int *fd = data, ret;
-#ifdef NO_PTHREADS
-	close(fd[1]);
-#endif
+	if (async_with_fork())
+		close(fd[1]);
 	ret = recv_sideband("send-pack", fd[0], out);
 	close(out);
 	return ret;
-- 
2.19.1.1005.gac84295441

[PATCH v3 04/14] index-pack: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 63 ++++++++++----------------------------------
 1 file changed, 14 insertions(+), 49 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da2..682042579b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -42,9 +42,7 @@ struct base_data {
 };
 
 struct thread_local {
-#ifndef NO_PTHREADS
 	pthread_t thread;
-#endif
 	struct base_data *base_cache;
 	size_t base_cache_used;
 	int pack_fd;
@@ -98,8 +96,6 @@ static uint32_t input_crc32;
 static int input_fd, output_fd;
 static const char *curr_pack;
 
-#ifndef NO_PTHREADS
-
 static struct thread_local *thread_data;
 static int nr_dispatched;
 static int threads_active;
@@ -179,26 +175,6 @@ static void cleanup_thread(void)
 	free(thread_data);
 }
 
-#else
-
-#define read_lock()
-#define read_unlock()
-
-#define counter_lock()
-#define counter_unlock()
-
-#define work_lock()
-#define work_unlock()
-
-#define deepest_delta_lock()
-#define deepest_delta_unlock()
-
-#define type_cas_lock()
-#define type_cas_unlock()
-
-#endif
-
-
 static int mark_link(struct object *obj, int type, void *data, struct fsck_options *options)
 {
 	if (!obj)
@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)
 
 static inline struct thread_local *get_thread_data(void)
 {
-#ifndef NO_PTHREADS
-	if (threads_active)
-		return pthread_getspecific(key);
-	assert(!threads_active &&
-	       "This should only be reached when all threads are gone");
-#endif
+	if (HAVE_THREADS) {
+		if (threads_active)
+			return pthread_getspecific(key);
+		assert(!threads_active &&
+		       "This should only be reached when all threads are gone");
+	}
 	return &nothread_data;
 }
 
-#ifndef NO_PTHREADS
 static void set_thread_data(struct thread_local *data)
 {
 	if (threads_active)
 		pthread_setspecific(key, data);
 }
-#endif
 
 static struct base_data *alloc_base_data(void)
 {
@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)
 	find_unresolved_deltas(base_obj);
 }
 
-#ifndef NO_PTHREADS
 static void *threaded_second_pass(void *data)
 {
 	set_thread_data(data);
@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data)
 	}
 	return NULL;
 }
-#endif
 
 /*
  * First pass:
@@ -1213,7 +1185,6 @@ static void resolve_deltas(void)
 		progress = start_progress(_("Resolving deltas"),
 					  nr_ref_deltas + nr_ofs_deltas);
 
-#ifndef NO_PTHREADS
 	nr_dispatched = 0;
 	if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
 		init_thread();
@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)
 		cleanup_thread();
 		return;
 	}
-#endif
 
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
@@ -1531,11 +1501,10 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
 		if (nr_threads < 0)
 			die(_("invalid number of threads specified (%d)"),
 			    nr_threads);
-#ifdef NO_PTHREADS
-		if (nr_threads != 1)
+		if (!HAVE_THREADS && nr_threads != 1) {
 			warning(_("no threads support, ignoring %s"), k);
-		nr_threads = 1;
-#endif
+			nr_threads = 1;
+		}
 		return 0;
 	}
 	return git_default_config(k, v, cb);
@@ -1723,12 +1692,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				nr_threads = strtoul(arg+10, &end, 0);
 				if (!arg[10] || *end || nr_threads < 0)
 					usage(index_pack_usage);
-#ifdef NO_PTHREADS
-				if (nr_threads != 1)
-					warning(_("no threads support, "
-						  "ignoring %s"), arg);
-				nr_threads = 1;
-#endif
+				if (!HAVE_THREADS && nr_threads != 1) {
+					warning(_("no threads support, ignoring %s"), arg);
+					nr_threads = 1;
+				}
 			} else if (starts_with(arg, "--pack_header=")) {
 				struct pack_header *hdr;
 				char *c;
@@ -1791,14 +1758,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (strict)
 		opts.flags |= WRITE_IDX_STRICT;
 
-#ifndef NO_PTHREADS
-	if (!nr_threads) {
+	if (HAVE_THREADS && !nr_threads) {
 		nr_threads = online_cpus();
 		/* An experiment showed that more threads does not mean faster */
 		if (nr_threads > 3)
 			nr_threads = 3;
 	}
-#endif
 
 	curr_pack = open_pack_file(pack_name);
 	parse_pack_header();
-- 
2.19.1.1005.gac84295441

[PATCH v3 05/14] name-hash.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index 1fcda73cb3..b3c9ac791d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -7,6 +7,7 @@
  */
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "thread-utils.h"
 
 struct dir_entry {
 	struct hashmap_entry ent;
@@ -131,22 +132,6 @@ static int cache_entry_cmp(const void *unused_cmp_data,
 static int lazy_try_threaded = 1;
 static int lazy_nr_dir_threads;
 
-#ifdef NO_PTHREADS
-
-static inline int lookup_lazy_params(struct index_state *istate)
-{
-	return 0;
-}
-
-static inline void threaded_lazy_init_name_hash(
-	struct index_state *istate)
-{
-}
-
-#else
-
-#include "thread-utils.h"
-
 /*
  * Set a minimum number of cache_entries that we will handle per
  * thread and use that to decide how many threads to run (upto
@@ -516,6 +501,9 @@ static void threaded_lazy_init_name_hash(
 	struct lazy_dir_thread_data *td_dir;
 	struct lazy_name_thread_data *td_name;
 
+	if (!HAVE_THREADS)
+		return;
+
 	k_start = 0;
 	nr_each = DIV_ROUND_UP(istate->cache_nr, lazy_nr_dir_threads);
 
@@ -574,8 +562,6 @@ static void threaded_lazy_init_name_hash(
 	free(lazy_entries);
 }
 
-#endif
-
 static void lazy_init_name_hash(struct index_state *istate)
 {
 
-- 
2.19.1.1005.gac84295441

[PATCH v3 06/14] attr.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/attr.c b/attr.c
index 60d284796d..eaece6658d 100644
--- a/attr.c
+++ b/attr.c
@@ -41,23 +41,17 @@ const char *git_attr_name(const struct git_attr *attr)
 
 struct attr_hashmap {
 	struct hashmap map;
-#ifndef NO_PTHREADS
 	pthread_mutex_t mutex;
-#endif
 };
 
 static inline void hashmap_lock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&map->mutex);
-#endif
 }
 
 static inline void hashmap_unlock(struct attr_hashmap *map)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&map->mutex);
-#endif
 }
 
 /*
@@ -498,23 +492,17 @@ static struct check_vector {
 	size_t nr;
 	size_t alloc;
 	struct attr_check **checks;
-#ifndef NO_PTHREADS
 	pthread_mutex_t mutex;
-#endif
 } check_vector;
 
 static inline void vector_lock(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&check_vector.mutex);
-#endif
 }
 
 static inline void vector_unlock(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&check_vector.mutex);
-#endif
 }
 
 static void check_vector_add(struct attr_check *c)
@@ -1181,8 +1169,6 @@ void git_all_attrs(const struct index_state *istate,
 
 void attr_start(void)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
 	pthread_mutex_init(&check_vector.mutex, NULL);
-#endif
 }
-- 
2.19.1.1005.gac84295441

[PATCH v3 07/14] grep: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

This is a faithful conversion without attempting to improve
anything. That comes later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 59 +++++++++++++++++++-------------------------------
 grep.c         |  6 -----
 grep.h         |  6 -----
 3 files changed, 22 insertions(+), 49 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..6dd15dbaa2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@ static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static int num_threads;
 
-#ifndef NO_PTHREADS
 static pthread_t *threads;
 
 /* We use one producer thread and THREADS consumer
@@ -234,6 +233,9 @@ static int wait_all(void)
 	int hit = 0;
 	int i;
 
+	if (!HAVE_THREADS)
+		return 0;
+
 	grep_lock();
 	all_work_added = 1;
 
@@ -265,13 +267,6 @@ static int wait_all(void)
 
 	return hit;
 }
-#else /* !NO_PTHREADS */
-
-static int wait_all(void)
-{
-	return 0;
-}
-#endif
 
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
@@ -284,8 +279,7 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 		if (num_threads < 0)
 			die(_("invalid number of threads specified (%d) for %s"),
 			    num_threads, var);
-#ifdef NO_PTHREADS
-		else if (num_threads && num_threads != 1) {
+		else if (!HAVE_THREADS && num_threads && num_threads != 1) {
 			/*
 			 * TRANSLATORS: %s is the configuration
 			 * variable for tweaking threads, currently
@@ -294,7 +288,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 			warning(_("no threads support, ignoring %s"), var);
 			num_threads = 0;
 		}
-#endif
 	}
 
 	if (!strcmp(var, "submodule.recurse"))
@@ -330,17 +323,14 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 	grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
 	strbuf_release(&pathbuf);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -363,17 +353,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
 	strbuf_release(&buf);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -1038,20 +1025,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	pathspec.recursive = 1;
 	pathspec.recurse_submodules = !!recurse_submodules;
 
-#ifndef NO_PTHREADS
-	if (list.nr || cached || show_in_pager)
-		num_threads = 0;
-	else if (num_threads == 0)
-		num_threads = GREP_NUM_THREADS_DEFAULT;
-	else if (num_threads < 0)
-		die(_("invalid number of threads specified (%d)"), num_threads);
-	if (num_threads == 1)
+	if (HAVE_THREADS) {
+		if (list.nr || cached || show_in_pager)
+			num_threads = 0;
+		else if (num_threads == 0)
+			num_threads = GREP_NUM_THREADS_DEFAULT;
+		else if (num_threads < 0)
+			die(_("invalid number of threads specified (%d)"), num_threads);
+		if (num_threads == 1)
+			num_threads = 0;
+	} else {
+		if (num_threads)
+			warning(_("no threads support, ignoring --threads"));
 		num_threads = 0;
-#else
-	if (num_threads)
-		warning(_("no threads support, ignoring --threads"));
-	num_threads = 0;
-#endif
+	}
 
 	if (!num_threads)
 		/*
@@ -1062,15 +1049,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		 */
 		compile_grep_patterns(&opt);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
 		    && (opt.pre_context || opt.post_context ||
 			opt.file_break || opt.funcbody))
 			skip_first_line = 1;
 		start_threads(&opt);
 	}
-#endif
 
 	if (show_in_pager && (cached || list.nr))
 		die(_("--open-files-in-pager only works on the worktree"));
diff --git a/grep.c b/grep.c
index f6bd89e40b..4db1510d16 100644
--- a/grep.c
+++ b/grep.c
@@ -1513,7 +1513,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	}
 }
 
-#ifndef NO_PTHREADS
 int grep_use_locks;
 
 /*
@@ -1539,11 +1538,6 @@ static inline void grep_attr_unlock(void)
  */
 pthread_mutex_t grep_read_mutex;
 
-#else
-#define grep_attr_lock()
-#define grep_attr_unlock()
-#endif
-
 static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
 {
 	xdemitconf_t *xecfg = opt->priv;
diff --git a/grep.h b/grep.h
index 1a57d12b90..fb04893721 100644
--- a/grep.h
+++ b/grep.h
@@ -229,7 +229,6 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs);
 extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
 extern int grep_threads_ok(const struct grep_opt *opt);
 
-#ifndef NO_PTHREADS
 /*
  * Mutex used around access to the attributes machinery if
  * opt->use_threads.  Must be initialized/destroyed by callers!
@@ -250,9 +249,4 @@ static inline void grep_read_unlock(void)
 		pthread_mutex_unlock(&grep_read_mutex);
 }
 
-#else
-#define grep_read_lock()
-#define grep_read_unlock()
-#endif
-
 #endif
-- 
2.19.1.1005.gac84295441

[PATCH v3 08/14] grep: clean up num_threads handling

[Nguyễn Thái Ngọc Duy]

When NO_PTHREADS is still used in this file, we have two separate code
paths for thread and no thread support. The latter will always have
num_threads remain zero while the former uses num_threads zero as
"default number of threads".

With recent changes blur the line between thread and no-thread
support, this num_threads handling becomes a bit strange so let's
redefine it like this:

- num_threads == 0 means default number of threads and should become
  positive after all configuration and option parsing is done if
  multithread is supported.

- num_threads <= 1 runs no threads. It does not matter if the platform
  supports threading or not.

- num_threads > 1 will run multiple threads and is invalid if
  HAVE_THREADS is false. pthread API is only used in this case.

PS. a new warning is also added when num_threads is forced back to one
because a thread-incompatible option is specified.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 58 +++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6dd15dbaa2..de3f568cee 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -69,13 +69,11 @@ static pthread_mutex_t grep_mutex;
 
 static inline void grep_lock(void)
 {
-	assert(num_threads);
 	pthread_mutex_lock(&grep_mutex);
 }
 
 static inline void grep_unlock(void)
 {
-	assert(num_threads);
 	pthread_mutex_unlock(&grep_mutex);
 }
 
@@ -234,7 +232,7 @@ static int wait_all(void)
 	int i;
 
 	if (!HAVE_THREADS)
-		return 0;
+		BUG("Never call this function unless you have started threads");
 
 	grep_lock();
 	all_work_added = 1;
@@ -279,14 +277,14 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 		if (num_threads < 0)
 			die(_("invalid number of threads specified (%d) for %s"),
 			    num_threads, var);
-		else if (!HAVE_THREADS && num_threads && num_threads != 1) {
+		else if (!HAVE_THREADS && num_threads > 1) {
 			/*
 			 * TRANSLATORS: %s is the configuration
 			 * variable for tweaking threads, currently
 			 * grep.threads
 			 */
 			warning(_("no threads support, ignoring %s"), var);
-			num_threads = 0;
+			num_threads = 1;
 		}
 	}
 
@@ -323,7 +321,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 	grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
 	strbuf_release(&pathbuf);
 
-	if (HAVE_THREADS && num_threads) {
+	if (num_threads > 1) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
@@ -353,7 +351,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
 	strbuf_release(&buf);
 
-	if (HAVE_THREADS && num_threads) {
+	if (num_threads > 1) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
@@ -1025,36 +1023,34 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	pathspec.recursive = 1;
 	pathspec.recurse_submodules = !!recurse_submodules;
 
-	if (HAVE_THREADS) {
-		if (list.nr || cached || show_in_pager)
-			num_threads = 0;
-		else if (num_threads == 0)
-			num_threads = GREP_NUM_THREADS_DEFAULT;
-		else if (num_threads < 0)
-			die(_("invalid number of threads specified (%d)"), num_threads);
-		if (num_threads == 1)
-			num_threads = 0;
+	if (list.nr || cached || show_in_pager) {
+		if (num_threads > 1)
+			warning(_("invalid option combination, ignoring --threads"));
+		num_threads = 1;
+	} else if (!HAVE_THREADS && num_threads > 1) {
+		warning(_("no threads support, ignoring --threads"));
+		num_threads = 1;
+	} else if (num_threads < 0)
+		die(_("invalid number of threads specified (%d)"), num_threads);
+	else if (num_threads == 0)
+		num_threads = HAVE_THREADS ? GREP_NUM_THREADS_DEFAULT : 1;
+
+	if (num_threads > 1) {
+		if (!HAVE_THREADS)
+			BUG("Somebody got num_threads calculation wrong!");
+		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
+		    && (opt.pre_context || opt.post_context ||
+			opt.file_break || opt.funcbody))
+			skip_first_line = 1;
+		start_threads(&opt);
 	} else {
-		if (num_threads)
-			warning(_("no threads support, ignoring --threads"));
-		num_threads = 0;
-	}
-
-	if (!num_threads)
 		/*
 		 * The compiled patterns on the main path are only
 		 * used when not using threading. Otherwise
-		 * start_threads() below calls compile_grep_patterns()
+		 * start_threads() above calls compile_grep_patterns()
 		 * for each thread.
 		 */
 		compile_grep_patterns(&opt);
-
-	if (HAVE_THREADS && num_threads) {
-		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
-		    && (opt.pre_context || opt.post_context ||
-			opt.file_break || opt.funcbody))
-			skip_first_line = 1;
-		start_threads(&opt);
 	}
 
 	if (show_in_pager && (cached || list.nr))
@@ -1106,7 +1102,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		hit = grep_objects(&opt, &pathspec, &list);
 	}
 
-	if (num_threads)
+	if (num_threads > 1)
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
-- 
2.19.1.1005.gac84295441

[PATCH v3 09/14] preload-index.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 preload-index.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,17 +7,7 @@
 #include "fsmonitor.h"
 #include "config.h"
 #include "progress.h"
-
-#ifdef NO_PTHREADS
-static void preload_index(struct index_state *index,
-			  const struct pathspec *pathspec,
-			  unsigned int refresh_flags)
-{
-	; /* nothing */
-}
-#else
-
-#include <pthread.h>
+#include "thread-utils.h"
 
 /*
  * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,
 	struct thread_data data[MAX_PARALLEL];
 	struct progress_data pd;
 
-	if (!core_preload_index)
+	if (!HAVE_THREADS || !core_preload_index)
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,
 
 	trace_performance_leave("preload index");
 }
-#endif
 
 int read_index_preload(struct index_state *index,
 		       const struct pathspec *pathspec,
-- 
2.19.1.1005.gac84295441

[PATCH v3 10/14] pack-objects: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 26 ++------------------------
 pack-objects.h         |  6 ------
 2 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..12edd6da16 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1953,8 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 	return 0;
 }
 
-#ifndef NO_PTHREADS
-
 /* Protect access to object database */
 static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
@@ -1979,16 +1977,6 @@ static pthread_mutex_t progress_mutex;
  * ahead in the list because they can be stolen and would need
  * progress_mutex for protection.
  */
-#else
-
-#define read_lock()		(void)0
-#define read_unlock()		(void)0
-#define cache_lock()		(void)0
-#define cache_unlock()		(void)0
-#define progress_lock()		(void)0
-#define progress_unlock()	(void)0
-
-#endif
 
 /*
  * Return the size of the object without doing any delta
@@ -2347,8 +2335,6 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 	free(array);
 }
 
-#ifndef NO_PTHREADS
-
 static void try_to_free_from_threads(size_t size)
 {
 	read_lock();
@@ -2578,10 +2564,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	free(p);
 }
 
-#else
-#define ll_find_deltas(l, s, w, d, p)	find_deltas(l, &s, w, d, p)
-#endif
-
 static void add_tag_chain(const struct object_id *oid)
 {
 	struct tag *tag;
@@ -2734,12 +2716,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 		if (delta_search_threads < 0)
 			die(_("invalid number of threads specified (%d)"),
 			    delta_search_threads);
-#ifdef NO_PTHREADS
-		if (delta_search_threads != 1) {
+		if (!HAVE_THREADS && delta_search_threads != 1) {
 			warning(_("no threads support, ignoring %s"), k);
 			delta_search_threads = 0;
 		}
-#endif
 		return 0;
 	}
 	if (!strcmp(k, "pack.indexversion")) {
@@ -3402,10 +3382,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!delta_search_threads)	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
 
-#ifdef NO_PTHREADS
-	if (delta_search_threads != 1)
+	if (!HAVE_THREADS && delta_search_threads != 1)
 		warning(_("no threads support, ignoring --threads"));
-#endif
 	if (!pack_to_stdout && !pack_size_limit)
 		pack_size_limit = pack_size_limit_cfg;
 	if (pack_to_stdout && pack_size_limit)
diff --git a/pack-objects.h b/pack-objects.h
index 2ca39cfcfe..3a42727c7d 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -145,9 +145,7 @@ struct packing_data {
 	struct packed_git **in_pack_by_idx;
 	struct packed_git **in_pack;
 
-#ifndef NO_PTHREADS
 	pthread_mutex_t lock;
-#endif
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -169,15 +167,11 @@ void prepare_packing_data(struct packing_data *pdata);
 
 static inline void packing_data_lock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_lock(&pdata->lock);
-#endif
 }
 static inline void packing_data_unlock(struct packing_data *pdata)
 {
-#ifndef NO_PTHREADS
 	pthread_mutex_unlock(&pdata->lock);
-#endif
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
-- 
2.19.1.1005.gac84295441

[PATCH v3 11/14] read-cache.c: remove #ifdef NO_PTHREADS

[Nguyễn Thái Ngọc Duy]

This is a faithful conversion with no attempt to clean up whatsoever.
Code indentation is left broken. There will be another commit to clean
it up and un-indent if we just indent now. It's just more code noise.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index d57958233e..40fc0cb65f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
 	struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
 	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
 	return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 
 	return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	size_t mmap_size;
 	struct load_index_extensions p;
 	size_t extension_offset = 0;
-#ifndef NO_PTHREADS
 	int nr_threads, cpus;
 	struct index_entry_offset_table *ieot = NULL;
-#endif
 
 	if (istate->initialized)
 		return istate->cache_nr;
@@ -2181,7 +2172,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-#ifndef NO_PTHREADS
+	if (HAVE_THREADS) {
 	nr_threads = git_config_get_index_threads();
 
 	/* TODO: does creating more threads than cores help? */
@@ -2219,21 +2210,19 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
 	}
-#else
-	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-#endif
+	} else {
+		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
+	}
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-#ifndef NO_PTHREADS
-	if (extension_offset) {
+	if (HAVE_THREADS && extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
 	}
-#endif
 	if (!extension_offset) {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
@@ -2756,8 +2745,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS) {
+		nr_threads = git_config_get_index_threads();
+
 	if (nr_threads != 1) {
 		int ieot_blocks, cpus;
 
@@ -2787,7 +2777,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
 		}
 	}
-#endif
+	}
 
 	offset = lseek(newfd, 0, SEEK_CUR);
 	if (offset < 0) {
@@ -2871,8 +2861,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-#ifndef NO_PTHREADS
-	if (ieot) {
+	if (HAVE_THREADS && ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
@@ -2883,7 +2872,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		if (err)
 			return -1;
 	}
-#endif
 
 	if (!strip_extensions && istate->split_index) {
 		struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3457,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
 	strbuf_add(sb, hash, the_hash_algo->rawsz);
 }
 
-#ifndef NO_PTHREADS
 #define IEOT_VERSION	(1)
 
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
@@ -3542,4 +3529,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
 	       strbuf_add(sb, &buffer, sizeof(uint32_t));
        }
 }
-#endif
-- 
2.19.1.1005.gac84295441

[PATCH v3 12/14] read-cache.c: reduce branching based on HAVE_THREADS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 40fc0cb65f..00cd416816 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2172,7 +2172,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-	if (HAVE_THREADS) {
 	nr_threads = git_config_get_index_threads();
 
 	/* TODO: does creating more threads than cores help? */
@@ -2183,6 +2182,9 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 			nr_threads = cpus;
 	}
 
+	if (!HAVE_THREADS)
+		nr_threads = 1;
+
 	if (nr_threads > 1) {
 		extension_offset = read_eoie_extension(mmap, mmap_size);
 		if (extension_offset) {
@@ -2210,20 +2212,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
 	}
-	} else {
-		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-	}
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-	if (HAVE_THREADS && extension_offset) {
+	if (extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
-	}
-	if (!extension_offset) {
+	} else {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
 	}
@@ -2745,8 +2743,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-	if (HAVE_THREADS) {
+	if (HAVE_THREADS)
 		nr_threads = git_config_get_index_threads();
+	else
+		nr_threads = 1;
 
 	if (nr_threads != 1) {
 		int ieot_blocks, cpus;
@@ -2777,7 +2777,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
 		}
 	}
-	}
 
 	offset = lseek(newfd, 0, SEEK_CUR);
 	if (offset < 0) {
@@ -2861,7 +2860,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-	if (HAVE_THREADS && ieot) {
+	if (ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
-- 
2.19.1.1005.gac84295441

[PATCH v3 13/14] read-cache.c: initialize copy_len to shut up gcc 8

[Nguyễn Thái Ngọc Duy]

It was reported that when building with NO_PTHREADS=1,
-Wmaybe-uninitialized is triggered. Just initialize the variable from
the beginning to shut the compiler up (because this warning is enabled
in config.dev)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 00cd416816..c510f598b1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1746,7 +1746,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 	size_t len;
 	const char *name;
 	unsigned int flags;
-	size_t copy_len;
+	size_t copy_len = 0;
 	/*
 	 * Adjacent cache entries tend to share the leading paths, so it makes
 	 * sense to only store the differences in later entries.  In the v4
@@ -1786,8 +1786,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 				die(_("malformed name field in the index, near path '%s'"),
 					previous_ce->name);
 			copy_len = previous_len - strip_len;
-		} else {
-			copy_len = 0;
 		}
 		name = (const char *)cp;
 	}
-- 
2.19.1.1005.gac84295441

[PATCH v3 14/14] Clean up pthread_create() error handling

[Nguyễn Thái Ngọc Duy]

Normally pthread_create() rarely fails. But with new pthreads wrapper,
pthread_create() will return ENOSYS on a system without thread support.

Threaded code _is_ protected by HAVE_THREADS and pthread_create()
should never run in the first place. But the situation could change in
the future and bugs may sneak in. Make sure that all pthread_create()
reports the error cause.

While at there, mark these strings for translation if they aren't.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c     | 16 ++++++++++------
 preload-index.c |  8 ++++++--
 run-command.c   |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index b3c9ac791d..623ca6923a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -494,6 +494,7 @@ static inline void lazy_update_dir_ref_counts(
 static void threaded_lazy_init_name_hash(
 	struct index_state *istate)
 {
+	int err;
 	int nr_each;
 	int k_start;
 	int t;
@@ -526,8 +527,9 @@ static void threaded_lazy_init_name_hash(
 		if (k_start > istate->cache_nr)
 			k_start = istate->cache_nr;
 		td_dir_t->k_end = k_start;
-		if (pthread_create(&td_dir_t->pthread, NULL, lazy_dir_thread_proc, td_dir_t))
-			die("unable to create lazy_dir_thread");
+		err = pthread_create(&td_dir_t->pthread, NULL, lazy_dir_thread_proc, td_dir_t);
+		if (err)
+			die(_("unable to create lazy_dir thread: %s"), strerror(err));
 	}
 	for (t = 0; t < lazy_nr_dir_threads; t++) {
 		struct lazy_dir_thread_data *td_dir_t = td_dir + t;
@@ -547,13 +549,15 @@ static void threaded_lazy_init_name_hash(
 	 */
 	td_name->istate = istate;
 	td_name->lazy_entries = lazy_entries;
-	if (pthread_create(&td_name->pthread, NULL, lazy_name_thread_proc, td_name))
-		die("unable to create lazy_name_thread");
+	err = pthread_create(&td_name->pthread, NULL, lazy_name_thread_proc, td_name);
+	if (err)
+		die(_("unable to create lazy_name thread: %s"), strerror(err));
 
 	lazy_update_dir_ref_counts(istate, lazy_entries);
 
-	if (pthread_join(td_name->pthread, NULL))
-		die("unable to join lazy_name_thread");
+	err = pthread_join(td_name->pthread, NULL);
+	if (err)
+		die(_("unable to join lazy_name thread: %s"), strerror(err));
 
 	cleanup_dir_mutex();
 
diff --git a/preload-index.c b/preload-index.c
index 0e24886aca..ddca1c216e 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -121,6 +121,8 @@ static void preload_index(struct index_state *index,
 
 	for (i = 0; i < threads; i++) {
 		struct thread_data *p = data+i;
+		int err;
+
 		p->index = index;
 		if (pathspec)
 			copy_pathspec(&p->pathspec, pathspec);
@@ -129,8 +131,10 @@ static void preload_index(struct index_state *index,
 		if (pd.progress)
 			p->progress = &pd;
 		offset += work;
-		if (pthread_create(&p->pthread, NULL, preload_thread, p))
-			die("unable to create threaded lstat");
+		err = pthread_create(&p->pthread, NULL, preload_thread, p);
+
+		if (err)
+			die(_("unable to create threaded lstat: %s"), strerror(err));
 	}
 	for (i = 0; i < threads; i++) {
 		struct thread_data *p = data+i;
diff --git a/run-command.c b/run-command.c
index 3c3b8814df..decf3239bd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1213,7 +1213,7 @@ int start_async(struct async *async)
 	{
 		int err = pthread_create(&async->tid, NULL, run_thread, async);
 		if (err) {
-			error_errno("cannot create thread");
+			error(_("cannot create async thread: %s"), strerror(err));
 			goto error;
 		}
 	}
-- 
2.19.1.1005.gac84295441

Re: [PATCH v3 00/14] Reduce #ifdef NO_PTHREADS

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Changes since v2
>
> - more cleanups in grep.c, read-cache.c and index-pack.c
> - the send-pack.c changes are back, but this time I just add
>   async_with_fork() to move NO_PTHREADS back in run-command.c

The patches all looked sensible; I'll wait for a few more days
to see if anybody else spots something.

Thanks.