Rebase to Cygwin v3.6.5 #112
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Corinna Vinschen <[email protected]>
The clock init functions checked timer ticks/period for 0 and then called InterlockedExchange64 if the value was still 0. This is not quite as atomic as it was supposed to be. Use InterlockedCompareExchange64 instead. Fixes: 2b72887 ("Cygwin: clocks: fix a hang on pre-Windows 10 machines") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit 167ace9a6d9605f67cd52369dd1e7758ec0d0af5)
…clocks Add CLOCK_REALTIME_COARSE and CLOCK_REALTIME_ALARM to the clocks allowing an absolute timeout value to be used as such in NtSetTimer. Fixes: c05df02 ("Cygwin: implement extensible clock interface") Fixes: 013e2bd ("Cygwin: posix timers: Add support for CLOCK_REALTIME_ALARM/CLOCK_BOOTTIME_ALARM") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit 2e8c180a837e83e2a37ee68e6b38b40462d5c5d2)
Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit 554173db31ec7dbf32b32c5d9854e4e8ea68eb7f)
With the commit 656df31, if a thread acquires sfp lock after another thread calls fclose() and fp lock is acquired, the first thread falls into deadlock if it tries to acquire fp lock. This can happen if the first thread calls __sfp_lock_all() while the second thread calls fclose(). This patch reverts the changes for newlib/libc/stdio/fclose.c in the commit 656df31. Addresses: https://cygwin.com/pipermail/cygwin/2025-June/258323.html Fixes: 656df31 ("* libc/stdio/fclose.c: Only use sfp lock to guard non-atomic changes of flags and fp lock.") Reported-by: Takashi Yano <[email protected]> Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit e8bc312a2d32a465260e38f948d91f2847a1d00a)
Long-standing bug in the sys/termios.h file which, for some reason,
has never been encountered before.
STC:
#include <sys/termios.h>
int main()
{
struct winsize win;
tcgetwinsize (0, &win);
}
Result with gcc 12.4.0:
termios-bug.c: In function ‘main’:
termios-bug.c:7:20: warning: passing argument 2 of ‘tcgetwinsize’ from incompatible pointer type [-Wincompatible-pointer-types]
7 | tcgetwinsize (0, &win);
| ^~~~
| |
| struct winsize *
In file included from termios-bug.c:1:
/usr/include/sys/termios.h:304:42: note: expected ‘struct winsize *’ but argument is of type ‘struct winsize *’
304 | int tcgetwinsize(int fd, struct winsize *winsz);
| ~~~~~~~~~~~~~~~~^~~~~
This warning apparently becomes an error with C23.
The reason is that struct winsize is defined in sys/termios.h after
using it as argument type in function declarations. From the compil;er
perspective it's now a different type , regardless of having the same
name.
Move declaration of struct winsize up so it's defined before being used.
Reported-by: Zachary Santer <[email protected]>
Suggested-by: Zachary Santer <[email protected]>
Addresses: https://cygwin.com/pipermail/cygwin/2025-July/258474.html
Fixes: 1fd5e00 ("import winsup-2000-02-17 snapshot")
Signed-off-by: Corinna Vinschen <[email protected]>
(cherry picked from commit 73600d68227e125af24b7de7c3fccbd4eb66ee03)
...so that cygheap can be locked/unlocked from outside of mm/cygheap.cc. Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit aec6dc77a11b264fcac73b63ada701854fc62a22)
...completion in child process because the cygheap should not be changed to avoid mismatch between child_info::cygheap_max and ::cygheap_max. Otherwise, child_copy() might copy cygheap being modified by other process. In addition, to avoid deadlock, move close_all_files() for non- Cygwin processes after unlocking cygheap, since close_all_files() calls cfree(), which attempts to lock cygheap even when it's already locked. Fixes: 977ad54 ("* spawn.cc (spawn_guts): Call refresh_cygheap before creating a new process to ensure that cygheap_max is up-to-date.") Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 9b667234bfa6220dcca1afa1233a928d9100cfde)
POSIX states system() shall be thread-safe, however, it is not in current cygwin. This is because ch_spawn is a global and is shared between threads. With this patch, system() uses ch_spawn_local instead which is local variable. popen() has the same problem, so it has been fixed in the same way. Addresses: https://cygwin.com/pipermail/cygwin/2025-June/258324.html Fixes: 1fd5e00 ("import winsup-2000-02-17 snapshot") Reported-by: Takashi Yano <[email protected]> Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 1f836c5f7394ccde8b2f502d632ac8e07835586d)
(cherry picked from commit 9e0162a18d7db74f8692789bf726aa753540fb51)
…_t == UTF-16"
This reverts commit b374973d14ac7969b10ba719feedc709f6971c0d.
Turns out this patch breaks mbrtowc. Example:
--- SNIP ---
void mb(unsigned char c)
{
wchar_t wc;
int ret = mbrtowc(&wc, &c, 1, 0);
printf("%02X -> %04X : %d\n", c, wc, ret);
}
void main ()
{
setlocale (LC_CTYPE, "");
mb(0xF0);
mb(0x9F);
mb(0x98);
mb(0x8E);
}
--- SNAP ---
Output before commit b374973d14ac:
F0 -> 0000 : -2
9F -> 0000 : -2
98 -> D83D : 1
8E -> DE0E : 1
Output after commit b374973d14ac:
F0 -> 0000 : -2
9F -> 0000 : -2
98 -> 0000 : -2
8E -> D83D : 3
By using mbrtowc(), the high surrogate is only emitted after byte 4, and
there's no way to recover the low surrogate. The byte count is also incorrect.
Conclusion: We have to emit the high surrogate already after byte 3
to be able to emit the low surrogate after byte 4.
Reported-by: Thomas Wolff <[email protected]>
Addresses: https://cygwin.com/pipermail/cygwin/2025-July/258513.html
Fixes: b374973d14ac ("mbrtowc: fix handling invalid UTF-8 4 byte sequences if wchar_t == UTF-16")
Signed-off-by: Corinna Vinschen <[email protected]>
(cherry picked from commit ba962ee04543855cfc6e2dc79a7369a78218815a)
When a 4 byte utf-8 sequence has an invalid 4th byte, it's actually an invalid 3 byte sequence. In this case we already generated the high surrogate and only realize the problem when byte 4 doesn't match. At this point _sys_mbstowcs transposes the invalid 4th byte into the private use area. This is wrong. The invalid byte sequence here is the 3 byte sequence already converted to a high surrogate, not the trailing 4th byte. Fix this by backtracking to the start of the broken sequence and overwrite the already written high surrogate with a sequence of the original three bytes transposed to the private use area. Reset the mbstate and restart normal conversion at the non-matching 4th byte, which might start a new multibyte sequence. The resulting wide-char string can be converted back to multibyte and back again to wide-char, and the result will be identical, even if the multibyte sequence differs from the original sequence. Fixes: e44b906 ("* strfuncs.cc (sys_cp_mbstowcs): Treat src as unsigned char *. Convert failure of f_mbtowc into a single malformed utf-16 value.") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit 1463b41d403e861e4033387cdc71006e1664203a)
Add a FIXME comment to the conversion of private use area bytes to "normal" bytes in _sys_wcstombs detailing the conversion difference between _sys_wcstombs and standard wcstombs. It might be a good idea to drop the conversion to gather compatibility with wcstombs. Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit ada3b8f7e5a71db4881135380c407a86a08f6663)
Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit f21fbcaf583e1cadcbca9e094e269632bf0f2a9d)
Previously, process_fd failed to correctly handle fhandlers using an archetype. This was due to the missing PATH_OPEN flag in path_conv, which caused build_fh_pc() to skip archetype initialization. The root cause was a bug where open() did not set the PATH_OPEN flag for fhandlers using an archetype. This patch introduces a new method, path_conv::set_isopen(), to explicitly set the PATH_OPEN flag in path_flags in fhandler_base:: open_with_arch(). Addresses: https://cygwin.com/pipermail/cygwin/2025-May/258167.html Fixes: 92ddb74 ("(build_pc_pc): Use fh_alloc to create. Set name from fh->dev if appropriate. Generate an archetype or point to one here.") Reported-by: Christian Franke <[email protected]> Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit f67a5cffe052b9e746f0baf64fb762e8b2de162d)
The class child_info_spawn has two constructors: one without arguments and one with two arguments. The former does not initialize any members. Commit 1f836c5f7394 used the latter to ensure that the local ch_spawn (i.e., ch_spawn_local) would be properly initialized. However, this was insufficient - it initialized only the base child_info members, not the fields specific to child_info_spawn. This led to the issue reported in https://cygwin.com/pipermail/cygwin/2025-August/258660.html. This patch introduces a new constructor to properly initialize member variable 'ev', etc., which were referred without initialization, and switches ch_spawn_local to use it. 'subproc_ready', which may not be initialized, is also initialized in the constructor of the base class child_info. Addresses: https://cygwin.com/pipermail/cygwin/2025-August/258660.html Fixes: 1f836c5f7394 ("Cygwin: spawn: Make system() thread-safe") Reported-by: Denis Excoffier <[email protected]> Reviewed-by: Jeremy Drake <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 880c96576b2423d1e77e25a3a2da7fc761377728)
Previsouly, FLUSHO did not work correctly.
1) Even when FLUSHO is set, read() returns with undesired data in
the pipe if select() is called in advance.
2) FLUSHO is toggled even in the case pseudo console enabled.
Due to these problems, read data in the pty master was partially
lost when Ctrl-O is pressed.
With this patch, the mask_flusho flag, that was introduced by the
commit 9677efc and caused the issue 1), has been dropped and
select() and read() for pty master discards the pipe data instead
if FLUSHO flag is set. In addition, FLUSHO handling in the case
pseudo console activated is disabled.
Addresses: https://cygwin.com/pipermail/cygwin/2025-August/258717.html
Fixes: 2cab4d0 ("Cygwin: pty, console: Refactor the code processing special keys.")
Fixes: 9677efc ("Cygwin: pty: Make FLUSHO and Ctrl-O work.")
Reported-by: Reported-by: Thomas Wolff <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
(cherry picked from commit 7b5fb35e376cfab42335291008758a2a64ff66b1)
another thread may simultaneously be doing a cmalloc/cfree while the cygheap is being copied to the child. Addresses: https://cygwin.com/pipermail/cygwin/2025-September/258801.html Signed-off-by: Jeremy Drake <[email protected]> (cherry picked from commit 8a5d39527f9a56d1a623e86d30af6b590fd1472d)
In 827743ab76 (Cygwin: symlink_native: allow linking to `..`, 2025-06-20), I fixed linking to `..` (which had inadvertently targeted an incorrect location prior to that fix), but inadvertently broke linking to `.` (which would now try to pass the empty string as `lpTargetFileName` to `CreateSymbolicLinkW()`, failing with an `ERROR_INVALID_REPARSE_DATA` which would be surfaced as "Permission denied"). Let's fix this by special-casing an empty string as path as referring to the current directory. Note: It is unclear to me why the `winsymlinks:nativestrict` code path even tries to simplify the symbolic link's target path (e.g. turn an absolute path into a relative one). As long as it refers to a regular Win32 file or directory, I would think that even something like `././c` should have only the slashes converted, not the path simplified (i.e. `.\.\c` instead of `c`). But that's a larger discussion, and I would like to have the bug worked around swiftly. Fixes: 827743ab76 (Cygwin: symlink_native: allow linking to `..`, 2025-06-20) Signed-off-by: Johannes Schindelin <[email protected]> (cherry picked from commit 60c67169d69577179bfe7b7f9d8c40fdb0a9b5f7)
Cygwin's speclib doesn't handle dashes or dots. However, we are about to rename the output file name from `cygwin1.dll` to `msys-2.0.dll`. Let's preemptively fix up all the import libraries that would link against `msys_2_0.dll` to correctly link against `msys-2.0.dll` instead.
…ent variables to Windows form for native Win32 applications.
…t without ACLs. - Can read /etc/fstab with short mount point format.
The new `winsymlinks` mode `deepcopy` (which is made the default) lets calls to `symlink()` create (deep) copies of the source file/directory. This is necessary because unlike Cygwin, MSYS2 does not try to be its own little ecosystem that lives its life separate from regular Win32 programs: the latter have _no idea_ about Cygwin-emulated symbolic links (i.e. system files whose contents start with `!<symlink>\xff\xfe` and the remainder consists of the NUL-terminated, UTF-16LE-encoded symlink target). To support Cygwin-style symlinks, the new mode `sysfile` is introduced. Co-authored-by: Johannes Schindelin <[email protected]> Co-authored-by: Jeremy Drake <[email protected]>
With MSys1, it was necessary to set the TERM variable to "msys". To allow for a smooth transition from MSys1 to MSys2, let's simply handle TERM=msys as if the user had not specified TERM at all and wanted us to use our preferred TERM value.
Strace is a Windows program so MSYS2 will convert all arguments and environment vars and that makes debugging msys2 software with strace very tricky.
Commit message for this code was: * strace.cc (create_child): Set CYGWIN=noglob when starting new process so that Cygwin will leave already-parsed the command line alonw." I can see no reason for it and it badly breaks the ability to use strace.exe to investigate calling a Cygwin program from a Windows program, for example: strace mingw32-make.exe .. where mingw32-make.exe finds sh.exe and uses it as the shell. The reason it badly breaks this use-case is because dcrt0.cc depends on globbing to happen to parse commandlines from Windows programs; irrespective of whether they contain any glob patterns or not. See quoted () comment: "This must have been run from a Windows shell, so preserve quotes for globify to play with later."
This topic branch fixes the problem where a UTF-16 command-line was converted to UTF-8 in an incorrect way (because Cygwin treated it as if it was a file name and applied some magic that is intended to allow for otherwise invalid file names on Windows). Signed-off-by: Johannes Schindelin <[email protected]>
msys2-runtime: restore fast path for current user primary group
Seeing as Git for Windows tries to stay close to the upstream MSYS2 project, it makes sense to integrate their patches verbatim. Signed-off-by: Johannes Schindelin <[email protected]>
Some Action version updates for MSYS2's CI definition. Signed-off-by: Johannes Schindelin <[email protected]>
Let's handle paths that start with non-ASCII characters well. TODO: Verify that this is still necessary, maybe even add a test to msys2-tests? Signed-off-by: Johannes Schindelin <[email protected]>
Workaround certain anti-malware programs Signed-off-by: Johannes Schindelin <[email protected]>
See https://docs.github.com/en/code-security/dependabot/working-with-dependabot/keeping-your-actions-up-to-date-with-dependabot#enabling-dependabot-version-updates-for-actions for details. Signed-off-by: Johannes Schindelin <[email protected]>
This is a forked repository... Signed-off-by: Johannes Schindelin <[email protected]>
One particularly important part of Git for Windows' MSYS2 runtime is that it is used to run Git's tests, and regressions happened there: For example, the first iteration of MSYS2 runtime v3.5.5 caused plenty of hangs. This was realized unfortunately only after deploying the msys2-runtime Pacman package, and some painful vacation-time scrambling was required to revert to v3.5.4.This was realized unfortunately only after deploying the msys2-runtime Pacman package, and some painful vacation-time scrambling was required to revert to v3.5.4. To verify that this does not happen anymore, let's reuse what `setup-git-for-windows-sdk` uses in Git's very own CI: - determine the latest successful `ci-artifacts` workflow run in git-for-windows/git-sdk-64 - download its Git files and build artifacts - download its minimal-sdk - overwrite the MSYS2 runtime in the minimal-sdk - run the test suite and the assorted validations just like the `ci-artifacts` workflow (from which these jobs are copied) This obviously adds a hefty time penalty (around 7 minutes!) to every MSYS2 runtime PR in the git-for-windows org. Happily, these days we don't need many of those, and the balance between things like the v3.5.5 scramble and waiting a little longer for the CI to finish is clearly in favor of the latter. Co-authored-by: Jeremy Drake <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
ci: run Git's entire test suite
AutoHotKey is not only a convenient way to add keyboard shortcuts for functionality (or applications) that does not come with shortcuts, but it is in general a powerful language to remote control GUI elements. We will use this language to implement a couple of automated tests that should hopefully prevent regressions as we have experienced in the past (for example, a regression that was fixed and immediately re-broken, which went unnoticed for months). So let's start by adding a library of useful functions, to be extended as needed. Signed-off-by: Johannes Schindelin <[email protected]>
The issue reported in microsoft/git#730 was fixed, but due to missing tests for the issue a regression slipped in within mere weeks. Let's add an integration test that will (hopefully) prevent this issue from regressing again. This integration test is implement as an AutoHotKey script. It might look unnatural to use a script language designed to implement global keyboard shortcuts, but it is a quite powerful approach. While there are miles between the ease of developing AutoHotKey scripts and developing, say, Playwright tests, there is a decent integration into VS Code (including single-step debugging), and AutoHotKey's own development and community are quite vibrant and friendly. I had looked at alternatives to AutoHotKey, such as WinAppDriver, SikuliX, nut.js and AutoIt, in particular searching for a solution that would have a powerful recording feature similar to Playwright, but did not find any that is 1) mature, 2) well-maintained, 3) open source and 4) would be easy to integrate into a GitHub workflow. In the end, AutoHotKey appeared my clearest preference. So how is the test implemented? It lives in `ui-test/` and requires AutoHotKey v2 as well as Windows Terminal (the Legacy Prompt would not reproduce the problem). It then follows the reproducer I gave to the Cygwin team: 1. initialize a Git repository 2. install a `pre-commit` hook 3. this hook shall spawn a non-Cygwin/MSYS2 process in the background 4. that background process shall print to the console after Git exits 5. open a Command Prompt in Windows Terminal 6. run `git commit` 7. wait until the background process is done printing 8. press the Cursor Up key 9. observe that the Command Prompt does not react (in the test, it _does_ expect a reaction: the previous command in the command history should be shown, i.e. `git commit`) In my reproducer, I then also suggested to press the Enter key and to observe that now the "More ?" prompt is shown, but no input is accepted, until Ctrl+Z is pressed. Naturally, the test should not expect _that_ ;-) There were a couple of complications I needed to face when developing this test: - I did not find any easy macro recorder for AutoHotKey that I liked. It would not have helped much, anyway, because intentions are hard to record. - Before I realized that there is excellent AutoHotKey support in VS Code via the AutoHotKey++ and AutoHotKey Debug extensions, I struggled quite a bit to get the syntax right. - Windows Terminal does not use classical Win32 controls that AutoHotKey knows well. In particular, there is no easy way to capture the text that is shown in the Terminal. I tried the (pretty excellent!) [OCR for AutoHotKey](https://github.com/Descolada/OCR), but it uses UWP OCR which does not recognize constructs like "C:\Users\runneradmin>" because it is not English (or any other human language). I ended up with a pretty inelegant method of selecting the text via mouse movements and then copying that into the clipboard. This stops scrolling and I worked around that by emulating the mouse wheel afterwards. - Since Windows Terminal does not use classical Win32 controls, it is relatively hard to get to the exact bounding box of the text, as there is no convenient way to determine the size of the title bar or the amount of padding around the text. I ended up hard-coding those values, I'm not proud of that, but at least it works. - Despite my expectations, `ExitApp` would not actually exit AutoHotKey before the spawned process exits and/or the associated window is closed. For good measure, run this test both on windows-2022 (corresponding to Windows 10) and on windows-2025 (corresponding to Windows 11). Co-authored-by: Eu-Pin Tien <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]> Converted ui-tests workflow into a test matrix that uses both the windows-2022 and windows-2025 runners.
The test logs are quite interesting to have, and not only those: In case of a fatal failure, the test directory is valuable information, too. Let's always upload them as build artifacts. For convenience, let's just reuse the `ui-tests/` directory as the place to put all of those files; Technically, we do not need the files in there that are tracked by Git, but practically speaking, it is neat to have them packaged in the same `.zip` file as the test logs and stuff. Signed-off-by: Johannes Schindelin <[email protected]>
Sometimes the logs are empty and it is highly unclear what has happened.
In such a scenario, a picture is indeed worth more than a thousand
words.
Note that this commit is more complicated than anyone would like, for
two reasons:
- While PowerShell is the right tool for the job, a PowerShell step in
GitHub Actions will pop up a Terminal window, _hiding_ what we want to
screenshot. To work around that, I tried to run things in a Bash step.
_Also_ opens a Terminal window! Node.js to the rescue.
- _Of course_ it is complicated to take a screenshot. The challenge is
to figure out the dimensions of the screen, which should be as easy as
looking at `[System.Windows.Forms.Screen]::PrimaryScreen`'s `Bounds`
attribute.
Easy peasy, right? No, it's not. Most machines nowadays have a
_ridiculous_ resolution which is why most setups have a _zoom factor_.
Getting to that factor should be trivial, by calling
`GetDeviceCaps(hDC, LOGPIXELSX)`, but that's not working in modern
Windows! There is a per-monitor display scaling ("DPI"). But even
_that_ is hard to get at, calling `GetDpiForMonitor()` will still
return 96 DPI (i.e. 100% zoom) because PowerShell is not marked as
_Per-Monitor DPI Aware_. Since we do not want to write a manifest into
the same directory as `powershell.exe` resides, we have to jump
through yet another hoop to get that.
Signed-off-by: Johannes Schindelin <[email protected]>
The Ctrl+C way to interrupt run-away processes is highly important. It was recently broken in multiple ways in the Cygwin runtime (and hence also in the MSYS2 runtime). Let's add some integration tests that will catch regressions. It is admittedly less than ideal to add _integration_ tests; While imitating exactly what the end user does looks appealing at first, excellent tests impress by how quickly they allow regressions not only to be identified but also to be fixed. Even worse: all integration tests, by virtue of working in a broader environment than, say, unit tests, incur the price of sometimes catching unactionable bugs, i.e. bugs in software that is both outside of our control as well as not the target of our testing at all. Nevertheless, seeing as Cygwin did not add any unit tests for those Ctrl+C fixes (which is understandable, given how complex testing for Ctrl+C without UI testing would be), it is better to have integration tests than no tests at all. So here goes: This commit introduces a test that verifies that the MSYS2 `sleep.exe` can be interrupted when run from PowerShell in a Windows Terminal. This was broken in v3.6.0 and fixed in 7674c51 (Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread, 2025-07-01). Signed-off-by: Johannes Schindelin <[email protected]>
This was the actual use case that was broken and necessitated the fix in 7674c51 (Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread, 2025-07-01). It does require an SSH server, which Git for Windows no longer ships. Therefore, this test uses the `sshd.exe` of OpenSSH for Windows (https://github.com/powershell/Win32-OpenSSH) in conjunction with Git for Windows' `ssh.exe` (because using OpenSSH for Windows' variant of `ssh.exe` would not exercise the MSYS2 runtime and therefore not demonstrate a regression, should it surface in the future). To avoid failing the test because OpenSSH for Windows is not available, the test case is guarded by the environment variable `OPENSSH_FOR_WINDOWS_DIRECTORY` which needs to point to a directory that contains a working `sshd.exe`. Signed-off-by: Johannes Schindelin <[email protected]>
In the previous commit, I added a new UI test that generates a somewhat large repository for testing the clone via SSH. Since that repository is created in the test directory, that would inflate the `ui-tests` build artifact rather dramatically. So let's create the repository outside of that directory. Signed-off-by: Johannes Schindelin <[email protected]>
The fixes of 7674c51 (Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread, 2025-07-01) were unfortunately not complete; There were still a couple of edge cases where Ctrl+C was unable to interrupt processes. Let's add a demonstration of that issue. Signed-off-by: Johannes Schindelin <[email protected]>
In 0ae6a6f (Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader, 2025-06-27), a quite problematic bug was fixed where somewhat large-ish repositories could not be cloned via SSH anymore. This fix was not accompanied by a corresponding test case in Cygwin's test suite, i.e. there is no automated way to ensure that there won't be any regressions on that bug (and therefore it would fall onto end users to deal with those). This constitutes what Michael C. Feathers famously characterized as "legacy code" in his book "Working Effectively with Legacy Code": To me, legacy code is simply code without tests. I've gotten some grief for this definition. What do tests have to do with whether code is bad? To me, the answer is straightforward, and it is a point that I elaborate throughout the book: Code without tests is bad code. It doesn’t matter how well written it is; it doesn’t matter how pretty or object-oriented or well-encapsulated it is. With tests, we can change the behavior of our code quickly and verifiably. Without them, we really don’t know if our code is getting better or worse. Just to drive this point home, let me pull out Exhibit A: The bug fix in question, which is the latest (and hopefully last) commit in a _long_ chain of bug fixes that fix bugs introduced by preceding bug fixes: - 9e4d308 (Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe., 2021-11-10) fixed a bug where Cygwin hung by mistake while piping output from one .NET program as input to another .NET program (potentially introduced by 3651990 (Cygwin: pipe: Avoid false EOF while reading output of C# programs., 2021-11-07), which was itself a bug fix). It introduced a bug that was fixed by... - fc691d0 (Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps., 2024-03-11). Which introduced a bug that was purportedly fixed by... - 7ed9adb (Cygwin: pipe: Switch pipe mode to blocking mode by default, 2024-09-05). Which introduced a bug that was fixed by... - cbfaeba (Cygwin: pipe: Fix incorrect write length in raw_write(), 2024-11-06). Which introduced a bug that was fixed by... the SSH hang fix in 0ae6a6f (Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader, 2025-06-27). There is not only the common thread here that each of these bug fixes introduced a new bug, but also the common thread that none of the commits introduced new test cases into the test suite that could potentially have helped prevent future breakages in this code. So let's at least add an integration test here. Side note: I am quite unhappy with introducing integration tests. I know there are a lot of fans out there, but I cannot help wondering whether they favor the convenience of writing tests quickly over the vast cost of making debugging any regression a highly cumbersome and unenjoyable affair (try single-stepping through a test case that requires several processes to be orchestrated in unison). Also, integration tests have the large price of introducing moving parts outside the code base that is actually to be tested, opening the door for breakages caused by software (or infrastructure, think: network glitches!) that are completely outside the power or responsibility of the poor engineer tasked with fixing the breakages. Nevertheless, I have been unable despite days of trying to wrap my head around the issue to figure out a way to reproduce the `fhandler_pipe_fifo::raw_write()` hang without involving a MINGW `git.exe` and an MSYS2/Cygwin `ssh.exe`. So: It's the best I could do with any reasonable amount of effort. It's better to have integration tests that would demonstrate regressions than not having any tests for that at all. Signed-off-by: Johannes Schindelin <[email protected]>
There have been way too many regressions in Cygwin as of late, in particular in the console handling. The worst part? Many of those bugs were introduced _in bug fix patches_! Here are a bunch of tests that are designed to help Git for Windows increase confidence in upgrades to newer Cygwin versions. Signed-off-by: Johannes Schindelin <[email protected]>
mjcheetham
approved these changes
Oct 10, 2025
Member
Author
Range-diff
I used this opportunity to do a lot of stuff:
|
Member
Author
|
/open pr The workflow run was started |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The usual thing...