Config option to disable side-band-64k for transport

[assarbad]

Hi @dscho

this reintroduces the original changeset from Thomas Braun. Hope that everything is well-formed and as expected.

I also opened a separate ticket #2376 for the underlying issue (as outlined a bit in issue #907). For now reintroducing the option should "unbreak" git push for the old git://-protocol users on Windows.

This also addresses issues #2278 and #2314. The sendpack.sideband setting was previously used to work around the stalled git push issue on Windows, 2.22 broke this, as the patch was deemed obsolete.

I verified this for now by simply using git push versus git -c sendpack.sideband=false push against a local server. Provided I can address the underlying issue, the configuration option can probably be removed again. However, otherwise you should pester me about providing a proper test case for this patch, so it doesn't get accidentally removed again.

Original message below

(I edited the horizontal rulers so they don't mess up the rendered Markdown):

---

Since commit 0c499ea the send-pack builtin 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.

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, and thus makes the dump git protocol work.

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

[jes: split out the documentation into Documentation/config/]

Signed-off-by: Thomas Braun [email protected]
Signed-off-by: Johannes Schindelin [email protected]
Signed-off-by: Oliver Schneider [email protected]

Thanks for taking the time to contribute to Git!

Those seeking to contribute to the Git for Windows fork should see
http://gitforwindows.org/#contribute on how to contribute Windows specific
enhancements.

If your contribution is for the core Git functions and documentation
please be aware that the Git community does not use the github.com issues
or pull request mechanism for their contributions.

Instead, we use the Git mailing list ([email protected]) for code and
documenatation submissions, code reviews, and bug reports. The
mailing list is plain text only (anything with HTML is sent directly
to the spam folder).

Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

[dscho]

There is an existing test for push via git:// in t5700-protocol-v1.sh. However, on Windows, this test is disabled because it wants to use git daemon in a way that required Unix FIFOs (and while the mkfifo command exists in MSYS2, only MSYS programs understand those FIFOs, and git daemon is a MINGW program).

Having said that, I can get the test to pass here via this patch:

diff --git a/daemon.c b/daemon.c
index 9d2e0d20ef3..c4d40e3404b 100644
--- a/daemon.c
+++ b/daemon.c
@@ -15,6 +15,7 @@ static enum log_destination {
 	LOG_DESTINATION_STDERR = 1,
 	LOG_DESTINATION_SYSLOG = 2,
 } log_destination = LOG_DESTINATION_UNSET;
+static FILE *log_tee_file;
 static int verbose;
 static int reuseaddr;
 static int informative_errors;
@@ -88,6 +89,13 @@ static void logreport(int priority, const char *err, va_list params)
 		break;
 	}
 	case LOG_DESTINATION_STDERR:
