[PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[Thomas Braun]

Since commit 0c499ea60f the send-pack builtin uses the side-band-64k
capability if advertised by the server. Unfortunately this breaks
pushing over the dump git protocol with a windows git client.

The detailed reasons for this breakage are (by courtesy of Jeff
Preshing, quoted from
https://groups.google.com/d/msg/msysgit/at8D7J-h7mw/eaLujILGUWoJ):
----------------------------------------------------------------------------
MinGW wraps Windows sockets in CRT file descriptors in order to mimic
the functionality of POSIX sockets. This causes msvcrt.dll to treat
sockets as Installable File System (IFS) handles, calling ReadFile,
WriteFile, DuplicateHandle and CloseHandle on them. This approach works
well in simple cases on recent versions of Windows, but does not support
all usage patterns.  In particular, using this approach, any attempt to
read & write concurrently on the same socket (from one or more
processes) will deadlock in a scenario where the read waits for a
response from the server which is only invoked after the write. This is
what send_pack currently attempts to do in the use_sideband codepath.
----------------------------------------------------------------------------

The new config option "sendpack.sideband" allows to override the
side-band-64k capability of the server.

Other transportation methods like ssh and http/https still benefit from
the sideband channel, therefore the default value of "sendpack.sideband"
is still true.

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.
- Turning the capability side-band-64k off completely. This would remove a useful
  feature for users of non-affected transport protocols.

Signed-off-by: Thomas Braun <thomas.braun@byte-physics.de>
---

This patch, with a slightly less polished commit message, is already part of
msysgit/git see b68e386. 

A lengthy discussion can be found here [1].

What do you think, is this also for you as upstream interesting?

[1]: https://github.com/msysgit/git/issues/101

 Documentation/config.txt |  6 ++++++
 send-pack.c              | 14 +++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..13ff657 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2435,3 +2435,9 @@ web.browser::
 	Specify a web browser that may be used by some commands.
 	Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
 	may use it.
+
+sendpack.sideband::
+  Allows to disable the side-band-64k capability for send-pack even
+  when it is advertised by the server. Makes it possible to work
+  around a limitation in the git for windows implementation together
+  with the dump git protocol. Defaults to true.
diff --git a/send-pack.c b/send-pack.c
index 6129b0f..aace1fc 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -12,6 +12,16 @@
 #include "version.h"
 #include "sha1-array.h"
 
+static int config_use_sideband = 1;
+
+static int send_pack_config(const char *var, const char *value, void *unused)
+{
+	if (!strcmp("sendpack.sideband", var))
+		config_use_sideband = git_config_bool(var, value);
+
+	return 0;
+}
+
 static int feed_object(const unsigned char *sha1, int fd, int negative)
 {
 	char buf[42];
@@ -209,6 +219,8 @@ int send_pack(struct send_pack_args *args,
 	int ret;
 	struct async demux;
 
+	git_config(send_pack_config, NULL);
+
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
 		status_report = 1;
@@ -216,7 +228,7 @@ int send_pack(struct send_pack_args *args,
 		allow_deleting_refs = 1;
 	if (server_supports("ofs-delta"))
 		args->use_ofs_delta = 1;
-	if (server_supports("side-band-64k"))
+	if (config_use_sideband && server_supports("side-band-64k"))
 		use_sideband = 1;
 	if (server_supports("quiet"))
 		quiet_supported = 1;
-- 
1.9.1

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[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?

[...]
> 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?

I assume someone on the list wouldn't mind writing such a patch, so I
don't think the engineering effort would be a problem for that.

Thanks,
Jonathan

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[Erik Faye-Lund]

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.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[Erik Faye-Lund]

On Mon, May 19, 2014 at 10:00 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> 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.

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

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[Thomas Braun]

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

diff --git a/compat/mingw.c b/compat/mingw.c
index 383cafe..4a58135 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -366,6 +366,71 @@ 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();
+        warning("errno %d",errno);
+    }
+    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);
+
+    ret = send((SOCKET)fh, buf, count, 0);
+    if (ret < 0)
+    {
+        errno = WSAGetLastError();
+        warning("errno %d",errno);
+    }
+    return ret;
+}
+
+
 static BOOL WINAPI ctrl_ignore(DWORD type)
 {
 	return TRUE;
@@ -1542,7 +1607,7 @@ int mingw_socket(int domain, int type, int protocol)
 	SOCKET s;
 
 	ensure_socket_initialization();
-	s = WSASocket(domain, type, protocol, NULL, 0, 0);
+	s = WSASocket(domain, type, protocol, NULL, 0, WSA_FLAG_OVERLAPPED);
 	if (s == INVALID_SOCKET) {
 		/*
 		 * WSAGetLastError() values are regular BSD error codes
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
 




-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[Erik Faye-Lund]

On Tue, May 20, 2014 at 10:46 AM, Thomas Braun
<thomas.braun@byte-physics.de> wrote:
> 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
>

Yeah, sorry, I noticed this right after sending and tested with that
as well. My results were the same :/

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[Thomas Braun]

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.
- Rewriting read/write to accept windows handles instead of file 
descriptors. Only a theoretical option IMHO.

For me the goal is also to minimise the diff between git and msysgit/git.

>
>> - 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?
>

Yes, if this would be also acceptable for git.git.

I can check at the call site of send_pack in transport.c what protocol 
is in use, and then pass a new parameter use_sideband to it.
Or maybe "adapt" server_capabilities in connect.c to not include 
side-band-64k if using git:// ?


-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k

[Erik Faye-Lund]

On Mon, May 19, 2014 at 11:15 PM, Thomas Braun
<thomas.braun@byte-physics.de> wrote:
> 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.

You clearly haven't stepped through MSVCRT's read and write implementations :P

I wouldn't worry too much about this, at least not until the numbers are in.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.