Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2012, 22:37 UTC
Message-ID: <7v62adaee3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CABPQNSZ7=kqSjXPLior+LXqAt4AzcybOjJ1P8ZLQ+J-9gCgvdw@mail.gmail.com>

Erik Faye-Lund <kusmabite@gmail.com> writes:

Show 16 quoted lines

> On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>>
>>>    - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD
>>
>> Sensible.
>>
>>>    - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather
>>>      than in git-compat-util.h
>>
>> I think it is a bad change.  When compat/ pread gets improved to be
>> thread-safe, this will surely be missed.
>
> But CAN it be fixed? I don't think it could, at least not without
> wrapping ALL calls to functions that perform IO on file handles or
> file descriptors...

Is that relevant? It may be true that both Erik and Junio are not being clever enough to come up with a solution offhand. But is that a good justification to go against a sound engineering practice?

There may be more than one platforms that want their own different pread emulations, some safe with thread and some others not. I think you would prefer to see something like this:

	#if NO_PREAD
        #if on platform A
	# define NO_THREAD_SAFE_PREAD
        # define pread(fd, buf, count, ofs) git_pread_A((fd), (buf), (count), (ofs))
	#endif
        #if on platform B
        # define pread(fd, buf, count, ofs) git_pread_B((fd), (buf), (count), (ofs))
	#endif

and keep "make -DNO_PREAD" to mean "The platform does not have a pread, please emulate" and nothing else. The implementation of the emulation itself would be the one that knows of its thread safeness.

Having said that, I do not care too deeply either way, so the patch is queued as-is.