Rebase to Cygwin v3.6.4 #106
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
After inherited permissons were removed, apparently there were no permissions left allowing access, and GHA recently started failing on actions/checkout with EPERM. Signed-off-by: Jeremy Drake <[email protected]>
(cherry picked from commit 2029784e05d9805aa074dcadb99c31311790b7ac)
When the thread is suspended and Rip (instruction pointer) points to an instruction that causes an exception, modifying Rip and calling ResumeThread() may sometimes result in a crash. To prevent this, advance execution by a single instruction by setting the trap flag (TF) before calling ResumeThread() as a workaround. This will trigger either STATUS_SINGLE_STEP or the exception caused by the instruction that Rip originally pointed to. By suspending the targeted thread within exception::handle(), Rip no longer points to the problematic instruction, allowing safe handling of the interrupt. As a result, Rip can be adjusted appropriately, and the thread can resume execution without unexpected crashes. Addresses: https://cygwin.com/pipermail/cygwin/2025-May/258153.html Fixes: 1fd5e00 ("import winsup-2000-02-17 snapshot") Reported-by: Christian Franke <[email protected]> Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit f305ca916ad25870fb010334e4fcaf93057c78b9)
This patch partially reverts the commit b7097ab because it seems to cause issues when longjmp is used within a signal handler. The problem that the commit addressed no longer occurs even if this change is reverted. Fixes: b7097ab ("Cygwin: signal: Revive toggling incyg flag in call_signal_handler()") Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 9c3e1e9ea9d2d930fef1ae6a62e61b101c1e047c)
Currently, _cygtls::sigmask is set in call_signal_handler(), but this is too late to effectively prevent a masked signal from being armed. With this patch, deltamask, which is set in _cygtls::interrupt_setup() in advance, is also checked as well as sigmask to determine if the signal can be armed. Fixes: 0d675c5 ("* exceptions.cc (interrupt_setup): Don't set signal mask here or races occur with main thread. Set it in sigdelayed instead.") Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 364226a4caaa277d86fcc0c58ef862cb23a50603)
(cherry picked from commit 83af2fd6d1018bfe1c08c0028f65076f57684474) Signed-off-by: Radek Bartoň <[email protected]>
In order to avoid a restriction on any reinterpret_cast-like behavior in constinit expressions, use assembly and the linker to define symbols with the not-valid-address addresses. Due to being built with -mcmodel=small, this linker trick does not always work in the Cygwin DLL, specifically in the cases where they are used in comparisons (==). As a result, leave the case where the symbols would have had to have been classes rather than structs as casts of integers. If Cygwin ever needs these to be constinit compliant, it may need to move to the medium code model or add some other workaround. Signed-off-by: Jeremy Drake <[email protected]> (cherry picked from commit 5be6ebd4d5c3492c539e662cc8849c284a456e7d)
When running CYGWIN=winsymlinks:nativestrict ln -s .. abc the counter-intuitive result is _not_ a symbolic link to `..`, but instead to `../../$(basename "$PWD")`. The reason for this is that the search for the longest common prefix assumes that the link target is not a strict prefix of the parent directory of the link itself. Let's fix that. Signed-off-by: Johannes Schindelin <[email protected]> (cherry picked from commit 827743ab7684a09bc077c91f6206e16ea85881b4)
Signed-off-by: Johannes Schindelin <[email protected]> (cherry picked from commit d8e6d54f3cc5b0dd51ae7b8d7b88b9dae7d51079)
After the commit f305ca916ad2, some stress-ng tests fail in arm64 windows. There seems to be two causes for this issue. One is that calling SuspendThread(GetCurrentThread()) may suspend myself in the kernel. Branching to sigdelayed in the kernel code does not work as expected as the original _cygtls::interrup_now() intended. The other cause is, single step exception sometimes does not trigger exception::handle() for some reason. Therefore, register vectored exception handler (VEH) and use it for single step exception instead. Addresses: https://cygwin.com/pipermail/cygwin/2025-June/258332.html Fixes: f305ca916ad2 ("Cygwin: signal: Prevent unexpected crash on frequent SIGSEGV") Reported-by: Jeremy Drake <[email protected]> Reviewed-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit b0a9b628aad8dd35892b9da3511c434d9a61d37f)
…F-16 When commit 28186e8 split _mbtowc_r into per-codeset functions, the code generating wchar_t from UTF-8 input was slightly rearranged. Unfortunately the new code introduced a bug: On systems with wchar_t being UTF-16, 4 byte sequences have to be converted to surrogate pairs. The low surrogate pair can be fully created from the first 3 bytes of the sequence. However, the surrogates should only be created if it's clear that the 4th byte is valid, and the entire 4 byte string represents a valid UTF-8 sequence. The code change in 28186e8 neglected just that: In contrast to the original code, it now created the low surrogate after having read the first 3 bytes of the sequence, without checking validity of the 4th byte. This patch moves the test sequence to check the 4th byte in front of the code generating the low surrogate. Make sure to return the value 3 (3 bytes digested) rather than the content of the local variable i, which is already set to 4 at this point. Reported-by: Christian Franke <[email protected]> Addresses: https://cygwin.com/pipermail/cygwin/2025-June/258358.html Fixes: 28186e8 ("* libc/ctype/iswalpha.c: Handle all wchar_t as unicode on _MB_CAPABLE systems.") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit b374973d14ac7969b10ba719feedc709f6971c0d)
Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit 5c8475417bc3b5c93c6c9b23f245211828cde50c)
…able The mechanism to convert characters invalid in DOS/Windows filenames to the Unicode private use area starting at 0xf000 accidentally also converts ASCII NUL to the private use area. That usually doesn't happen, but what if the filename contains the 0xf000 character and we want to convert it back to multibyte? In this case the 0xf000 becomes the ASCII NUL again, and the resulting string is cut short and does not match with the real filename. This leads to all sorts of problems like the inability to unlink the file. Fix this problem by not marking ASCII NUL as transposable character. This problem has been introduced by adding code to support abstract sockets which may contain a NUL char as part of the socket name. By reverting the ASCII NUL in the transposition tables, transform_chars_af_unix has to handle ASCII NUL explicitely now. Reported-by: Christian Franke <[email protected]> Addresses: https://cygwin.com/pipermail/cygwin/2025-June/258367.html Fixes: 7d260cf ("Cygwin: add transform_chars_af_unix helper") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit 61e38a8fe0270797b4c9b9f2e2ce33265d498aac)
If ssh is used with non-cygwin pipe reader, ssh some times hangs. This happens when non-cygwin git (Git for Windows) starts cygwin ssh. The background of the bug is as follows. Before attempting to NtWriteFile() in raw_write() in non-blocking mode, the amount of writable space in the pipe is checked by calling NtQueryInformationFile with FilePipeLocalInformation parameter. The same is also done by pipe_data_available() in select.cc. However, if the read side of the pipe is simultaneously consuming data, NtQueryInformationFile() returns less value than the amount of writable space, i.e. the amount of writable space minus the size of buffer to be read. This does not happen when the reader is a cygwin app because cygwin read() for the pipe attempts to read the amount of the data in the pipe at most. This means NtReadFile() never enters a pending state. However, if the reader is non-cygwin app, this cannot be expected. As a workaround for this problem, the code checking the pipe space temporarily attempts to toggle the pipe-mode. If the pipe contains data, this operation fails with STATUS_PIPE_BUSY indicating that the pipe is not empty. If it succeeds, the pipe is considered empty. The current code uses this technic only when NtQueryInformationFile() retuns zero. Therefore, if NtQueryInformationFile() returns 1, the amount of writable space is assumed to be 1 even in the case that e.g. the pipe size is 8192 bytes and reader is pending to read 8191 bytes. Even worse, the current code fails to write more than 1 byte to 1 byte pipe space due to the remnant of the past design. Then the reader waits for data with 8191 bytes buffer while the writer continues to fail to write to 1 byte space of the pipe. This is the cause of the deadlock. In practice, when using Git for Windows in combination with Cygwin SSH, it has been observed that a read of 8191 bytes is occasionally issued against a pipe with 8192 bytes of available space. With this patch, the blocking-mode-toggling-check is performed even if NtQueryInformationFile() returns non-zero value so that the amount of the writable space in the pipe is always estimated correctly. Addresses: git-for-windows/git#5682 Fixes: 7ed9adb ("Cygwin: pipe: Switch pipe mode to blocking mode by default") Reported-by: Vincent-Liem (@github), Johannes Schindelin <[email protected]> Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 5786a8ac4dd9dbc79559f00e3b37538610aec1ad)
The commit "Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader" modifies how the amount of writable data is evaluated. This patch updates the source comments to align with that change. Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 82d4ad1dfb28c72d5ce5fd3135ff4dde99e85fca)
... rather than 1 when the pipe space estimation fails, so that select() and raw_wrie() can perform appropriate fallback handling. In select(), even if pipe space is unknown, return writable to avoid deadlock. Even with select() returns writable, write() can blocked anyway, if data is larger than pipe space. In raw_write(), if the pipe is real non-blocking mode, attempting to write larger data than pipe space is safe. Otherwise, return error. For other cases than FH_PIPEW, PDA_UNKNOWN never orrurs. Therefore, it is not necessary to handle it in that cases. Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 6d6af651151d48796204d4a358db8ef3c121a501)
…ble() ipipe_data_available() is called from raw_write(). If the pipe is in real_non_blocking_mode at that time, calling pipe_data_available() can, in some cases, inadvertently revert the pipe to blocking mode. Here is the background: pipe_data_available() checks the amount of writable space in the pipe by calling NtQueryInformationFile() with the FilePipeLocalInformation parameter. However, if the read side of the pipe is simultaneously consuming data with a large buffer, NtQueryInformationFile() may return 0 for WriteQuotaAvailable. As a workaround for this behavior, pipe_data_available() temporarily attempts to change the pipe-mode to blocking. If the pipe contains data, this operation fails-indicating that the pipe is full. If it succeeds, the pipe is considered empty. The problem arises from the assumption that the pipe is always in real blocking mode before attempting to flip the mode. However, if raw_write() has already set the pipe to non-blocking mode due to its failure to determine available space, two issues occur: 1) Changing to non-blocking mode in pipe_data_available() always succeeds, since the pipe is already in non-blocking mode. 2) After this, pipe_data_available() sets the pipe back to blocking mode, unintentionally overriding the non-blocking state required by raw_write(). This patch addresses the issue by having pipe_data_available() check the current real blocking mode, temporarily flip the pipe-mode and then restore the pipe-mode to its original state. Addresses: git-for-windows/git#5682 (comment) Fixes: 7ed9adb ("Cygwin: pipe: Switch pipe mode to blocking mode by default") Reported-by: Andrew Ng <[email protected]> Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 940dbeffa7134dcd0795bd146f60edac3548e32e)
Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 41104f60e72f8fa3cf81863aa51e87d97039c68d)
Fixes: 940dbeffa713 ("Cygwin: pipe: Fix unexpected blocking mode change by pipe_data_available()")
Reported by: Andrew Ng <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
(cherry picked from commit bc95e1bef29b7b1c9be00f8cd342d303466ee350)
Currently, ENABLE_PROCESSED_INPUT is set in set_input_mode() if master_thread_suspended is true. This enables Ctrl-C handling when cons_master_thread is suspended, since Ctrl-C is normally handled by cons_master_thread. However, when disable_master_thread is true, ENABLE_PROCESSED_INPUT is not set, even though this also disables Ctrl-C handling in cons_master_thread. Due to this bug, the command C:\cygwin64\bin\sleep 10 < NUL in the Command Prompt cannot be terminated with Ctrl-C. This patch addresses the issue by setting ENABLE_PROCESSED_INPUT when either disable_master_thread or master_thread_suspended is true. This bug also affects cases where non-Cygwin Git (Git for Windows) launches Cygwin SSH. In such cases, SSH also cannot be terminated with Ctrl-C. Addresses: git-for-windows/git#5682 (comment) Fixes: 746c811 ("Cygwin: console: Allow pasting very long text input.") Reported-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 476135a24506dd624eb46b50fd634fcd740008ba)
A __WCTOMB call on the terminating NUL may emit more than a NUL byte. This is the case if the string ends with a lone UTF-16 high surrogate. Fixes: 2a3a02a ("Add SUSV2 support for calculating size if output buffer is NULL") Signed-off-by: Christian Franke <[email protected]> (cherry picked from commit fa272e05bbd049bb39420c6c8997700397283100)
…thread
With the commit 476135a24506, set_input_mode() reffers to the flag
disable_master_thread in tty::cygwin mode. So it is necessary to call
set_input_mode() after changing disable_master_thread flag. However,
the commit 476135a24506 was missing that.
With this patch, set_input_mode() is called after changing the flag
disable_master_thread, if the console input mode is tty::cygwin.
Fixes: 476135a24506 ("Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread");
Signed-off-by: Takashi Yano <[email protected]>
(cherry picked from commit 65a48c7202387f8dff2646e89716f712db29d811)
We're using RtlQueryProcessDebugInformation from dlsym since commit 31ddf45 ("* autoload.cc (EnumProcessModules): Remove.") Observations on the Cygwin mailing list show that calling RtlQueryProcessDebugInformation on a process is neither thread-safe, nor multi-process-safe, see https://cygwin.com/pipermail/cygwin/2025-July/258403.html for details. This patch essentially reverts 31ddf45. Fetch the list of loaded modules in the current process by calling EnumProcessModules again. Reported-by: Christian Franke <[email protected]> Fixes: 31ddf45 ("* autoload.cc (EnumProcessModules): Remove.") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit 8ac2bc46b186ba3234e5f50931805308ead4af8d)
To fetch heap info for a process in our /proc/PID/maps emulation, we call RtlQueryProcessDebugInformation on this process since commit b4966f9 ("(heap_info::heap_info): Rearrange using RtlQueryProcessDebugInformation"). However, it turns out that this call can crash the targeted process, if it's called from multiple threads or processes in parallel. Worse, the entire code from creating the debug buffer, over fetching the debug info, subsequent collecting the information from said debug buffer, up to destroying the buffer, needs to be guarded against parallel access. We do this by adding a global mutex object, serializing access to the debug info of a process. Reported-by: Christian Franke <[email protected]> Fixes: b4966f9 ("(heap_info::heap_info): Rearrange using RtlQueryProcessDebugInformation") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit f075f3ba2720b4cca65e63b67c865959ba6b6c92)
Previously, it was always applied to "myself", which is only appropriate in the case of _P_OVERLAY (aka exec). Apply the flag to "myself" only in that case, and apply it to the new child instead in other cases. Also, clear the flag from "myself" in the case of a failed exec. Fixes: 8d8724e ("Cygwin: pty: Fix Ctrl-C handling further for non-cygwin apps.") Signed-off-by: Jeremy Drake <[email protected]>
Only try to fix the cached DOS attributes if the caller actually tried to create the file. Otherwise the fixup code is called even in cases where we open the file with minimal query access bits and NtQueryInformationFile() could fail with STATUS_ACCESS_DENIED and the new cached DOS attributes are wrong again. Fixes: 37c49de ("Cygwin: open: only fix up cached DOS file attributes for on-disk files") Signed-off-by: Corinna Vinschen <[email protected]> (cherry picked from commit c86c08e0ab41ba8d181876a4bdcfa4cd62264827)
Previously, TCIFLUSH flushed the pipe to_slave which transfers input from master to slave. However, this was not sufficiant. The master side holds input data before accept_input() in the read-ahead buffer. So, if input data before 'enter' key can be leaked into slave input after TCIFLUSH. With this patch, TCIFLUSH requests master to flush read-ahead buffer via master control pipe. To realize this, add cmd filed to pipe_request structure so that the flush request can be distinguished from existing pipe handle request. Addresses: https://cygwin.com/pipermail/cygwin/2025-July/258442.html Fixes: 41946df (" (fhandler_tty_slave::tcflush): Implement input queue flushing by calling read with NULL buffer.") Reported-by: Christoph Reiter <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 2aa41b516055ea9383508342706288deb3baf1f6)
Raw devices of partitions may be accessible from unprivileged processes, for example if connected via USB. Signed-off-by: Christian Franke <[email protected]> (cherry picked from commit 8065978ff838f393dfbc7e61374df56f1f157847)
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.
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 `ui-tests` job currently takes around half a minute to a minute. Therefore it is quite alarming when the job times out after the default of 6h. Let's let it time out after 10 minutes (we can always increase it when more tests are added). Unfortunately, this changes the failure mode and `cancelled()` will no longer return `true`. Which means that the existing `if:` clauses have to be adapted for showing the logs and taking & uploading a screenshot. While thinking of the logs, I realized that it would be good also to have them in the case that the run succeeded, so let's always show them. Signed-off-by: Johannes Schindelin <[email protected]>
Git is still totally unable to represent DOS line endings gracefully, so let's just follow the German wisdom that between two parties, the wise one gives in. Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
…erminal This function will soon be moved into a library, therefore it makes sense to give it a non-generic name. Signed-off-by: Johannes Schindelin <[email protected]>
These functions will be used in an upcoming test script to verify that Ctrl+C does what it is supposed to do. Signed-off-by: Johannes Schindelin <[email protected]>
This base name should be different for the different UI tests (once we have more than one) so that they do not clash. Signed-off-by: Johannes Schindelin <[email protected]>
And likewise, have a tear-down function (CleanUpWorkTree()). Signed-off-by: Johannes Schindelin <[email protected]>
It is a tricky (and somewhat fragile) part of the testing framework, let's document what it does, how, and why it's done this way. Signed-off-by: Johannes Schindelin <[email protected]>
…unction This code is way too useful _not_ to make it reusable. While at it, improve on the timeout logic to be quite a bit more precise. Signed-off-by: Johannes Schindelin <[email protected]>
It used only 3 spaces, when 4 were called for. Signed-off-by: Johannes Schindelin <[email protected]>
There is actually no need to have a flashing window just to capture the output of the command: using the trick (use `Run()`, pipe to `clip` and then use `A_Clipboard`) from https://stackoverflow.com/a/32298415 makes it possible to hide the console window _and_ capture the output. Signed-off-by: Johannes Schindelin <[email protected]>
The `uname` executable is not on the `PATH` by default. Let's use the Git alias trick to execute it, then. Signed-off-by: Johannes Schindelin <[email protected]>
When the command fails, we should not simply continue, mistaking an empty string for being the valid output of a successful run. 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 imitates Unix shell's `$(...)` behavior better and is eminently more useful when capturing, say, the output of `cygpath`. Signed-off-by: Johannes Schindelin <[email protected]>
I want to add an integration test that clones a repository via SSH. Unfortunately, I have not found a better way than to run OpenSSH for Windows' `sshd.exe` in debug mode, in which case every successful connection will open a terminal window. Let's be prepared for these annoying extra windows by forcefully activating the desired window as needed. This change is admittedly a bit intrusive; I did not find any more elegant way to achieve the same result, though. 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]>
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
14 participants
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.
Range-diff relative to main
1: f51657b < -: ---------- Add MSYS2 triplet
2: 3155a8c = 1: b662426 Fix msys library name in import libraries
3: d9b4e1d ! 2: 50d9d80 Rename dll from cygwin to msys
4: 02128e4 = 3: f95036c Add functionality for converting UNIX paths in arguments and environment variables to Windows form for native Win32 applications.
5: f5641c9 = 4: 10bdfe7 Add functionality for changing OS name via MSYSTEM environment variables.
6: 834af42 = 5: 8b9860b - Move root to /usr. - Change sorting mount points. - By default mount without ACLs. - Can read /etc/fstab with short mount point format.
7: a7a80da = 6: 242443b Instead of creating Cygwin symlinks, use deep copy by default
8: f671632 = 7: 9e313a6 Automatically rewrite TERM=msys to TERM=cygwin
9: 8f58dba = 8: 3f325c8 Do not convert environment for strace
10: 055c3f4 = 9: c657f7c strace.cc: Don't set MSYS=noglob
11: a836780 = 10: 1cd647f Add debugging for strace make_command_line
12: 16e335f = 11: 0dc024e strace --quiet: be really quiet
13: 0f97400 = 12: 322dd0f path_conv: special-case root directory to have trailing slash
14: 406cc50 = 13: 27d2935 When converting to a Unix path, avoid double trailing slashes
15: 665a09b = 14: 31e634e msys2_path_conv: pass PC_NOFULL to path_conv
16: b2f6faf = 15: 0d54d37 path-conversion: Introduce ability to switch off conversion.
17: 38378c5 = 16: 753f879 dcrt0.cc: Untangle allow_glob from winshell
18: b5312cb = 17: 6caceef dcrt0.cc (globify): Don't quote literal strings differently when dos_spec
19: 9ef5267 = 18: ab509c6 Add debugging for build_argv
20: b9d7099 = 19: 64d12ce environ.cc: New facility/environment variable MSYS2_ENV_CONV_EXCL
21: f43ffb1 = 20: 816c4c8 Introduce the
enable_pconvalue forMSYS22: 65cf9f0 = 21: 324c53b popen: call /usr/bin/sh instead of /bin/sh
23: 0866429 = 22: 8505c8a Disable the 'cygwin' GitHub workflow
24: b80641d = 23: 1653375 CI: add a GHA for doing a basic build test
25: d2a9e3c = 24: de1fcbc Set up a GitHub Action to keep in sync with Cygwin
26: 1e2b8bc = 25: 7e67f56 Expose full command-lines to other Win32 processes by default
27: 0e6a5b9 = 26: f7f1624 Add a helper to obtain a function's address in kernel32.dll
28: 803f234 = 27: 1453ede Emulate GenerateConsoleCtrlEvent() upon Ctrl+C
29: 4148d00 = 28: e30cdeb kill: kill Win32 processes more gently
30: c6208de = 29: 5733e91 Cygwin: make option for native inner link handling.
31: 1e06519 = 30: 82fd4e1 docs: skip building texinfo and PDF files
32: c325e7f = 31: f6b74e2 install-libs: depend on the "toollibs"
33: 4b304dc = 32: fe46171 POSIX-ify the SHELL variable
34: 129ee40 = 33: 8023549 Handle ORIGINAL_PATH just like PATH
35: 5f01a6f = 34: b03e6f9 uname: allow setting the system name to CYGWIN
36: 2897bc0 = 35: 5194fb9 Pass environment variables with empty values
37: 23846ef = 36: 13ef297 Optionally disallow empty environment values again
38: 24df6e4 = 37: cd4b539 build_env(): respect the
MSYSenvironment variable39: 6bb8c62 = 38: b0fe358 Revert "Cygwin: Enable dynamicbase on the Cygwin DLL by default"
40: 5670c93 = 39: fdb1b28 Avoid sharing cygheaps across Cygwin versions
41: 7b0c7ae = 40: 6378f63 uname: report msys2-runtime commit hash, too
42: e3e73ed = 41: 23a25d4 Cygwin: Adjust CWD magic to accommodate for the latest Windows previews
43: f23b83a ! 42: 5cd8625 Bump actions/checkout from 2 to 4
44: 2940558 = 43: 798bc25 dependabot: help keeping GitHub Actions versions up to date
45: efae895 = 44: 2c3031c Do not try to sync with Cygwin
46: 7cd580a = 45: 0db3752 Handle 8-bit characters under LOCALE=C
47: b2914cf = 46: cf305cf Mention the extremely useful small_printf() function
48: 6f2afa8 = 47: 12471a5 Make paths' WCS->MBS conversion explicit
53: c708f4a = 48: fa62a01 ci: run Git's entire test suite
49: 0944d19 = 49: 4a56ce2 Fixed path converting with non ascii char.
50: e5b7338 = 50: bf72b24 Allow native symlinks to non-existing targets in 'nativestrict' mode
51: 7e7c72b = 51: 473511f Use MB_CUR_MAX == 6 by default
52: 423c4c3 = 52: 42211d8 Change the default base address for x86_64
54: 09097de = 53: daa35cf msys2-runtime: restore fast path for current user primary group
55: 53c83cb = 54: b908190 ci: add an AutoHotKey-based integration test
56: 1f8def9 < -: ---------- Cygwin: Fix compatibility with w32api headers v13
57: 4ee8a03 < -: ---------- symlink_native: allow linking to
..58: cb9b2d0 < -: ---------- pipe: fix SSH hang (again)
59: 5e98c29 < -: ---------- fixup! pipe: fix SSH hang (again)
60: 0ae6a6f < -: ---------- Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader
61: 034c762 < -: ---------- Cygwin: pipe; Update source comment align with previous commit
62: 2f6e584 < -: ---------- Cygwin: pipe: Make pipe_data_available() return PDA_UNKNOWN
63: 7674c51 < -: ---------- Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread
64: ce17739 = 55: fa5d153 ci(ui-tests): use a better way to call AutoHotKey
65: cf7a164 = 56: c705ade ci(ui-tests): upload the test logs
66: 41d3ae0 = 57: 5fbae4f ci(ui-tests): take a screenshot when canceled
67: 8d098c5 = 58: f16b376 ci(ui-tests): enforce a sensible timeout
68: d287c6b = 59: 0bb4178 ui-tests: convert DOS line endings to Unix ones
69: 596d3dc = 60: d0bbbf0 ui-tests: ensure that
.ahkscripts are stored with Unix line endings70: 4e375cc = 61: 1e05e28 ui-tests: refactor setup into its own function
71: 9b6f402 = 62: 60476a6 ui-tests: rename
CaptureText()to reflect that it's about Windows Terminal72: e33eefe = 63: b0ec0a6 ui-tests: Move reusable functions into a library
73: fd9bd82 = 64: 7b76669 ui-tests: let SetWorkTree() accept a directory base name
74: ab61d99 = 65: 73feec7 ui-tests: create the Git worktree as part of SetWorkTree()
75: febbeef = 66: 9bbd0b6 ui-tests: document how
CaptureTextFromWindowsTerminal()works76: f58efbe = 67: 442014d ui-tests: move code to wait for text in the Windows Terminal into a function
77: cbb3b37 = 68: 6f81e28 ui-tests: fix indentation of
RunWaitOne()78: 3ae005b = 69: bef2469 ui-tests: improve
RunWaitOne()79: 0e5a76a = 70: 0b7a146 ui-tests: fix the
uname -ainvocation80: ef6a272 = 71: c291070 ui-tests: do not ignore errors in
RunWaitOne()81: 0ba85df = 72: c92fe4f ui-tests: verify that a
sleepin Windows Terminal can be interrupted82: 59f8289 = 73: 72721d0 ui-tests: trim the output in
RunWaitOne()83: 99f35a8 = 74: 2d053b2 ui-tests: prepare
WaitForRegExInWindowsTerminal()for disruptions84: 22ed451 = 75: a1fb347 ui-tests: verify that interrupting clones via SSH works
85: 0be3642 = 76: 46ff962 ci(ui-tests): exclude the large repository from the build artifact
86: 6673d8e < -: ---------- Cygwin: console: Call set_input_mode() after changing disable_master_thread
87: f6c4c7f = 77: f1e59e1 ui-tests: add
pinginterrupt test88: 772caa7 = 78: 05f0b0e ui-tests: do verify the SSH hang fix
The usual stuff: a couple of squashed-in fixes, and backports dropped because the rebase integrated the original commits.