git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 0/4] builtin add -p: hopefully final readkey fixes

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Mar 16, 2022, 18:54 UTC
Message-ID
<20220316185405.29551-1-phillip.wood123@gmail.com>
In-Reply-To
<20220304131126.8293-1-phillip.wood123@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Thank to Junio for his comments on V3.
Changes since V3:
 * Patch 2
   - only close tty_fd if it is open and SAVE_TERM_STDIN was not
     requested
   - close tty_fd in save_term() if there is an error.
   - folded long line
 * Patch 4
   - add a comment explaining the purpose of write_err()

There's an updated conflict resolution after the range-diff at the end of this message.

V3 Cover Letter:
Thanks to Ævar, Carlo and Dscho for their comments on V2.
Changes since V2:
 * Patch 1
   - fix save_term() prototype (thanks to Carlo)
 * Patch 2
   - avoid closing tty_fd if it is stdin
 * Patch 3
   - removed redundant #include (thanks to Ævar)
   - handle select() timeout correctly
 * Patch 4
   - reset job signals if settcattr() fails

These patches are based on a merge of pw/single-key-interactive and master. There are a couple of simple textual conflicts with 'cb/save-term-across-editor-invocation' and unfortunately some semantic conflicts as well. A diff on top of the textual resolution which handles the semantic conflicts is given after the range-diff at the end of this message.

V2 Cover Letter:
Thanks to Ramsay and Ævar for their comments on V1.
Changes since V1:
 * Patch 1
   - use an enum for save_term()'s flags (suggested by Ævar)
   - fixed argument order in the windows code (thanks to Ramsay)
 * Patch 2
   - fixed a typo in a comment (thanks to Ramsay)
 * Patch 4
   - stopped duplicating the strings returned by gettext() (suggested by
     Ævar)
   - reworked error message handling in the signal handler to add an
     "error: " prefix (suggested by Ævar)
   - tweaked the background resume error message
V1 Cover Letter:

Fix the remaining issues that I'm aware of when using the built in "add -p" with interactive.singlekey that are stopping it from being merged to master. The first three patches make sure that we call tcsetattr() and the same file descriptor that we use for read() and work around poll() being broken when reading from terminals on macos. The final patch is more of an improvement rather than a bug fix (the same issue already exists in the perl version) and could proceed separately.

Unfortunately these patches conflict with 'cb/save-term-across-editor-invocation' as well as the textual conflicts there is a semantic conflict as the argument to save_term() is changed so the code in editor.c will need updating.

These patches are based on 'pw/single-key-interactive'
Phillip Wood (4):
  terminal: use flags for save_term()
  terminal: don't assume stdin is /dev/tty
  terminal: work around macos poll() bug
  terminal: restore settings on SIGTSTP
 compat/terminal.c | 238 +++++++++++++++++++++++++++++++++++++++-------
 compat/terminal.h |   9 +-
 2 files changed, 211 insertions(+), 36 deletions(-)
Range-diff against v3:
-:  ---------- > 1:  2c4c5424d7 terminal: use flags for save_term()
1:  cf9275c40c ! 2:  405a69852f terminal: don't assume stdin is /dev/tty
    @@ Commit message
     
      ## compat/terminal.c ##
     @@ compat/terminal.c: static void restore_term_on_signal(int sig)
    + #define INPUT_PATH "/dev/tty"
    + #define OUTPUT_PATH "/dev/tty"
    + 
    ++static volatile sig_atomic_t term_fd_needs_closing;
      static int term_fd = -1;
      static struct termios old_term;
      
     +static void close_term_fd(void)
     +{
    -+	if (term_fd)
    ++	if (term_fd_needs_closing)
     +		close(term_fd);
    ++	term_fd_needs_closing = 0;
     +	term_fd = -1;
     +}
     +
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
      {
      	if (term_fd < 0)
     -		term_fd = open("/dev/tty", O_RDWR);
    -+		term_fd = (flags & SAVE_TERM_STDIN) ? 0
    -+						    : open("/dev/tty", O_RDWR);
    ++		term_fd = ((flags & SAVE_TERM_STDIN)
    ++			   ? 0
    ++			   : open("/dev/tty", O_RDWR));
      	if (term_fd < 0)
      		return -1;
    - 	if (tcgetattr(term_fd, &old_term) < 0)
    +-	if (tcgetattr(term_fd, &old_term) < 0)
    ++	term_fd_needs_closing = !(flags & SAVE_TERM_STDIN);
    ++	if (tcgetattr(term_fd, &old_term) < 0) {
    ++		close_term_fd();
    + 		return -1;
    ++	}
    + 	sigchain_push_common(restore_term_on_signal);
    + 
    + 	return 0;
     @@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)
    + 	struct termios t;
    + 
    + 	if (save_term(flags) < 0)
    +-		goto error;
    ++		return -1;
    + 
    + 	t = old_term;
    + 
    +@@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)
    + 		return 0;
      
      	sigchain_pop_common();
    - error:
    +-error:
     -	close(term_fd);
     -	term_fd = -1;
     +	close_term_fd();