+		if (log_tee_file) {
+			fprintf(log_tee_file, "[%"PRIuMAX"] ",
+				(uintmax_t)getpid());
+			vfprintf(log_tee_file, err, params);
+			fputc('\n', log_tee_file);
+			fflush(log_tee_file);
+		}
 		/*
 		 * Since stderr is set to buffered mode, the
 		 * logging of different processes will not overlap
@@ -1317,6 +1325,12 @@ int cmd_main(int argc, const char **argv)
 			} else if (!strcmp(v, "stderr")) {
 				log_destination = LOG_DESTINATION_STDERR;
 				continue;
+			} else if (skip_prefix(v, "tee:", &v)) {
+				log_destination = LOG_DESTINATION_STDERR;
+				if (log_tee_file)
+					fclose(log_tee_file);
+				log_tee_file = xfopen(v, "a");
+				continue;
 			} else if (!strcmp(v, "none")) {
 				log_destination = LOG_DESTINATION_NONE;
 				continue;
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index fb8f8870801..0b558bac060 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -21,11 +21,6 @@ then
 	test_done
 fi
 
-if test_have_prereq !PIPE
-then
-	test_skip_or_die GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
-fi
-
 test_set_port LIB_GIT_DAEMON_PORT
 
 GIT_DAEMON_PID=
@@ -52,21 +47,23 @@ start_git_daemon() {
 	fi
 
 	say >&3 "Starting git daemon ..."
-	mkfifo git_daemon_output
 	${LIB_GIT_DAEMON_COMMAND:-git daemon} \
 		--listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
 		--reuseaddr --verbose --pid-file="$GIT_DAEMON_PIDFILE" \
+		--log-destination=tee:git_daemon_output \
 		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
-		>&3 2>git_daemon_output &
+		>&3 2>&4 &
 	GIT_DAEMON_PID=$!
-	{
-		read -r line <&7
-		printf "%s\n" "$line" >&4
-		cat <&7 >&4 &
-	} 7<git_daemon_output &&
+
+	# Wait for the first line in the output
+	while test ! -s git_daemon_output
+	do
+		sleep 1
+	done
 
 	# Check expected output
+	read -r line <git_daemon_output
 	if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
 	then
 		kill "$GIT_DAEMON_PID"
@@ -88,7 +85,7 @@ stop_git_daemon() {
 	kill "$GIT_DAEMON_PID"
 	wait "$GIT_DAEMON_PID" >&3 2>&4
 	ret=$?
-	if ! test_match_signal 15 $ret
+	if ! test_match_signal 15 $ret && test 127 != $ret
 	then
 		error "git daemon exited with status: $ret"
 	fi
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index 7c9511c593c..fc726b3db69 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -67,6 +67,11 @@ test_expect_success 'pull with git:// using protocol v1' '
 test_expect_success 'push with git:// using protocol v1' '
 	test_commit -C daemon_child three &&
 
+	if test_have_prereq MINGW
+	then
+		test_config -C daemon_child sendpack.sideband false
+	fi &&
+
 	# Push to another branch, as the target repository has the
 	# master branch checked out and we cannot push into it.
 	GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 \
@@ -169,7 +174,7 @@ test_expect_success 'create repo to be served by ssh:// transport' '
 
 test_expect_success 'clone with ssh:// using protocol v1' '
 	GIT_TRACE_PACKET=1 git -c protocol.version=1 \
-		clone "ssh://myhost:$(pwd)/ssh_parent" ssh_child 2>log &&
+		clone "ssh://myhost:$PWD/ssh_parent" ssh_child 2>log &&
 	expect_ssh git-upload-pack &&
 
 	git -C ssh_child log -1 --format=%s >actual &&

Obviously, this needs to be split into at least 3 commits: one to set sendpack.sideband=false for pushes via git:// on Windows, one to introduce the --log-destination=tee:<path> option, and one to use it in the git daemon tests instead of the FIFO (and doing away with requiring the FIFO prereq),.

Further, I suspect that that other tests will now start to fail on Windows (most notably, t5570-git-daemon.sh and t5702-protocol-v2.sh) because they had been disabled due to that FIFO prereq.

@assarbad maybe you can take it from here?

[dscho]

Obviously, I mean the PIPE prereq when I wrote FIFO prereq. Time to call it a day.

[dscho]

dscho@fd4bd1e is a pullable version of the patch I provided above. I do not intend to work on this any further ;-)

[assarbad]

Would you like me to incorporate this into the pull request for good measure, @dscho ?

[dscho]

Would you like me to incorporate this into the pull request for good measure, @dscho ?

If you manage to get it into a good shape in time for Git for Windows v2.24.0? That version is due in one week ;-)

What needs to be done is to split the patch, of course (and I just remembered that a $(pwd) -> $PWD change was necessary in t5700 in the SSH-specific part, which would need its own commit message that is based on fd318a9's and possibly references that commit, too).

Also, I would be perfectly fine with using the Azure Pipeline to guide us where the tests needs to be fixed after the change to lib-git-daemon.sh that stops skipping git daemon tests on Windows.

It would be splendid, of course, if it would work out in time for v2.24.0-rc2, which is due this Wednesday. If it does not work out, I would still like to take the PR in the current shape and then work on the tests after v2.24.0 comes out.

Does that sound good?

[assarbad]

Will see what I can do, sure.

[dscho]

Will see what I can do, sure.

Thank you, @assarbad!

[dscho]

It would be splendid, of course, if it would work out in time for v2.24.0-rc2, which is due this Wednesday. If it does not work out, I would still like to take the PR in the current shape and then work on the tests after v2.24.0 comes out.

I will merge this Pull Request, still hoping that we can get the git daemon regression tests to work on Windows in a subsequent Pull Request.

[assarbad]

I didn't find the time during the evenings on workdays. Hoping I'll be able to spend some time over the weekend to integrate working tests. Sorry about the delay.

[dscho]

I didn't find the time during the evenings on workdays.

:-)

Hoping I'll be able to spend some time over the weekend to integrate working tests.

That would be nice ;-) But don't work too hard on it, at this stage I am reluctant to integrate anything but critical bug fixes into v2.24.0 final, anyways.

Sorry about the delay.

No worries!