Support git:// with old MinGW

PATCH In review Johannes Schindelin via GitGitGadget 5 participants 10 messages Apr 29, 2019 – May 3, 2019 Documentation

lore

Share & tools

lore

Johannes Schindelin via GitGitGadget

author

2019-04-29 22:04 UTC #0 lore raw

cover Support git:// with old MinGW

The MinGW project (which can now be considered "old", as mingw-w64 is the standard thing to use nowadays, since it also supports 64-bit builds) had some problems with our sideband feature, and in Git for Windows <2.x, we introduced a config setting to still allow using the git:// protocol by forcing off the sideband channel.

Let's contribute this patch to upstream Git, at long last.

Thomas Braun (1):
mingw: optionally disable side-band-64k for transport

 Documentation/config.txt          |  2 ++
 Documentation/config/sendpack.txt |  5 +++++
 send-pack.c                       | 14 +++++++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/sendpack.txt

base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-137%2Fdscho%2Fsideband-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-137/dscho/sideband-bug-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/137

Signature · 1 line

gitgitgadget

Thomas Braun via GitGitGadget

author

2019-04-29 22:04 UTC #1 lore raw

PATCH 1/1 mingw: optionally disable side-band-64k for transport

From: Thomas Braun <[email protected]>

Since commit 0c499ea60f (send-pack: demultiplex a sideband stream with status data, 2010-02-05) the built-in send-pack uses the side-band-64k capability if advertised by the server.

Unfortunately this breaks pushing over the dump git protocol if used over a network connection when using MinGW (but *not* when using mingw-w64).

The detailed reasons for this, are 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, and thus makes the dump git protocol work.

As this only affects builds against MinGW, the default is still to use side band.

[jes: split out the documentation into Documentation/config/, touched up the commit message.]

Signed-off-by Thomas Braun <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
 Documentation/config.txt          |  2 ++
 Documentation/config/sendpack.txt |  5 +++++
 send-pack.c                       | 14 +++++++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/sendpack.txt

Documentation/config.txt +2 -0

index d87846faa6..e9b2d10e99 100644
@@ -406,6 +406,8 @@ include::config/reset.txt[]
 
 include::config/sendemail.txt[]
 
+include::config/sendpack.txt[]
+
 include::config/sequencer.txt[]
 
 include::config/showbranch.txt[]

Documentation/config/sendpack.txt new file +5 -0

new file mode 100644
index 0000000000..e306f657fb
@@ -0,0 +1,5 @@
+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.

send-pack.c +13 -1

index 6dc16c3211..fa9e8cf1fc 100644
@@ -38,6 +38,16 @@ int option_parse_push_signed(const struct option *opt,
 	die("bad %s argument: %s", opt->long_name, arg);
 }
 
+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 void feed_object(const struct object_id *oid, FILE *fh, int negative)
 {
 	if (negative && !has_object_file(oid))

@@ -390,6 +400,8 @@ int send_pack(struct send_pack_args *args,
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 
+	git_config(send_pack_config, NULL);
+
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
 		status_report = 1;

@@ -397,7 +409,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;

Signature · 1 line

gitgitgadget

Eric Sunshine

2019-04-29 22:10 UTC #2 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

Quoted 5 lines · Thomas Braun via GitGitGadget

Since commit 0c499ea60f (send-pack: demultiplex a sideband stream with status data, 2010-02-05) the built-in send-pack uses the side-band-64k capability if advertised by the server.
Unfortunately this breaks pushing over the dump git protocol if used

s/dump/dumb/

Quoted 12 lines

over a network connection when using MinGW (but *not* when using mingw-w64).
[...]
The new config option "sendpack.sideband" allows to override the side-band-64k capability of the server, and thus makes the dump git protocol work.
[...]
Signed-off-by Thomas Braun <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---

Documentation/config/sendpack.txt line 5

Quoted 5 lines

Documentation/config/sendpack.txt +5 -0

@@ -0,0 +1,5 @@
+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.

s/dump/dumb/

For someone who hasn't read the commit message of this patch, "work around a limitation in ... git for windows" doesn't mean much. Perhaps this documentation could explain in more precise terms under what circumstances this option should be used?

Johannes Schindelin

2019-04-29 23:17 UTC #3 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

Hi Eric,

Quoted 9 lines · Eric Sunshine

Quoted 5 lines · Thomas Braun via GitGitGadget
Since commit 0c499ea60f (send-pack: demultiplex a sideband stream with status data, 2010-02-05) the built-in send-pack uses the side-band-64k capability if advertised by the server.
Unfortunately this breaks pushing over the dump git protocol if used
s/dump/dumb/

Of course!

Quoted 20 lines

Quoted 12 lines
over a network connection when using MinGW (but *not* when using mingw-w64).
[...]
The new config option "sendpack.sideband" allows to override the side-band-64k capability of the server, and thus makes the dump git protocol work.
[...]
Signed-off-by Thomas Braun <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---
Documentation/config/sendpack.txt line 5
Quoted 5 lines
Documentation/config/sendpack.txt +5 -0
@@ -0,0 +1,5 @@
+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.
s/dump/dumb/

Quoted 4 lines

For someone who hasn't read the commit message of this patch, "work around a limitation in ... git for windows" doesn't mean much. Perhaps this documentation could explain in more precise terms under what circumstances this option should be used?

You're right, this is confusing, especially since Git for Windows 2.x does not have that bug.

I simply dropped that sentence.

Thanks!
Dscho

brian m. carlson

2019-04-29 23:19 UTC #8 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

In reply to Thomas Braun via GitGitGadget

Quoted 13 lines · Thomas Braun via GitGitGadget

From: Thomas Braun <[email protected]>
Since commit 0c499ea60f (send-pack: demultiplex a sideband stream with status data, 2010-02-05) the built-in send-pack uses the side-band-64k capability if advertised by the server.
Unfortunately this breaks pushing over the dump git protocol if used over a network connection when using MinGW (but *not* when using mingw-w64).
The detailed reasons for this, are courtesy of Jeff Preshing, quoted from https://groups.google.com/d/msg/msysgit/at8D7J-h7mw/eaLujILGUWoJ:

Quoted 12 lines

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.

Since this is a platform-specific issue, can we address this using a compile-time constant instead of a config option? It would be better to do the right thing automatically in this case and not have to have people set a config option. It will also allow us to not to have to maintain a config option indefinitely if MinGW becomes more capable in the future.

Signature · 2 lines

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Johannes Sixt

2019-04-30 06:21 UTC #4 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

In reply to Johannes Schindelin

Am 30.04.19 um 01:17 schrieb Johannes Schindelin:

Quoted 12 lines · Eric Sunshine

Quoted 11 lines
On Mon, Apr 29, 2019 at 6:04 PM Thomas Braun via GitGitGadget
Documentation/config/sendpack.txt line 5
Quoted 7 lines
Documentation/config/sendpack.txt +5 -0
@@ -0,0 +1,5 @@
+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.
s/dump/dumb/

Quoted 7 lines

Quoted 4 lines
For someone who hasn't read the commit message of this patch, "work around a limitation in ... git for windows" doesn't mean much. Perhaps this documentation could explain in more precise terms under what circumstances this option should be used?
You're right, this is confusing, especially since Git for Windows 2.x does not have that bug.

If there is no bug, why do we need the patch?

-- Hannes

Johannes Schindelin

2019-04-30 22:34 UTC #5 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

Hi Hannes,

Quoted 18 lines · Johannes Sixt

Am 30.04.19 um 01:17 schrieb Johannes Schindelin:
Quoted 17 lines · Eric Sunshine
Quoted 15 lines
On Mon, Apr 29, 2019 at 6:04 PM Thomas Braun via GitGitGadget
Documentation/config/sendpack.txt line 5
Quoted 7 lines
Documentation/config/sendpack.txt +5 -0
@@ -0,0 +1,5 @@
+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.
s/dump/dumb/
For someone who hasn't read the commit message of this patch, "work around a limitation in ... git for windows" doesn't mean much. Perhaps this documentation could explain in more precise terms under what circumstances this option should be used?

Quoted 4 lines

Quoted 2 lines
You're right, this is confusing, especially since Git for Windows 2.x does not have that bug.
If there is no bug, why do we need the patch?

I thought you of all people (building with the ancient MSys/MINGW toolchains) would benefit from it :-)

But if even you don't want it, I'll gladly drop it from Git for Windows' patches and be done with it.

Ciao,
Dscho

Johannes Schindelin

2019-04-30 22:37 UTC #9 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

In reply to brian m. carlson

Hi brian,

Quoted 27 lines · brian m. carlson

Quoted 13 lines · Thomas Braun via GitGitGadget
From: Thomas Braun <[email protected]>
Since commit 0c499ea60f (send-pack: demultiplex a sideband stream with status data, 2010-02-05) the built-in send-pack uses the side-band-64k capability if advertised by the server.
Unfortunately this breaks pushing over the dump git protocol if used over a network connection when using MinGW (but *not* when using mingw-w64).
The detailed reasons for this, are courtesy of Jeff Preshing, quoted from https://groups.google.com/d/msg/msysgit/at8D7J-h7mw/eaLujILGUWoJ:
Quoted 12 lines
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.

Quoted 6 lines

Since this is a platform-specific issue, can we address this using a compile-time constant instead of a config option? It would be better to do the right thing automatically in this case and not have to have people set a config option. It will also allow us to not to have to maintain a config option indefinitely if MinGW becomes more capable in the future.

I was really not sure at the time whether this would be fixed in MinGW at some stage, and with the switch to mingw-w64 (by moving from MSys to MSYS2 as of Git for Windows 2.x) we do not really have any concrete need for it anymore.

I just thought that it might benefit somebody (it was my impression that Hannes Sixt still built his own copy with MSys/MinGW, and we do have a track record of maintaining certain things even for single users, see e.g. our insistence on creating the .git/branches/ directory upon git init).

But if the consensus is that we do not need this at all anymore, I'll be just as happy to drop that patch.

Ciao,
Dscho

Johannes Sixt

2019-04-30 22:55 UTC #6 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

In reply to Johannes Schindelin

Am 01.05.19 um 00:33 schrieb Johannes Schindelin:

Quoted 5 lines · Johannes Sixt

Quoted 4 lines
Am 30.04.19 um 01:17 schrieb Johannes Schindelin:
Quoted 2 lines
You're right, this is confusing, especially since Git for Windows 2.x does not have that bug.

Quoted 7 lines

Quoted 1 line
If there is no bug, why do we need the patch?
I thought you of all people (building with the ancient MSys/MINGW toolchains) would benefit from it :-)
But if even you don't want it, I'll gladly drop it from Git for Windows' patches and be done with it.

Ah, this is only for the ancient MinGW! I do indeed not mind if you drop it as I have updated my toolchain to the one you provide long ago.

Thank you for being considerate!

-- Hannes

Johannes Schindelin

2019-05-03 08:42 UTC #7 lore raw

Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport

Hi Hannes,

Quoted 8 lines · Johannes Sixt

Am 01.05.19 um 00:33 schrieb Johannes Schindelin:
Quoted 7 lines · Johannes Sixt
Quoted 5 lines
Am 30.04.19 um 01:17 schrieb Johannes Schindelin:
Quoted 2 lines
You're right, this is confusing, especially since Git for Windows 2.x does not have that bug.
If there is no bug, why do we need the patch?

Quoted 8 lines

Quoted 5 lines
I thought you of all people (building with the ancient MSys/MINGW toolchains) would benefit from it :-)
But if even you don't want it, I'll gladly drop it from Git for Windows' patches and be done with it.
Ah, this is only for the ancient MinGW! I do indeed not mind if you drop it as I have updated my toolchain to the one you provide long ago.

Okay then, let's drop this!

Ciao,
Dscho