jonas / tig

Text-mode interface for git
https://jonas.github.io/tig/
GNU General Public License v2.0
12.48k stars 611 forks source link

macOS search ('/') is not <Ctrl-C> cancellable #1347

Closed landonb closed 4 months ago

landonb commented 4 months ago

On macOS, when I start a search (using '/'), I cannot cancel out of it.

It seems like the readline call ignores SIGINT.

The only way I can return to the main view is to press <Enter> (and to run the search).

I do not experience this issue on Linux Mint MATE.

OS and tig versions

The problem affects both Brew tig and my own tig on macOS Sonoma 14.5:

  @macOS $ /opt/homebrew/Cellar/tig/2.5.10/bin/tig --version
  tig version 2.5.10
  ncursesw version 6.5.20240427
  readline version 8.2
  PCRE2 version 10.44 2024-06-07

Here's my own build, using the latest commit (and the latest readline and ncurses from Brew):

  @macOS $ tig --version
  tig version 2.5.10-4-g37ccb67e
  ncursesw version 6.5.20240427
  readline version 8.2
  PCRE2 version 10.44 2024-06-07

I do not see the problem on Linux Mint MATE ~21.4~ 21.3, also running the latest commit, albeit not the same readline (though it's the latest from apt):

  @linuxmint $ tig --version
  tig version 2.5.10
  ncursesw version 6.3.20211021
  readline version 8.1
  PCRE2 version 10.39 2021-10-29

(Limited) diagnosis

I am somewhat surprised that I didn't find an existing issue regarding this, but I looked.

I also haven't debugged C code in eons, and I'm not that familiar with readline sources, but I tracked it down to the prompt.c realine call

  line = readline(prompt);

Poking around some more, I fixed the problem by disabling these two lines:

    rl_deprep_term_function = NULL;
    rl_prep_term_function = NULL;

Those two lines were introduced a while back, 2019-05-02: Piping to tig segfaults on exit #893 / ea43f8be22c7

My solution (or the best I could come up with), is to exclude those two lines on macOS builds (#ifndef __APPLE__). Though I'm not saying this is the best solution, just that it appears to work for me.

I also glanced at the readline sources but didn't feel any more or less confident about the fix. There's also docs on the two functions in question:

Furthermore, there are (only) 69 code hits on GitHub for rl_prep_term_function = NULL;:

https://github.com/search?q=%22rl_prep_term_function+%3D+NULL%3B%22&type=code

though a lot of those hits are for the original readline.c, and for tig forks. A few other hits do reference disabling those functions to avoid conflicting with ncurses, but those sources I looked at also disable readline signals (rl_catch_signals = 0).

Test results

I tested the error mentioned in #893, and it works for me:

  echo | tig
  :q

I also ran make test on both my branch and the upstream branch, and I see the same results:

   SUMMARY  Failed 56 of 558 assertions in 145 tests (2 skipped)
   $ make test
   ...
   CASE  test/main/emoji-test:emoji-commit-titles-col-300
         | Failed 3 out of 6 test(s)
         | [FAIL] emoji-commit-titles-col-46.screen != expected/emoji-commit-titles-col-46.screen
         | diff --git a/expected/emoji-commit-titles-col-46.screen b/emoji-commit-titles-col-46.screen
         | index 4e7e712..af40a99 100644
         | --- a/expected/emoji-commit-titles-col-46.screen
         | +++ b/emoji-commit-titles-col-46.screen
         | @@ -1,4 +1,4 @@
         | -2009-04-06 01:44 +0000 Committer o [master] 🌏
         | +2009-04-06 01:44 +0000 Committer o [main] 🌏💧
  ...

Finally, while I only this afternoon made this branch, I'll continue to run it.

Given all that, I think my branch is an acceptable solution.

Thanks for reading! A little in-depth, but I wanted to do a thorough diagnosis.

landonb commented 4 months ago

Please see PR #1348

koutcher commented 4 months ago

Thanks for you detailed report. By checking the existing PRs you could have found #1342 was already dealing with this issue. Fixing the remaining annoyances you mentioned will require quite some work to switch to the alternate readline interface and for a somehow limited benefit.

landonb commented 4 months ago

Thanks for your kind response, @koutcher!

So embarrasing! 🤦 I could've sworn I had fetched recently, but somehow I missed the commit from June 20. (Edit: Though checking ~/.bash_history, I did fetch and merge, so perhaps it just hadn't been pushed to GH yet... in any case, I'll check PRs more thoroughly next time)

Thanks for all your hard work on this project, it contends with Vim as my favorite OSS project.

Edit: I tested the June 20 commit and <Ctrl-C> now works to quit search ('/') (Thank you, @intelfx!)

Completely unrelated:

I have an extensive ~/.config/tig/config, but I'm not sure if there's a proper place to highlight community configs, or if that's something that's been discussed... but here's mine: https://github.com/DepoXy/tig-newtons