git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/1] Support git:// with old MinGW
@ 2019-04-29 22:04 Johannes Schindelin via GitGitGadget
  2019-04-29 22:04 ` [PATCH 1/1] mingw: optionally disable side-band-64k for transport Thomas Braun via GitGitGadget
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-04-29 22:04 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-29 22:04 [PATCH 0/1] Support git:// with old MinGW Johannes Schindelin via GitGitGadget
@ 2019-04-29 22:04 ` Thomas Braun via GitGitGadget
  2019-04-29 22:10   ` Eric Sunshine
  2019-04-29 23:19   ` brian m. carlson
  0 siblings, 2 replies; 10+ messages in thread
From: Thomas Braun via GitGitGadget @ 2019-04-29 22:04 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Thomas Braun

From: Thomas Braun <thomas.braun@byte-physics.de>

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 <thomas.braun@byte-physics.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d87846faa6..e9b2d10e99 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -406,6 +406,8 @@ include::config/reset.txt[]
 
 include::config/sendemail.txt[]
 
+include::config/sendpack.txt[]
+
 include::config/sequencer.txt[]
 
 include::config/showbranch.txt[]
diff --git a/Documentation/config/sendpack.txt b/Documentation/config/sendpack.txt
new file mode 100644
index 0000000000..e306f657fb
--- /dev/null
+++ b/Documentation/config/sendpack.txt
@@ -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.
diff --git a/send-pack.c b/send-pack.c
index 6dc16c3211..fa9e8cf1fc 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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;
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-29 22:04 ` [PATCH 1/1] mingw: optionally disable side-band-64k for transport Thomas Braun via GitGitGadget
@ 2019-04-29 22:10   ` Eric Sunshine
  2019-04-29 23:17     ` Johannes Schindelin
  2019-04-29 23:19   ` brian m. carlson
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Sunshine @ 2019-04-29 22:10 UTC (permalink / raw)
  To: Thomas Braun via GitGitGadget; +Cc: Git List, Junio C Hamano, Thomas Braun

On Mon, Apr 29, 2019 at 6:04 PM Thomas Braun via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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/

> 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 <thomas.braun@byte-physics.de>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/Documentation/config/sendpack.txt b/Documentation/config/sendpack.txt
> @@ -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?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-29 22:10   ` Eric Sunshine
@ 2019-04-29 23:17     ` Johannes Schindelin
  2019-04-30  6:21       ` Johannes Sixt
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Schindelin @ 2019-04-29 23:17 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Thomas Braun via GitGitGadget, Git List, Junio C Hamano, Thomas Braun

Hi Eric,

On Mon, 29 Apr 2019, Eric Sunshine wrote:

> On Mon, Apr 29, 2019 at 6:04 PM Thomas Braun via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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!

> > 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 <thomas.braun@byte-physics.de>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/Documentation/config/sendpack.txt b/Documentation/config/sendpack.txt
> > @@ -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?

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-29 22:04 ` [PATCH 1/1] mingw: optionally disable side-band-64k for transport Thomas Braun via GitGitGadget
  2019-04-29 22:10   ` Eric Sunshine
@ 2019-04-29 23:19   ` brian m. carlson
  2019-04-30 22:37     ` Johannes Schindelin
  1 sibling, 1 reply; 10+ messages in thread
From: brian m. carlson @ 2019-04-29 23:19 UTC (permalink / raw)
  To: Thomas Braun via GitGitGadget; +Cc: git, Junio C Hamano, Thomas Braun

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

On Mon, Apr 29, 2019 at 03:04:36PM -0700, Thomas Braun via GitGitGadget wrote:
> From: Thomas Braun <thomas.braun@byte-physics.de>
> 
> 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.

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-29 23:17     ` Johannes Schindelin
@ 2019-04-30  6:21       ` Johannes Sixt
  2019-04-30 22:33         ` Johannes Schindelin
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2019-04-30  6:21 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Eric Sunshine, Thomas Braun via GitGitGadget, Git List,
	Junio C Hamano, Thomas Braun

