Skip to content

Fixes #1083 in main git repository. #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

doronbehar
Copy link

@doronbehar doronbehar commented Sep 2, 2017

Issue
Notice: I haven't tested it because I don't have a windows machine to do so.
Please tell me if it works.

* 'master' of ssh://github.com/git-for-windows/build-extra: (172 commits)
  please.sh test_remote_branch: handle submodule gracefully
  Mention Bug Fix in release notes
  ReleaseNotes: keep only latest BusyBox update note
  Mention New Feature in release notes
  Mention Bug Fix in release notes
  Mention New Feature in release notes
  installer: make sure that the hardlinked .dll files are up-to-date
  Mention Bug Fix in release notes
  Mention New Feature in release notes
  git-extra: new version
  git-extra: adjust checksums
  git-extra (git-update): warn if more than one Bash is killed
  git-extra (git-update): really return early
  git-extra (git-update): clarify why `ps` is safe
  please.sh rebase: allow --jump again ;-)
  Mention New Feature in release notes
  installer: fix 'Unknown identified'
  Mention Bug Fix in release notes
  installer: override previous default of symlink option
  installer: refactor IsDowngrade() function
  ...
If it's empty it means Vim was launched from a git process, therefor we
need to disable `<C-z>` which might cause lose of our work during
editing the files (could be a `difftool` or `mergetool`).
@dscho
Copy link
Member

dscho commented Sep 6, 2017

Thanks for your contribution.

I had a look and I think it would do what you want it to do: disable Ctrl+Z handling when vim was most likely spawned from git.exe.

However, I think that a more thorough approach might be to patch the MSYS2 runtime to refuse kill() with the SIGSUSP or SIGCONT signal unless the target process is an MSYS2 process.

The rationale: MSYS2 emulates the POSIX behavior as long as it is in control, and with pure-Win32 processes, it is not in control, not really. For extra bonus points, the MSYS2 runtime would then also log a warning "not signaling pid : Win32 process" or something similar.

The advantage of that approach is that it would not prevent MSYS2 child processes of vim to be handled via Ctrl+Z, and it would handle more than just git.exe child processes spawned from within vim.exe: it would skip job control of any Win32 child process spawned from any MSYS2 process.

It is pretty easy to find the code location which would need to be patched for this, as Git for Windows' MSYS2 runtime fork already has some special code in the MSYS2 runtime to emulate kill for Win32 processes: git-for-windows/msys2-runtime@01a0bbf. The patch in question will most likely boil down to making the exit_process() call conditional upon si.si_signo != SIGSUSP && si.si_signo != SIGCONT (possibly a larger list), otherwise call system_printf("Skip signalling win32 process (pid %u)", (unsigned int)GetProcessId(ch_spawn));.

Ths would need to be tested as described in https://github.com/git-for-windows/git/wiki/Building-msys2-runtime#rebuild-the-msys2-runtime.

@doronbehar
Copy link
Author

Well it's exactly the kind of PR I would have loved to contribute to mysys2. Unfortunately, it's beyond my league. Besides, I don't even use MS Windows, the reason I created this PR, as I explained in #1053 in git-for-windows/git was because I once was forced to use git for windows when I helped some friends and I lost 2-3 hours of work with mergetool. I just wanted to create just a temporally solution so users won't be lost like I was.

dscho added a commit to git-for-windows/msys2-runtime that referenced this pull request Sep 7, 2017
When calling Win32 processes from MSYS2, it does not really make sense
to kill them when a user tries to suspend them. It is much better to
warn the user that Win32 processes simply cannot be suspended.

This closes git-for-windows/git#1083 and
replaces git-for-windows/build-extra#154

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member

dscho commented Sep 7, 2017

@doronbehar thank you again for your contribution, even if the end result looks quite a bit different from your initial Pull Request. I hope you are as happy with the outcome as I am?

@dscho dscho closed this Sep 7, 2017
@doronbehar doronbehar deleted the fix-git-issue-1083 branch September 7, 2017 16:27
@doronbehar
Copy link
Author

It's so nice of you to ask :-]
To tell the truth, I'm much more happy with your fix in 65e334c than I would have been with my PR. Your fix changes the core behavior of these programs therefor it's a much better solution.

The point of a contribution shouldn't be just so our names will be marked on a peace of software. A contribution should be made just so it will make the software better and ease the lives of it's community. Without necessarily getting credit for it. That's because ideas don't have owners.

That's the point of free software IMO :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants