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

[Dan McGee <dpmcgee@gmail.com>]

On Sat, Jan 30, 2010 at 7:29 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Sat, 30 Jan 2010, Junio C Hamano wrote:
>
>> Dan McGee <dpmcgee@gmail.com> writes:
>>
>> > 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.
>
>> 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