Skip to content

Commit

Permalink
msys2-runtime: fix Git's t3701-add-interactive
Browse files Browse the repository at this point in the history
This reverts the problematic commit that causes a hang when `git add
-p` has `cat.exe` configured as a diff filter, and that diff filter is
fed input larger than the pipe's buffer size.

This fixes git-for-windows/git#4422.

Signed-off-by: Johannes Schindelin <[email protected]>
  • Loading branch information
dscho committed May 16, 2023
1 parent 87dab81 commit d81a17c
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 5 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
From 0e357e24075b2e8b66c9772bf56d3db6203f88fa Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <[email protected]>
Date: Tue, 16 May 2023 12:43:51 +0200
Subject: [PATCH 61/N] Revert "Cygwin: pipe: Restore blocking mode for cygwin
process at startup."

When Git for Windows upgraded its MSYS2 runtime from v3.3.* to v3.4.* at
long last, the t3701-add-interactive test of Git's test suite started to
hang consistently, timing out.

The culprit is a `git add -p` call that wants to read from a diff
filter. This diff filter is `cat.exe`, i.e. nothing special, but that
diff filter gets input that is larger than the pipe buffer, and
therefore must not block. Yet that is exactly what it does.

This was a regression that introduced by upgrading from MSYS2 runtime
v3.3.*. After a tedious, multi-day bisection session, the commit
introducing that regression was identified as 90788821b7 (Cygwin: pipe:
Restore blocking mode for cygwin process at startup., 2021-11-17), which
we hereby revert.

So what bug does this reversion reintroduce?

The commit seems to have been in response to
https://inbox.sourceware.org/cygwin/CAEv6GOB8PXHgHoz7hdJy6Bia2GWEmUDnTd252gTGinz2vuv=hA@mail.gmail.com/
which reported a bug when a C# program writes 0-byte content to a Cygwin
pipe. (Irony of ironies, this report originated from Git's realm, over
at https://github.com/git-ecosystem/git-credential-manager/issues/509.)

The analysis revealed, back in December '21, that a `CYGWIN=pipe_byte`
would work around the bug, too, but that did not fix the regression in
Git's test suite.

That leaves us with the somewhat unsatisfying conclusion that we _might_
reintroduce a regression when Git Credential Manager tries to talk to an
_MSYS_ `git.exe`. But since we do not need that in Git for Windows, and
since we need to have CI builds that do not time out after 6h causing
failures, it is better to revert that commit, and in the hopefully
unlikely event that this causes _other_ problems in Git for Windows'
ecosystem, we will simply have to revisit this issue in more depth.

This fixes https://github.com/git-for-windows/git/issues/4422.

Signed-off-by: Johannes Schindelin <[email protected]>
---
winsup/cygwin/dtable.cc | 3 ---
1 file changed, 3 deletions(-)

diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index f90465d..ce1bbbe 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,6 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
}

if (!fh->open_setup (openflags))
10 changes: 6 additions & 4 deletions msys2-runtime/PKGBUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
pkgbase=msys2-runtime
pkgname=('msys2-runtime' 'msys2-runtime-devel')
pkgver=3.4.6
pkgrel=1
pkgrel=2
pkgdesc="Cygwin POSIX emulation engine"
arch=('x86_64')
url="https://www.cygwin.com/"
Expand Down Expand Up @@ -86,9 +86,10 @@ source=('msys2-runtime'::git+https://github.com/cygwin/cygwin#tag=cygwin-${pkgve
0057-Use-MB_CUR_MAX-6-by-default.patch
0058-Change-the-default-base-address-for-x86_64.patch
0059-Avoid-sharing-cygheaps-across-Cygwin-versions.patch
0060-uname-report-msys2-runtime-commit-hash-too.patch)
0060-uname-report-msys2-runtime-commit-hash-too.patch
0061-Revert-Cygwin-pipe-Restore-blocking-mode-for-cygwin-.patch)
sha256sums=('SKIP'
'67751e4d22f6e80eedde68c7d63fcbc4b8d43a6f61728ea4105908d7e8787a3a'
'0dd8039cc7b85e5a97038090d7a602f4accc617644588f5efb93a36e349d8242'
'e061d933f7ec1a6ba5a8c56cbdb9e796e3df266409893afba33d9f4f5365f000'
'52f3b36d3e420374b75af3502afb971279b70828a88fe05870fe35ccab90e2f0'
'd1526bdea50b470fbbf5a6d5d5d7814a1bed7940d380e140153c290ead320979'
Expand Down Expand Up @@ -148,7 +149,8 @@ sha256sums=('SKIP'
'0dd0b9dc3d24e6556c5f6c758a3157405ae2a75b3760fab92acc3b183151bb50'
'6e6c217266559b46cd41e60b6cbe8f87a6166c7ea51d8c07241974c504c9ec15'
'7512fc55bbb0bc6c111ce05aa90269eb4423de8092a063be4009404967b1c6bd'
'77f4b172f0e3aee99e1c2da966efffa2e5440ef91e8374a53c488923baac1751')
'77f4b172f0e3aee99e1c2da966efffa2e5440ef91e8374a53c488923baac1751'
'c82378cdce57d224069bb3a84db2c4582449ab61f487b41cf5bef70219d5fddb')

# Helper macros to help make tasks easier #
apply_patch_with_msg() {
Expand Down
2 changes: 1 addition & 1 deletion msys2-runtime/msys2-runtime.commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
82f0d822f7e5ed5b4c5f335bc014a6404f7036f1
0e357e24075b2e8b66c9772bf56d3db6203f88fa

0 comments on commit d81a17c

Please sign in to comment.