Am 30.04.19 um 01:17 schrieb Johannes Schindelin:
> On Mon, 29 Apr 2019, Eric Sunshine wrote:
>> On Mon, Apr 29, 2019 at 6:04 PM Thomas Braun via GitGitGadget
>>> diff --git a/Documentation/config/sendpack.txt b/Documentation/config/sendpack.txt
>>> @@ -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?
> 
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-30  6:21       ` Johannes Sixt
@ 2019-04-30 22:33         ` Johannes Schindelin
  2019-04-30 22:55           ` Johannes Sixt
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Schindelin @ 2019-04-30 22:33 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Eric Sunshine, Thomas Braun via GitGitGadget, Git List,
	Junio C Hamano, Thomas Braun

Hi Hannes,

On Tue, 30 Apr 2019, Johannes Sixt wrote:

> Am 30.04.19 um 01:17 schrieb Johannes Schindelin:
> > On Mon, 29 Apr 2019, Eric Sunshine wrote:
> >> On Mon, Apr 29, 2019 at 6:04 PM Thomas Braun via GitGitGadget
> >>> diff --git a/Documentation/config/sendpack.txt b/Documentation/config/sendpack.txt
> >>> @@ -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?
> >
> > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-29 23:19   ` brian m. carlson
@ 2019-04-30 22:37     ` Johannes Schindelin
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Schindelin @ 2019-04-30 22:37 UTC (permalink / raw)
  To: brian m. carlson
  Cc: Thomas Braun via GitGitGadget, git, Junio C Hamano, Thomas Braun

Hi brian,

On Mon, 29 Apr 2019, brian m. carlson wrote:

> On Mon, Apr 29, 2019 at 03:04:36PM -0700, Thomas Braun via GitGitGadget wrote:
> > From: Thomas Braun <thomas.braun@byte-physics.de>
> >
> > 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.
>
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-30 22:33         ` Johannes Schindelin
@ 2019-04-30 22:55           ` Johannes Sixt
  2019-05-03  8:42             ` Johannes Schindelin
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2019-04-30 22:55 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Eric Sunshine, Thomas Braun via GitGitGadget, Git List,
	Junio C Hamano, Thomas Braun

Am 01.05.19 um 00:33 schrieb Johannes Schindelin:
> On Tue, 30 Apr 2019, Johannes Sixt wrote:
>> Am 30.04.19 um 01:17 schrieb Johannes Schindelin:
>>> 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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] mingw: optionally disable side-band-64k for transport
  2019-04-30 22:55           ` Johannes Sixt
@ 2019-05-03  8:42             ` Johannes Schindelin
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Schindelin @ 2019-05-03  8:42 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Eric Sunshine, Thomas Braun via GitGitGadget, Git List,
	Junio C Hamano, Thomas Braun

Hi Hannes,

On Wed, 1 May 2019, Johannes Sixt wrote:

> Am 01.05.19 um 00:33 schrieb Johannes Schindelin:
> > On Tue, 30 Apr 2019, Johannes Sixt wrote:
> >> Am 30.04.19 um 01:17 schrieb Johannes Schindelin:
> >>> 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.
>
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-05-03  8:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-29 22:04 [PATCH 0/1] Support git:// with old MinGW Johannes Schindelin via GitGitGadget
2019-04-29 22:04 ` [PATCH 1/1] mingw: optionally disable side-band-64k for transport Thomas Braun via GitGitGadget
2019-04-29 22:10   ` Eric Sunshine
2019-04-29 23:17     ` Johannes Schindelin
2019-04-30  6:21       ` Johannes Sixt
2019-04-30 22:33         ` Johannes Schindelin
2019-04-30 22:55           ` Johannes Sixt
2019-05-03  8:42             ` Johannes Schindelin
2019-04-29 23:19   ` brian m. carlson
2019-04-30 22:37     ` Johannes Schindelin

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).