Re: [PATCH] Make NO_PTHREADS the sole thread configuration variable

On Sat, Jan 30, 2010 at 7:29 PM, Nicolas Pitre [off-list ref] wrote:

On Sat, 30 Jan 2010, Junio C Hamano wrote:

quoted

Dan McGee [off-list ref] writes:

quoted

When the first piece of threaded code was introduced in commit 8ecce684, it
came with its own THREADED_DELTA_SEARCH Makefile option. Since this time,
more threaded code has come into the codebase and a NO_PTHREADS option has
also been added. Get rid of the original option as the newer, more generic
option covers everything we need.

The patch is a good change but only in the "if it were like this from day
one, things would have been much simpler" sense.  It is a breakage to
existing users with customized THREADED_DELTA_SEARCH in their config.mak
files, isn't it?

I think that the release of v1.7.0 is the perfect match for such a
"breakage".  Unlike for the dashless move, I really doubt the majority
of Git users are using a customized THREADED_DELTA_SEARCH in a
config.mak if they do build Git themselves at all.  So very few people
are likely to be inconvenienced, and yet the inconvenience can hardly be
qualified as a breakage since nothing will stop working in any case.

quoted

If we take only the part of your patch that applies to Makefile, but
exclude the first hunk (description of THREADED_DELTA_SEARCH) and the last
hunk (the necessary part to keep THREADED_DELTA_SEARCH working), and
instead add something like:

      ifndef NO_PTHREADS
              THREADED_DELTA_SEARCH = YesPlease
      endif

immediately before we include config.mak, would that be a workable
solution to:

 (1) keep existing users happy;

 (2) remove the redundant logic to compute the default for two Make
     variables; and

 (3) keep control over use of threading in general _and_ use of
     threading in delta computation?

IMHO I wouldn't bother that much.  Simply mentioning in the 1.7.0
release notes that THREADED_DELTA_SEARCH is no more should be fine.
Like I said, the existing users who might be affected are certainly few,
and the impact on them is rather trivial.

This is everything I wanted to say as well. I just don't think this is
that big of a deal to "break compatibility" as I can think of no
reason why you would only want 2/3 of the pthreads code enabled (your
point 3). What this does do is set the precedent for any future
threads code to only use NO_PTHREADS and not introduce yet another
preprocessor define.

If this gets applied, it needs a small correction- I can resubmit, but
the only difference is this:

diff --git a/config.mak.in b/config.mak.in
index 67b12f7..6008ac9 100644
--- a/config.mak.in
+++ b/config.mak.in

@@ -56,5 +56,4 @@ NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
 FREAD_READS_DIRECTORIES=@FREAD_READS_DIRECTORIES@
 SNPRINTF_RETURNS_BOGUS=@SNPRINTF_RETURNS_BOGUS@
 NO_PTHREADS=@NO_PTHREADS@
-THREADED_DELTA_SEARCH=@THREADED_DELTA_SEARCH@
 PTHREAD_LIBS=@PTHREAD_LIBS@


-Dan