RFC patchsend-pack.c: Allow to disable side-band-64k

> Hi,
>
> Thomas Braun wrote:
>
>> pushing over the dump git protocol with a windows git client.
>
> I've never heard of the dump git protocol.  Do you mean the git
> protocol that's used with git:// URLs?
>
> [...]
>> Alternative approaches considered but deemed too invasive:
>> - Rewrite read/write wrappers in mingw.c in order to distinguish between
>>   a file descriptor which has a socket behind and a file descriptor
>>   which has a file behind.
>
> I assume here "too invasive" means "too much engineering effort"?
>
> It sounds like a clean fix, not too invasive at all.  But I can
> understand wanting a stopgap in the meantime.
>

> On Mon, May 19, 2014 at 9:33 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Hi,
>>
>> Thomas Braun wrote:
>>
>>> pushing over the dump git protocol with a windows git client.
>>
>> I've never heard of the dump git protocol.  Do you mean the git
>> protocol that's used with git:// URLs?
>>
>> [...]
>>> Alternative approaches considered but deemed too invasive:
>>> - Rewrite read/write wrappers in mingw.c in order to distinguish between
>>>   a file descriptor which has a socket behind and a file descriptor
>>>   which has a file behind.
>>
>> I assume here "too invasive" means "too much engineering effort"?
>>
>> It sounds like a clean fix, not too invasive at all.  But I can
>> understand wanting a stopgap in the meantime.
>>
>
> Yeah, now that the problem seems to be understood, I don't think that
> would be too bad. I recently killed off our previous write()-wrapper
> in c9df6f4, but I see no reason why we can't add a new one.
>
> Would we need to wrap both ends, shouldn't wrapping only reading be
> good enough to prevent deadlocking?
>
> compat/poll/poll.c already contains a function called IsSocketHandle
> that is able to tell if a HANDLE points to a socket or not.

>>  [...]
>> Would we need to wrap both ends, shouldn't wrapping only reading be
>> good enough to prevent deadlocking?
>>
>> compat/poll/poll.c already contains a function called IsSocketHandle
>> that is able to tell if a HANDLE points to a socket or not.
>
> This very quick attempt did not work out :(
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 0335958..ec1d81f 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -370,6 +370,65 @@ int mingw_open (const char *filename, int oflags, ...)
>       return fd;
>   }
>
> +#define is_console_handle(h) (((long) (h) & 3) == 3)
> +
> +static int is_socket_handle(HANDLE h)
> +{
> +    WSANETWORKEVENTS ev;
> +
> +    if (is_console_handle(h))
> +        return 0;
> +
> +    /*
> +     * Under Wine, it seems that getsockopt returns 0 for pipes too.
> +     * WSAEnumNetworkEvents instead distinguishes the two correctly.
> +     */
> +    ev.lNetworkEvents = 0xDEADBEEF;
> +    WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
> +    return ev.lNetworkEvents != 0xDEADBEEF;
> +}
> +
> +#undef read
> +ssize_t mingw_read(int fd, void *buf, size_t count)
> +{
> +    int ret;
> +    HANDLE fh = (HANDLE)_get_osfhandle(fd);
> +
> +    if (fh == INVALID_HANDLE_VALUE) {
> +        errno = EBADF;
> +        return -1;
> +    }
> +
> +    if (!is_socket_handle(fh))
> +        return read(fd, buf, count);
> +
> +    ret = recv((SOCKET)fh, buf, count, 0);
> +    if (ret < 0)
> +        errno = WSAGetLastError();
> +    return ret;
> +}
> +
> +#undef write
> +ssize_t mingw_write(int fd, const void *buf, size_t count)
> +{
> +    int ret;
> +    HANDLE fh = (HANDLE)_get_osfhandle(fd);
> +
> +    if (fh == INVALID_HANDLE_VALUE) {
> +        errno = EBADF;
> +        return -1;
> +    }
> +
> +    if (!is_socket_handle(fh))
> +        return write(fd, buf, count);
> +
> +    return send((SOCKET)fh, buf, count, 0);
> +    if (ret < 0)
> +        errno = WSAGetLastError();
> +    return ret;
> +}
> +
> +
>   static BOOL WINAPI ctrl_ignore(DWORD type)
>   {
>       return TRUE;
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 08b83fe..1690098 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);
>   int mingw_open (const char *filename, int oflags, ...);
>   #define open mingw_open
>
> +ssize_t mingw_read(int fd, void *buf, size_t count);
> +#define read mingw_read
> +
> +ssize_t mingw_write(int fd, const void *buf, size_t count);
> +#define write mingw_write
> +
>   int mingw_fgetc(FILE *stream);
>   #define fgetc mingw_fgetc

