Skip to content
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

feat: add interrupt (Ctrl-C) handling #550

Merged

Conversation

mrcljx
Copy link
Contributor

@mrcljx mrcljx commented Sep 18, 2023

Closes #542

⚡ Summary

Hitting Ctrl-C during commit would terminate lefthook immediately, and stashed changes would not be restored. This can be improved by using a NotifyContext which prevents lefthook from being killed while still terminating subprocesses.

Test Plan

  1. In this repo…
  2. Added to .lefthook.toml
    [pre-commit.commands.sleep]
    run = "sleep 10 && exit 1"
    
  3. Staged that change
  4. Added # unstaged at the end of .lefthook.toml
  5. Ran git commit -m 'test'
  6. Hit Ctrl-C
  7. Ran git status && git stash list
  8. Before:
    • Output: ⠏ waiting: sleep^C
    • The unstaged comment was missing
    • Found stash@{0}: lefthook auto backup
  9. After:
    • Output:
      ⠦ waiting: sleep^C
      
      EXECUTE > sleep
      
      Error: Interrupted
      
    • The unstaged comment was restored
    • Stash list was empty

☑️ Checklist

  • Check locally
  • Add tests
  • Add documentation (N/A)

@mrcljx mrcljx changed the title feat: add context for interrupt handling feat: add interrupt (Ctrl-C) handling Sep 18, 2023
@mrcljx mrcljx force-pushed the add-context-for-interrupt-handling branch 5 times, most recently from a15d691 to 7258254 Compare September 19, 2023 08:56
@mrcljx
Copy link
Contributor Author

mrcljx commented Sep 19, 2023

  • Was able to reproduce the previous issue and it's fixed.
  • The macOS integrity test should pass now (shouldn't have tested for SIGHUP)

@mrcljx mrcljx force-pushed the add-context-for-interrupt-handling branch from 7258254 to f3f6ef1 Compare September 19, 2023 14:38
@mrcljx
Copy link
Contributor Author

mrcljx commented Sep 19, 2023

Ah, well, celebrated too early. Pushed another fix (since bash is at /bin/bash for macOS):

- /usr/bin/bash
+ /usr/bin/env bash

Copy link
Member

@mrexox mrexox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this work! Going to release this change this week

@mrexox mrexox merged commit fcd39eb into evilmartians:master Sep 20, 2023
10 checks passed
@mrcljx mrcljx deleted the add-context-for-interrupt-handling branch September 20, 2023 11:34
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.

[feat] Clean up stash backups automatically, or add option to do it
2 participants