From: Thomas Braun <hidden> Date: 2016-06-15 23:01:15
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 <redacted>
---
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(-)
@@ -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.
@@ -209,6 +219,8 @@ int send_pack(struct send_pack_args *args,intret;structasyncdemux;+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.
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:15
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.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 23:01:15
On Mon, May 19, 2014 at 9:33 PM, Jonathan Nieder [off-list ref] wrote:
Hi,
Thomas Braun wrote:
quoted
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?
[...]
quoted
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.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 23:01:15
On Mon, May 19, 2014 at 10:00 PM, Erik Faye-Lund [off-list ref] wrote:
On Mon, May 19, 2014 at 9:33 PM, Jonathan Nieder [off-list ref] wrote:
quoted
Hi,
Thomas Braun wrote:
quoted
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?
[...]
quoted
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.
@@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);intmingw_open(constchar*filename,intoflags,...);#define open mingw_open+ssize_tmingw_read(intfd,void*buf,size_tcount);+#define read mingw_read++ssize_tmingw_write(intfd,constvoid*buf,size_tcount);+#define write mingw_write+intmingw_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.
From: Thomas Braun <hidden> Date: 2016-06-15 23:01:15
Am 19.05.2014 21:33, schrieb Jonathan Nieder:
Hi,
Thomas Braun wrote:
quoted
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
[...]
quoted
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.
quoted
- 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.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 23:01:15
On Mon, May 19, 2014 at 11:15 PM, Thomas Braun
[off-list ref] wrote:
Am 19.05.2014 21:33, schrieb Jonathan Nieder:
quoted
Hi,
Thomas Braun wrote:
quoted
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
quoted
[...]
quoted
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.
From: Thomas Braun <hidden> Date: 2016-06-15 23:01:15
Am 19.05.2014 22:29, schrieb Erik Faye-Lund:
quoted hunk
quoted
[...]
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.
@@ -1542,7 +1607,7 @@ int mingw_socket(int domain, int type, int protocol)SOCKETs;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()valuesareregularBSDerrorcodes
@@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);intmingw_open(constchar*filename,intoflags,...);#define open mingw_open+ssize_tmingw_read(intfd,void*buf,size_tcount);+#define read mingw_read++ssize_tmingw_write(intfd,constvoid*buf,size_tcount);+#define write mingw_write+intmingw_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.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 23:01:15
On Tue, May 20, 2014 at 10:46 AM, Thomas Braun
[off-list ref] wrote:
Am 19.05.2014 22:29, schrieb Erik Faye-Lund:
quoted
quoted
[...]
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.
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.