> Am 19.05.2014 22:29, schrieb Erik Faye-Lund:
>>>  [...]
>>> Would we need to wrap both ends, shouldn't wrapping only reading be
>>> good enough to prevent deadlocking?
>>>
>>> compat/poll/poll.c already contains a function called IsSocketHandle
>>> that is able to tell if a HANDLE points to a socket or not.
>>
>> This very quick attempt did not work out :(
>>
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 0335958..ec1d81f 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -370,6 +370,65 @@ int mingw_open (const char *filename, int oflags, ...)
>>       return fd;
>>   }
>>
>> +#define is_console_handle(h) (((long) (h) & 3) == 3)
>> +
>> +static int is_socket_handle(HANDLE h)
>> +{
>> +    WSANETWORKEVENTS ev;
>> +
>> +    if (is_console_handle(h))
>> +        return 0;
>> +
>> +    /*
>> +     * Under Wine, it seems that getsockopt returns 0 for pipes too.
>> +     * WSAEnumNetworkEvents instead distinguishes the two correctly.
>> +     */
>> +    ev.lNetworkEvents = 0xDEADBEEF;
>> +    WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
>> +    return ev.lNetworkEvents != 0xDEADBEEF;
>> +}
>> +
>> +#undef read
>> +ssize_t mingw_read(int fd, void *buf, size_t count)
>> +{
>> +    int ret;
>> +    HANDLE fh = (HANDLE)_get_osfhandle(fd);
>> +
>> +    if (fh == INVALID_HANDLE_VALUE) {
>> +        errno = EBADF;
>> +        return -1;
>> +    }
>> +
>> +    if (!is_socket_handle(fh))
>> +        return read(fd, buf, count);
>> +
>> +    ret = recv((SOCKET)fh, buf, count, 0);
>> +    if (ret < 0)
>> +        errno = WSAGetLastError();
>> +    return ret;
>> +}
>> +
>> +#undef write
>> +ssize_t mingw_write(int fd, const void *buf, size_t count)
>> +{
>> +    int ret;
>> +    HANDLE fh = (HANDLE)_get_osfhandle(fd);
>> +
>> +    if (fh == INVALID_HANDLE_VALUE) {
>> +        errno = EBADF;
>> +        return -1;
>> +    }
>> +
>> +    if (!is_socket_handle(fh))
>> +        return write(fd, buf, count);
>> +
>> +    return send((SOCKET)fh, buf, count, 0);
>> +    if (ret < 0)
>> +        errno = WSAGetLastError();
>> +    return ret;
>> +}
>> +
>> +
>>   static BOOL WINAPI ctrl_ignore(DWORD type)
>>   {
>>       return TRUE;
>> diff --git a/compat/mingw.h b/compat/mingw.h
>> index 08b83fe..1690098 100644
>> --- a/compat/mingw.h
>> +++ b/compat/mingw.h
>> @@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);
>>   int mingw_open (const char *filename, int oflags, ...);
>>   #define open mingw_open
>>
>> +ssize_t mingw_read(int fd, void *buf, size_t count);
>> +#define read mingw_read
>> +
>> +ssize_t mingw_write(int fd, const void *buf, size_t count);
>> +#define write mingw_write
>> +
>>   int mingw_fgetc(FILE *stream);
>>   #define fgetc mingw_fgetc
>
> According to [1] you also have to pass WSA_FLAG_OVERLAPPED to avoid the deadlock.
>
> With that change I don't get a hang anymore but a read error with
> errno 10054 aka WSAECONNRESET.
>
> [1]: https://groups.google.com/forum/#!msg/msysgit/at8D7J-h7mw/PM9w-d41cDYJ
>

> Hi,
>
> Thomas Braun wrote:
>
>> pushing over the dump git protocol with a windows git client.
>
> I've never heard of the dump git protocol.  Do you mean the git
> protocol that's used with git:// URLs?

> [...]
>> Alternative approaches considered but deemed too invasive:
>> - Rewrite read/write wrappers in mingw.c in order to distinguish between
>>    a file descriptor which has a socket behind and a file descriptor
>>    which has a file behind.
>
> I assume here "too invasive" means "too much engineering effort"?
>
> It sounds like a clean fix, not too invasive at all.  But I can
> understand wanting a stopgap in the meantime.

>
>> - Turning the capability side-band-64k off completely. This would remove a useful
>>    feature for users of non-affected transport protocols.
>
> Would it make sense to turn off sideband unconditionally on Windows
> when using the relevant protocols?
>

> Am 19.05.2014 21:33, schrieb Jonathan Nieder:
>
>> Hi,
>>
>> Thomas Braun wrote:
>>
>>> pushing over the dump git protocol with a windows git client.
>>
>>
>> I've never heard of the dump git protocol.  Do you mean the git
>> protocol that's used with git:// URLs?
>
>
> You are right I mean the protocol involving git:// URLs. But unfortunately I
> got it wrong as according to [1] the git:// is one of the so-called smart
> protocols. That was also the source where I read that there are smart and
> dump protocols.
>
> [1]: http://git-scm.com/book/en/Git-Internals-Transfer-Protocols
>
>
>> [...]
>>>
>>> Alternative approaches considered but deemed too invasive:
>>> - Rewrite read/write wrappers in mingw.c in order to distinguish between
>>>    a file descriptor which has a socket behind and a file descriptor
>>>    which has a file behind.
>>
>>
>> I assume here "too invasive" means "too much engineering effort"?
>>
>> It sounds like a clean fix, not too invasive at all.  But I can
>> understand wanting a stopgap in the meantime.
>
>
> No actually I meant too invasive in the sense of "requiring large rewrites
> which only benefit git on windows and hurt all others".
>
> The two fixes I can think of either involve:
> - In a read *and* write wrapper the need to check if the fd is a socket, if
> yes use send/recv if no use read/write. According to Erik's comments this
> should be possible. But I would deem the expected performance penalty quite
> large as that will be done in every call.