Re: [PATCH] Enable index-pack threading in msysgit.

From: Karsten Blees <karsten.blees@gmail.com>
Date: Mar 20, 2014, 13:54 UTC
Message-ID: <532AF304.7040301@gmail.com>
In-Reply-To: <5328e903.joAd1dfenJmScBNr%szager@chromium.org>

Am 19.03.2014 01:46, schrieb szager@chromium.org:

> This adds a Windows implementation of pread.  Note that it is NOT
> safe to intersperse calls to read() and pread() on a file
> descriptor.

This is a bad idea. You're basically fixing the multi-threaded issue twice, while at the same time breaking single-threaded read/pread interop on the mingw and msvc platform. Users of pread already have to take care that its not thread-safe on some platforms, now you're adding another breakage that has to be considered in future development.

The mingw_pread implementation in [1] is both thread-safe and allows mixing read/pread in single-threaded scenarios, why not use this instead?

[1] http://article.gmane.org/gmane.comp.version-control.git/242120

> 
> http://article.gmane.org/gmane.comp.version-control.git/196042
>

Duy's patch alone enables multi-threaded index-pack on all platforms (including cygwin), so IMO this should be a separate patch.

> +	if (hand == INVALID_HANDLE_VALUE) {
> +		errno = EBADF;
> +		return -1;
> +	}

This check is redundant, ReadFile already ckecks for invalid handles and err_win_to_posix converts to EBADF.

Show 13 quoted lines

> +
> +	LARGE_INTEGER offset_value;
> +	offset_value.QuadPart = offset;
> +
> +	DWORD bytes_read = 0;
> +	OVERLAPPED overlapped = {0};
> +	overlapped.Offset = offset_value.LowPart;
> +	overlapped.OffsetHigh = offset_value.HighPart;
> +	BOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);
> +
> +	ssize_t ret = bytes_read;
> +
> +	if (!result && GetLastError() != ERROR_HANDLE_EOF)

According to MSDN docs, ReadFile never fails with ERROR_HANDLE_EOF, or is this another case where the documentation is wrong?

"When a synchronous read operation reaches the end of a file, ReadFile returns TRUE and sets *lpNumberOfBytesRead to zero."

Karsten