From: Junio C Hamano Date: Tue, 26 Jun 2012 22:37:24 GMT Subject: Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin Message-ID: <7v62adaee3.fsf@alter.siamese.dyndns.org> In-Reply-To: Erik Faye-Lund writes: > On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano wrote: >> Ramsay Jones 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.