Re-fix the Git hooks problem #95
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
Addresses: https://sourceware.org/pipermail/cygwin-patches/2025q2/013644.html Fixes: 3e8a7eb ("sys/unistd.h: fix definition of setproctitle_init") Reported-by: Brian Inglis <[email protected]> Co-authored-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 1c530c3)
Call msys2-tests composite workflows to run tests using built artifacts. Signed-off-by: Jeremy Drake <[email protected]>
... and restore it when app exits. The commit 0bfd91d has a bug that the console mode is stored into the shared memory when both: (1) cygwin process is started from non-cygwin process. (2) cygwin process started from non-cygwin process exits. (1) is intended, but (2) is not. Due to (2), the stored console mode is unexpectedly broken when the cygwin process exits. Then the mode restored will be not as expected. This causes undesired console mode in the use case that cygwin and non-cygwin apps are mixed. With this patch, the console mode will stored only in the case (1). This is done by putting the code, which stores the console mode, into fhandler_console::open() rather than fhandler_console::set_input_mode() and fhandler_console::set_output_mode(). Fixes: 0bfd91d ("Cygwin: console: tty::restore really restores the previous mode") Reported-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 09ae9f6)
This pulls in the current tip of the `msys2-3.6.1` branch, which includes a compile fix and a substantial enhancement of the CI builds by running MSYS2's tests on the built MSYS2 runtime. Signed-off-by: Johannes Schindelin <[email protected]>
The report in microsoft/git#730 describes a bug where running a background task in a Git hook was clashing with the MSYS2 runtime's console mode wrangling. Incidentally, the same fix addressed the bug git-for-windows/git#4776 reported: Running `git add -p` in Notepad++ (or VS Code), after `e`diting a hunk, the console gets stuck. Sadly, this bug fix caused a regression that was reported in msys2/msys2-runtime#268. Even more sadly, the fix for _that_ regression re-broke the Git hook issue described above. I reported this issue to Cygwin (via IRC, sorry, no public record), and they came up with this fix. 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. Signed-off-by: Johannes Schindelin <[email protected]>
derrickstolee
approved these changes
May 7, 2025
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
I'm not super-familiar with this .ahk filetype, but it looks like a robust test. As long as it failed without this fix, I'm super happy to have this improvement.
Member
Author
|
Thank you for the review @derrickstolee!
To be transparent, until yesterday I wasn't very familiar with it, either ;-)
Yes, it does. To prove it, I added two commits on top, one to reuse the build artifact (no need to build the MSYS2 runtime again) and another commit to use the current MSYS2 runtime from the MSYS2 project (which is equally broken), and the result (i.e. the expected failure) can be seen here: https://github.com/git-for-windows/msys2-runtime/actions/runs/14888904465/job/41815792740 |
Member
Author
|
/open pr The workflow run was started |
dscho
added a commit
to git-for-windows/MSYS2-packages
that referenced
this pull request
May 7, 2025
In git-for-windows/msys2-runtime#95, I introduced a new UI-based integration test. We do not need that to build the package. The test is meant to be run in the `msys2-runtime` repository's PRs _before_ the patches make it into `MSYS2-packages`. Therefore it does not make sense to carry it here. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
to dscho/msys2-runtime
that referenced
this pull request
May 26, 2025
…blem Re-fix the Git hooks problem
dscho
added a commit
to dscho/msys2-runtime
that referenced
this pull request
Jun 5, 2025
…blem Re-fix the Git hooks problem
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This re-fixes microsoft/git#730.