2:  4828d63ce5 = 3:  70bc6e9c06 terminal: work around macos poll() bug
3:  3185bc5223 ! 4:  f38d162f75 terminal: restore settings on SIGTSTP
    @@ compat/terminal.c
      #include "compat/terminal.h"
      #include "sigchain.h"
      #include "strbuf.h"
    -@@ compat/terminal.c: static void restore_term_on_signal(int sig)
    +@@ compat/terminal.c: static volatile sig_atomic_t term_fd_needs_closing;
      static int term_fd = -1;
      static struct termios old_term;
      
     +static const char *background_resume_msg;
     +static const char *restore_error_msg;
     +static volatile sig_atomic_t ttou_received;
     +
    ++/* async safe error function for use by signal handlers. */
     +static void write_err(const char *msg)
     +{
     +	write_in_full(2, "error: ", strlen("error: "));
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
     +
      static void close_term_fd(void)
      {
    - 	if (term_fd)
    + 	if (term_fd_needs_closing)
     @@ compat/terminal.c: void restore_term(void)
      	tcsetattr(term_fd, TCSAFLUSH, &old_term);
      	close_term_fd();
    @@ compat/terminal.c: void restore_term(void)
     +	struct sigaction sa;
     +
      	if (term_fd < 0)
    - 		term_fd = (flags & SAVE_TERM_STDIN) ? 0
    - 						    : open("/dev/tty", O_RDWR);
    + 		term_fd = ((flags & SAVE_TERM_STDIN)
    + 			   ? 0
     @@ compat/terminal.c: int save_term(enum save_term_flags flags)
    - 	if (tcgetattr(term_fd, &old_term) < 0)
      		return -1;
    + 	}
      	sigchain_push_common(restore_term_on_signal);
     +	/*
     +	 * If job control is disabled then the shell will have set the
    @@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t
      
      	sigchain_pop_common();
     +	reset_job_signals();
    - error:
      	close_term_fd();
      	return -1;
    + }
     @@ compat/terminal.c: static int getchar_with_timeout(int timeout)
      	fd_set readfds;
      	int res;

Conflict Resolution: diff --cc compat/terminal.c index 7db330c52d,8743df6c9a..1bfa92584a

--- a/compat/terminal.c
+++ b/compat/terminal.c
@@@ -134,47 -29,31 +134,56 @@@ void restore_term(void
  		return;

  	tcsetattr(term_fd, TCSAFLUSH, &old_term);
 -
 -	close(term_fd);
 -	term_fd = -1;
 +	close_term_fd();
  	sigchain_pop_common();
 +	reset_job_signals();
  }

+ static int is_controlling_terminal(int fd)
+ {
+ 	return (getpgid(0) == tcgetpgrp(fd));
+ }
+
 -int save_term(int full_duplex)
 +int save_term(enum save_term_flags flags)
  {
 +	struct sigaction sa;
 +
  	if (term_fd < 0)
 -		term_fd = open("/dev/tty", O_RDWR);
 +		term_fd = ((flags & SAVE_TERM_STDIN)
 +			   ? 0
 +			   : open("/dev/tty", O_RDWR));
  	if (term_fd < 0)
  		return -1;
 -	if (full_duplex && !is_controlling_terminal(term_fd)) {
 -		close(term_fd);
 -		term_fd = -1;
 +	term_fd_needs_closing = !(flags & SAVE_TERM_STDIN);
++	if ((flags & SAVE_TERM_DUPLEX) && !is_controlling_terminal(term_fd)) {
++		close_term_fd();
+ 		return -1;
+ 	}
 -	if (tcgetattr(term_fd, &old_term) < 0)
 +	if (tcgetattr(term_fd, &old_term) < 0) {
 +		close_term_fd();
  		return -1;
 +	}
  	sigchain_push_common(restore_term_on_signal);
 +	/*
 +	 * If job control is disabled then the shell will have set the
 +	 * disposition of SIGTSTP to SIG_IGN.
 +	 */
 +	sigaction(SIGTSTP, NULL, &sa);
 +	if (sa.sa_handler == SIG_IGN)
 +		return 0;
 +
 +	/* avoid calling gettext() from signal handler */
 +	background_resume_msg = _("cannot resume in the background, please use 'fg' to resume");
 +	restore_error_msg = _("cannot restore terminal settings");
 +	sa.sa_handler = restore_terminal_on_suspend;
 +	sa.sa_flags = SA_RESTART;
 +	sigemptyset(&sa.sa_mask);
 +	sigaddset(&sa.sa_mask, SIGTSTP);
 +	sigaddset(&sa.sa_mask, SIGTTIN);
 +	sigaddset(&sa.sa_mask, SIGTTOU);
 +	sigaction(SIGTSTP, &sa, NULL);
 +	sigaction(SIGTTIN, &sa, NULL);
 +	sigaction(SIGTTOU, &sa, NULL);

  	return 0;
  }
diff --cc editor.c
index 8b9648281d,6c5c95e6a2..192d6ea75d
--- a/editor.c
+++ b/editor.c
@@@ -48,6 -49,16 +49,16 @@@ const char *git_sequence_editor(void
  	return editor;
  }

+ static int prepare_term(const char *editor)
+ {
+ 	int need_saverestore = !strcmp(editor, DEFAULT_EDITOR);
+
+ 	git_config_get_bool("editor.stty", &need_saverestore);
+ 	if (need_saverestore)
 -		return save_term(1);
++		return save_term(SAVE_TERM_DUPLEX);
+ 	return 0;
+ }
+
  static int launch_specified_editor(const char *editor, const char *path,
  				   struct strbuf *buffer, const char *const *env)
  {

-- 
2.35.1
Previous: Phillip WoodNext: Phillip Wood
Message 49 of 53 in “builtin add -p: hopefully final readkey fixes”
  1. 0/4 builtin add -p: hopefully final readkey fixesPhillip Wood, Mar 4, 2022
  2. 1/4 terminal: use flags for save_term()Phillip Wood, Mar 4, 2022
  3. Ramsay JonesMar 4, 2022
  4. Phillip WoodMar 7, 2022
  5. Ramsay JonesMar 7, 2022
  6. Phillip WoodMar 8, 2022
  7. Ævar Arnfjörð BjarmasonMar 5, 2022
  8. Phillip WoodMar 7, 2022
  9. Ævar Arnfjörð BjarmasonMar 7, 2022
  10. 3/4 terminal: work around macos poll() bugPhillip Wood, Mar 4, 2022
  11. 2/4 terminal: don't assume stdin is /dev/ttyPhillip Wood, Mar 4, 2022
  12. Ramsay JonesMar 4, 2022
  13. 4/4 terminal: restore settings on SIGTSTPPhillip Wood, Mar 4, 2022
  14. Ævar Arnfjörð BjarmasonMar 5, 2022
  15. Phillip WoodMar 7, 2022
  16. Ævar Arnfjörð BjarmasonMar 7, 2022
  17. Phillip WoodMar 7, 2022
  18. Ævar Arnfjörð BjarmasonMar 7, 2022
  19. Phillip WoodMar 8, 2022
  20. Johannes SchindelinMar 9, 2022
  21. Phillip WoodMar 10, 2022
  22. 0/4 builtin add -p: hopefully final readkey fixesPhillip Wood, Mar 9, 2022
  23. 1/4 terminal: use flags for save_term()Phillip Wood, Mar 9, 2022
  24. Carlo ArenasMar 11, 2022
  25. Phillip WoodMar 14, 2022
  26. 2/4 terminal: don't assume stdin is /dev/ttyPhillip Wood, Mar 9, 2022
  27. 3/4 terminal: work around macos poll() bugPhillip Wood, Mar 9, 2022
  28. Ævar Arnfjörð BjarmasonMar 10, 2022
  29. Phillip WoodMar 10, 2022
  30. Junio C HamanoMar 10, 2022
  31. 4/4 terminal: restore settings on SIGTSTPPhillip Wood, Mar 9, 2022
  32. Junio C HamanoMar 9, 2022
  33. Junio C HamanoMar 9, 2022
  34. Phillip WoodMar 10, 2022
  35. Phillip WoodMar 10, 2022
  36. Junio C HamanoMar 10, 2022
  37. Johannes SchindelinMar 10, 2022
  38. Phillip WoodMar 10, 2022
  39. 0/4 builtin add -p: hopefully final readkey fixesPhillip Wood, Mar 15, 2022
  40. 2/4 terminal: don't assume stdin is /dev/ttyPhillip Wood, Mar 15, 2022
  41. Junio C HamanoMar 15, 2022
  42. rsbecker@nexbridge.comMar 15, 2022
  43. Junio C HamanoMar 15, 2022
  44. rsbecker@nexbridge.comMar 15, 2022
  45. 1/4 terminal: use flags for save_term()Phillip Wood, Mar 15, 2022
  46. 4/4 terminal: restore settings on SIGTSTPPhillip Wood, Mar 15, 2022
  47. Junio C HamanoMar 15, 2022
  48. 3/4 terminal: work around macos poll() bugPhillip Wood, Mar 15, 2022
  49. 0/4 builtin add -p: hopefully final readkey fixesPhillip Wood, Mar 16, 2022
  50. 1/4 terminal: use flags for save_term()Phillip Wood, Mar 16, 2022
  51. 2/4 terminal: don't assume stdin is /dev/ttyPhillip Wood, Mar 16, 2022
  52. 3/4 terminal: work around macos poll() bugPhillip Wood, Mar 16, 2022
  53. 4/4 terminal: restore settings on SIGTSTPPhillip Wood, Mar 16, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.