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

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

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 | 205 +++++++++++++++++++++++++++++++++++++++-------
 compat/terminal.h |   6 +-
 2 files changed, 182 insertions(+), 29 deletions(-)

-- 
2.35.1

[PATCH 1/4] terminal: use flags for save_term()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The next commit will add another flag in addition to the existing
full_duplex so change the function signature to take an unsigned flags
argument. Also alter the functions that call save_term() so that they
can pass flags down to it.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 41 +++++++++++++++++++++--------------------
 compat/terminal.h |  5 ++++-
 2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index d882dfa06e..bad8e04cd8 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -34,7 +34,7 @@ void restore_term(void)
 	sigchain_pop_common();
 }
 
-int save_term(int full_duplex)
+int save_term(unsigned flags)
 {
 	if (term_fd < 0)
 		term_fd = open("/dev/tty", O_RDWR);
@@ -47,11 +47,11 @@ int save_term(int full_duplex)
 	return 0;
 }
 
-static int disable_bits(tcflag_t bits)
+static int disable_bits(unsigned flags, tcflag_t bits)
 {
 	struct termios t;
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		goto error;
 
 	t = old_term;
@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)
 	return -1;
 }
 
-static int disable_echo(void)
+static int disable_echo(unsigned flags)
 {
-	return disable_bits(ECHO);
+	return disable_bits(flags, ECHO);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(unsigned flags)
 {
-	return disable_bits(ICANON | ECHO);
+	return disable_bits(flags, ICANON | ECHO);
 }
 
 #elif defined(GIT_WINDOWS_NATIVE)
@@ -126,15 +126,15 @@ void restore_term(void)
 	hconin = hconout = INVALID_HANDLE_VALUE;
 }
 
-int save_term(int full_duplex)
+int save_term(unsigned flags)
 {
 	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
 	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
 	    FILE_ATTRIBUTE_NORMAL, NULL);
 	if (hconin == INVALID_HANDLE_VALUE)
 		return -1;
 
-	if (full_duplex) {
+	if (flags & SAVE_TERM_DUPLEX) {
 		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
 			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
 			FILE_ATTRIBUTE_NORMAL, NULL);
@@ -154,7 +154,7 @@ int save_term(int full_duplex)
 	return -1;
 }
 
-static int disable_bits(DWORD bits)
+static int disable_bits(unsigned flags, DWORD bits)
 {
 	if (use_stty) {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)
 		use_stty = 0;
 	}
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		return -1;
 
 	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)
 	return 0;
 }
 
-static int disable_echo(void)
+static int disable_echo(unsigned flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT);
+	return disable_bits(ENABLE_ECHO_INPUT, flags);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(unsigned flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
+	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,
+			    flags);
 }
 
 /*
@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
 		return NULL;
 	}
 
-	if (!echo && disable_echo()) {
+	if (!echo && disable_echo(0)) {
 		fclose(input_fh);
 		fclose(output_fh);
 		return NULL;
@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical() < 0) {
+	if (warning_displayed || enable_non_canonical(0) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)
 
 #else
 
-int save_term(int full_duplex)
+int save_term(unsigned flags)
 {
-	/* full_duplex == 1, but no support available */
-	return -full_duplex;
+	/* no duplex support available */
+	return -!!(flags & SAVE_TERM_DUPLEX);
 }
 
 void restore_term(void)
diff --git a/compat/terminal.h b/compat/terminal.h
index 0fb9fa147c..f24b91390d 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,14 +1,17 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+/* Save input and output settings */
+#define SAVE_TERM_DUPLEX (1u << 0)
+
 /*
  * Save the terminal attributes so they can be restored later by a
  * call to restore_term(). Note that every successful call to
  * save_term() must be matched by a call to restore_term() even if the
  * attributes have not been changed. Returns 0 on success, -1 on
  * failure.
  */
-int save_term(int full_duplex);
+int save_term(unsigned flags);
 /* Restore the terminal attributes that were saved with save_term() */
 void restore_term(void);
 
-- 
2.35.1

Re: [PATCH 1/4] terminal: use flags for save_term()

[Ramsay Jones]

Hi Phillip,

I have not studied/applied your patches, they are just floating
past my inbox, so please ignore me if I have misunderstood ...

On 04/03/2022 13:11, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> The next commit will add another flag in addition to the existing
> full_duplex so change the function signature to take an unsigned flags
> argument. Also alter the functions that call save_term() so that they
> can pass flags down to it.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  compat/terminal.c | 41 +++++++++++++++++++++--------------------
>  compat/terminal.h |  5 ++++-
>  2 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/compat/terminal.c b/compat/terminal.c
> index d882dfa06e..bad8e04cd8 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -34,7 +34,7 @@ void restore_term(void)
>  	sigchain_pop_common();
>  }
>  
> -int save_term(int full_duplex)
> +int save_term(unsigned flags)
>  {
>  	if (term_fd < 0)
>  		term_fd = open("/dev/tty", O_RDWR);
> @@ -47,11 +47,11 @@ int save_term(int full_duplex)
>  	return 0;
>  }
>  
> -static int disable_bits(tcflag_t bits)
> +static int disable_bits(unsigned flags, tcflag_t bits)

.. you add the 'flags' as the new first parameter ...

>  {
>  	struct termios t;
>  
> -	if (save_term(0) < 0)
> +	if (save_term(flags) < 0)
>  		goto error;
>  
>  	t = old_term;
> @@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)
>  	return -1;
>  }
>  
> -static int disable_echo(void)
> +static int disable_echo(unsigned flags)
>  {
> -	return disable_bits(ECHO);
> +	return disable_bits(flags, ECHO);

.. and pass it as first parameter, good, and ...

>  }
>  
> -static int enable_non_canonical(void)
> +static int enable_non_canonical(unsigned flags)
>  {
> -	return disable_bits(ICANON | ECHO);
> +	return disable_bits(flags, ICANON | ECHO);

.. here as well, good, and ...

>  }
>  
>  #elif defined(GIT_WINDOWS_NATIVE)
> @@ -126,15 +126,15 @@ void restore_term(void)
>  	hconin = hconout = INVALID_HANDLE_VALUE;
>  }
>  
> -int save_term(int full_duplex)
> +int save_term(unsigned flags)
>  {
>  	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
>  	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
>  	    FILE_ATTRIBUTE_NORMAL, NULL);
>  	if (hconin == INVALID_HANDLE_VALUE)
>  		return -1;
>  
> -	if (full_duplex) {
> +	if (flags & SAVE_TERM_DUPLEX) {
>  		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
>  			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
>  			FILE_ATTRIBUTE_NORMAL, NULL);
> @@ -154,7 +154,7 @@ int save_term(int full_duplex)
>  	return -1;
>  }
>  
> -static int disable_bits(DWORD bits)
> +static int disable_bits(unsigned flags, DWORD bits)

.. Huh? Ah, the DWORD suggests this is in an #ifdef'd windows
part of the file, OK. ...

>  {
>  	if (use_stty) {
>  		struct child_process cp = CHILD_PROCESS_INIT;
> @@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)
>  		use_stty = 0;
>  	}
>  
> -	if (save_term(0) < 0)
> +	if (save_term(flags) < 0)
>  		return -1;
>  
>  	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
> @@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)
>  	return 0;
>  }
>  
> -static int disable_echo(void)
> +static int disable_echo(unsigned flags)
>  {
> -	return disable_bits(ENABLE_ECHO_INPUT);
> +	return disable_bits(ENABLE_ECHO_INPUT, flags);

.. but here, you pass the flags as the second parameter. ;-)

>  }
>  
> -static int enable_non_canonical(void)
> +static int enable_non_canonical(unsigned flags)
>  {
> -	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
> +	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,
> +			    flags);

.. ditto here.

ATB,
Ramsay Jones


>  }
>  
>  /*
> @@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
>  		return NULL;
>  	}
>  
> -	if (!echo && disable_echo()) {
> +	if (!echo && disable_echo(0)) {
>  		fclose(input_fh);
>  		fclose(output_fh);
>  		return NULL;
> @@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)
>  	static int warning_displayed;
>  	int ch;
>  
> -	if (warning_displayed || enable_non_canonical() < 0) {
> +	if (warning_displayed || enable_non_canonical(0) < 0) {
>  		if (!warning_displayed) {
>  			warning("reading single keystrokes not supported on "
>  				"this platform; reading line instead");
> @@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)
>  
>  #else
>  
> -int save_term(int full_duplex)
> +int save_term(unsigned flags)
>  {
> -	/* full_duplex == 1, but no support available */
> -	return -full_duplex;
> +	/* no duplex support available */
> +	return -!!(flags & SAVE_TERM_DUPLEX);
>  }
>  
>  void restore_term(void)
> diff --git a/compat/terminal.h b/compat/terminal.h
> index 0fb9fa147c..f24b91390d 100644
> --- a/compat/terminal.h
> +++ b/compat/terminal.h
> @@ -1,14 +1,17 @@
>  #ifndef COMPAT_TERMINAL_H
>  #define COMPAT_TERMINAL_H
>  
> +/* Save input and output settings */
> +#define SAVE_TERM_DUPLEX (1u << 0)
> +
>  /*
>   * Save the terminal attributes so they can be restored later by a
>   * call to restore_term(). Note that every successful call to
>   * save_term() must be matched by a call to restore_term() even if the
>   * attributes have not been changed. Returns 0 on success, -1 on
>   * failure.
>   */
> -int save_term(int full_duplex);
> +int save_term(unsigned flags);
>  /* Restore the terminal attributes that were saved with save_term() */
>  void restore_term(void);
>  

Re: [PATCH 1/4] terminal: use flags for save_term()

[Phillip Wood]

Hi Ramsay

On 04/03/2022 20:40, Ramsay Jones wrote:
> Hi Phillip,
> 
> I have not studied/applied your patches, they are just floating
> past my inbox, so please ignore me if I have misunderstood ...
> 
> On 04/03/2022 13:11, Phillip Wood wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The next commit will add another flag in addition to the existing
>> full_duplex so change the function signature to take an unsigned flags
>> argument. Also alter the functions that call save_term() so that they
>> can pass flags down to it.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   compat/terminal.c | 41 +++++++++++++++++++++--------------------
>>   compat/terminal.h |  5 ++++-
>>   2 files changed, 25 insertions(+), 21 deletions(-)
>>
>> diff --git a/compat/terminal.c b/compat/terminal.c
>> index d882dfa06e..bad8e04cd8 100644
>> --- a/compat/terminal.c
>> +++ b/compat/terminal.c
>> @@ -34,7 +34,7 @@ void restore_term(void)
>>   	sigchain_pop_common();
>>   }
>>   
>> -int save_term(int full_duplex)
>> +int save_term(unsigned flags)
>>   {
>>   	if (term_fd < 0)
>>   		term_fd = open("/dev/tty", O_RDWR);
>> @@ -47,11 +47,11 @@ int save_term(int full_duplex)
>>   	return 0;
>>   }
>>   
>> -static int disable_bits(tcflag_t bits)
>> +static int disable_bits(unsigned flags, tcflag_t bits)
> 
> .. you add the 'flags' as the new first parameter ...
> 
>>   {
>>   	struct termios t;
>>   
>> -	if (save_term(0) < 0)
>> +	if (save_term(flags) < 0)
>>   		goto error;
>>   
>>   	t = old_term;
>> @@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)
>>   	return -1;
>>   }
>>   
>> -static int disable_echo(void)
>> +static int disable_echo(unsigned flags)
>>   {
>> -	return disable_bits(ECHO);
>> +	return disable_bits(flags, ECHO);
> 
> .. and pass it as first parameter, good, and ...
> 
>>   }
>>   
>> -static int enable_non_canonical(void)
>> +static int enable_non_canonical(unsigned flags)
>>   {
>> -	return disable_bits(ICANON | ECHO);
>> +	return disable_bits(flags, ICANON | ECHO);
> 
> .. here as well, good, and ...
> 
>>   }
>>   
>>   #elif defined(GIT_WINDOWS_NATIVE)
>> @@ -126,15 +126,15 @@ void restore_term(void)
>>   	hconin = hconout = INVALID_HANDLE_VALUE;
>>   }
>>   
>> -int save_term(int full_duplex)
>> +int save_term(unsigned flags)
>>   {
>>   	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
>>   	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
>>   	    FILE_ATTRIBUTE_NORMAL, NULL);
>>   	if (hconin == INVALID_HANDLE_VALUE)
>>   		return -1;
>>   
>> -	if (full_duplex) {
>> +	if (flags & SAVE_TERM_DUPLEX) {
>>   		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
>>   			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
>>   			FILE_ATTRIBUTE_NORMAL, NULL);
>> @@ -154,7 +154,7 @@ int save_term(int full_duplex)
>>   	return -1;
>>   }
>>   
>> -static int disable_bits(DWORD bits)
>> +static int disable_bits(unsigned flags, DWORD bits)
> 
> .. Huh? Ah, the DWORD suggests this is in an #ifdef'd windows
> part of the file, OK. ...
> 
>>   {
>>   	if (use_stty) {
>>   		struct child_process cp = CHILD_PROCESS_INIT;
>> @@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)
>>   		use_stty = 0;
>>   	}
>>   
>> -	if (save_term(0) < 0)
>> +	if (save_term(flags) < 0)
>>   		return -1;
>>   
>>   	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
>> @@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)
>>   	return 0;
>>   }
>>   
>> -static int disable_echo(void)
>> +static int disable_echo(unsigned flags)
>>   {
>> -	return disable_bits(ENABLE_ECHO_INPUT);
>> +	return disable_bits(ENABLE_ECHO_INPUT, flags);
> 
> .. but here, you pass the flags as the second parameter. ;-)

Oh dear that's embarrassing, thanks for your careful review.

Are patches 3 & 4 OK for non-stop platforms?

Best Wishes

Phillip

>>   }
>>   
>> -static int enable_non_canonical(void)
>> +static int enable_non_canonical(unsigned flags)
>>   {
>> -	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
>> +	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,
>> +			    flags);
> 
> .. ditto here.
> 
> ATB,
> Ramsay Jones
> 
> 
>>   }
>>   
>>   /*
>> @@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
>>   		return NULL;
>>   	}
>>   
>> -	if (!echo && disable_echo()) {
>> +	if (!echo && disable_echo(0)) {
>>   		fclose(input_fh);
>>   		fclose(output_fh);
>>   		return NULL;
>> @@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)
>>   	static int warning_displayed;
>>   	int ch;
>>   
>> -	if (warning_displayed || enable_non_canonical() < 0) {
>> +	if (warning_displayed || enable_non_canonical(0) < 0) {
>>   		if (!warning_displayed) {
>>   			warning("reading single keystrokes not supported on "
>>   				"this platform; reading line instead");
>> @@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)
>>   
>>   #else
>>   
>> -int save_term(int full_duplex)
>> +int save_term(unsigned flags)
>>   {
>> -	/* full_duplex == 1, but no support available */
>> -	return -full_duplex;
>> +	/* no duplex support available */
>> +	return -!!(flags & SAVE_TERM_DUPLEX);
>>   }
>>   
>>   void restore_term(void)
>> diff --git a/compat/terminal.h b/compat/terminal.h
>> index 0fb9fa147c..f24b91390d 100644
>> --- a/compat/terminal.h
>> +++ b/compat/terminal.h
>> @@ -1,14 +1,17 @@
>>   #ifndef COMPAT_TERMINAL_H
>>   #define COMPAT_TERMINAL_H
>>   
>> +/* Save input and output settings */
>> +#define SAVE_TERM_DUPLEX (1u << 0)
>> +
>>   /*
>>    * Save the terminal attributes so they can be restored later by a
>>    * call to restore_term(). Note that every successful call to
>>    * save_term() must be matched by a call to restore_term() even if the
>>    * attributes have not been changed. Returns 0 on success, -1 on
>>    * failure.
>>    */
>> -int save_term(int full_duplex);
>> +int save_term(unsigned flags);
>>   /* Restore the terminal attributes that were saved with save_term() */
>>   void restore_term(void);
>>   

Re: [PATCH 1/4] terminal: use flags for save_term()

[Ramsay Jones]

Hi Phillip,

On 07/03/2022 11:11, Phillip Wood wrote:
> Hi Ramsay
[snip]
>> .. but here, you pass the flags as the second parameter. ;-)
> 
> Oh dear that's embarrassing, thanks for your careful review.
> 
> Are patches 3 & 4 OK for non-stop platforms?

Err... I didn't notice any problems with patches 3 & 4, but, as
far as the non-stop platform is concerned, I wouldn't have a clue! ;-)

(Perhaps you were thinking of Randall?)

ATB,
Ramsay Jones

Re: [PATCH 1/4] terminal: use flags for save_term()

[Phillip Wood]

Hi Ramsay

On 07/03/2022 20:21, Ramsay Jones wrote:
> Hi Phillip,
> 
> On 07/03/2022 11:11, Phillip Wood wrote:
>> Hi Ramsay
> [snip]
>>> .. but here, you pass the flags as the second parameter. ;-)
>>
>> Oh dear that's embarrassing, thanks for your careful review.
>>
>> Are patches 3 & 4 OK for non-stop platforms?
> 
> Err... I didn't notice any problems with patches 3 & 4, but, as
> far as the non-stop platform is concerned, I wouldn't have a clue! ;-)
> 
> (Perhaps you were thinking of Randall?)

Sorry, yes I had confused you with Randall

Apologies

Phillip

Re: [PATCH 1/4] terminal: use flags for save_term()

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 04 2022, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The next commit will add another flag in addition to the existing
> full_duplex so change the function signature to take an unsigned flags
> argument. Also alter the functions that call save_term() so that they
> can pass flags down to it.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  compat/terminal.c | 41 +++++++++++++++++++++--------------------
>  compat/terminal.h |  5 ++++-
>  2 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/compat/terminal.c b/compat/terminal.c
> index d882dfa06e..bad8e04cd8 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -34,7 +34,7 @@ void restore_term(void)
>  	sigchain_pop_common();
>  }
>  
> -int save_term(int full_duplex)
> +int save_term(unsigned flags)

Doing e.g.  ...

>  void restore_term(void)
> diff --git a/compat/terminal.h b/compat/terminal.h
> index 0fb9fa147c..f24b91390d 100644
> --- a/compat/terminal.h
> +++ b/compat/terminal.h
> @@ -1,14 +1,17 @@
>  #ifndef COMPAT_TERMINAL_H
>  #define COMPAT_TERMINAL_H
>  
> +/* Save input and output settings */
> +#define SAVE_TERM_DUPLEX (1u << 0)
	
	enum save_terminal_flags {
		SAVE_TERMINAL_FLAGS = 1 << 0,
	};
	
Here would be better IMO. See 3f9ab7ccdea (parse-options.[ch]:
consistently use "enum parse_opt_flags", 2021-10-08) for how it makes
debugging better.

Re: [PATCH 1/4] terminal: use flags for save_term()

[Phillip Wood]

On 05/03/2022 14:02, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Mar 04 2022, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The next commit will add another flag in addition to the existing
>> full_duplex so change the function signature to take an unsigned flags
>> argument. Also alter the functions that call save_term() so that they
>> can pass flags down to it.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   compat/terminal.c | 41 +++++++++++++++++++++--------------------
>>   compat/terminal.h |  5 ++++-
>>   2 files changed, 25 insertions(+), 21 deletions(-)
>>
>> diff --git a/compat/terminal.c b/compat/terminal.c
>> index d882dfa06e..bad8e04cd8 100644
>> --- a/compat/terminal.c
>> +++ b/compat/terminal.c
>> @@ -34,7 +34,7 @@ void restore_term(void)
>>   	sigchain_pop_common();
>>   }
>>   
>> -int save_term(int full_duplex)
>> +int save_term(unsigned flags)
> 
> Doing e.g.  ...
> 
>>   void restore_term(void)
>> diff --git a/compat/terminal.h b/compat/terminal.h
>> index 0fb9fa147c..f24b91390d 100644
>> --- a/compat/terminal.h
>> +++ b/compat/terminal.h
>> @@ -1,14 +1,17 @@
>>   #ifndef COMPAT_TERMINAL_H
>>   #define COMPAT_TERMINAL_H
>>   
>> +/* Save input and output settings */
>> +#define SAVE_TERM_DUPLEX (1u << 0)
> 	
> 	enum save_terminal_flags {
> 		SAVE_TERMINAL_FLAGS = 1 << 0,
> 	};
> 	
> Here would be better IMO. See 3f9ab7ccdea (parse-options.[ch]:
> consistently use "enum parse_opt_flags", 2021-10-08) for how it makes
> debugging better.

I'd remembered Junio objecting to enums for bit flags in the discussion 
of that patch but looking at the whole thread it seems like the debugger 
support lead him to change his mind. I'll update.

Best Wishes

Phillip

Re: [PATCH 1/4] terminal: use flags for save_term()

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 07 2022, Phillip Wood wrote:

> On 05/03/2022 14:02, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Mar 04 2022, Phillip Wood wrote:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> The next commit will add another flag in addition to the existing
>>> full_duplex so change the function signature to take an unsigned flags
>>> argument. Also alter the functions that call save_term() so that they
>>> can pass flags down to it.
>>>
>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> ---
>>>   compat/terminal.c | 41 +++++++++++++++++++++--------------------
>>>   compat/terminal.h |  5 ++++-
>>>   2 files changed, 25 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/compat/terminal.c b/compat/terminal.c
>>> index d882dfa06e..bad8e04cd8 100644
>>> --- a/compat/terminal.c
>>> +++ b/compat/terminal.c
>>> @@ -34,7 +34,7 @@ void restore_term(void)
>>>   	sigchain_pop_common();
>>>   }
>>>   -int save_term(int full_duplex)
>>> +int save_term(unsigned flags)
>> Doing e.g.  ...
>> 
>>>   void restore_term(void)
>>> diff --git a/compat/terminal.h b/compat/terminal.h
>>> index 0fb9fa147c..f24b91390d 100644
>>> --- a/compat/terminal.h
>>> +++ b/compat/terminal.h
>>> @@ -1,14 +1,17 @@
>>>   #ifndef COMPAT_TERMINAL_H
>>>   #define COMPAT_TERMINAL_H
>>>   +/* Save input and output settings */
>>> +#define SAVE_TERM_DUPLEX (1u << 0)
>> 	
>> 	enum save_terminal_flags {
>> 		SAVE_TERMINAL_FLAGS = 1 << 0,
>> 	};
>> 	
>> Here would be better IMO. See 3f9ab7ccdea (parse-options.[ch]:
>> consistently use "enum parse_opt_flags", 2021-10-08) for how it makes
>> debugging better.
>
> I'd remembered Junio objecting to enums for bit flags in the
> discussion of that patch but looking at the whole thread it seems like
> the debugger support lead him to change his mind. I'll update.

Yeah, aside from that I think part of that was whether it was worth it
to refactor it for existing code, but since this is new code & we tend
to use that pattern liberally (which pre-dates any recent changes I did
by quite a bit...), ....

[PATCH 2/4] terminal: don't assume stdin is /dev/tty

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

read_key_without_echo() reads from stdin but uses /dev/tty when it
disables echo. This is unfortunate as there no guarantee that stdin is
the same device as /dev/tty. The perl version of "add -p" uses stdin
when it sets the terminal mode, this commit does the same for the
builtin version. There is still a difference between the perl and
builtin versions though - the perl version will ignore any errors when
setting the terminal mode[1] and will still read single bytes when
stdin is not a terminal. The builtin version displays a warning if
setting the terminal mode fails and switches to reading a line at a
time.

[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 5 +++--
 compat/terminal.h | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index bad8e04cd8..249836e78f 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -37,7 +37,8 @@ void restore_term(void)
 int save_term(unsigned flags)
 {
 	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 (tcgetattr(term_fd, &old_term) < 0)
@@ -362,7 +363,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical(0) < 0) {
+	if (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
diff --git a/compat/terminal.h b/compat/terminal.h
index f24b91390d..f5655d0d0b 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -3,7 +3,8 @@
 
 /* Save input and output settings */
 #define SAVE_TERM_DUPLEX (1u << 0)
-
+/* Save stdin rather than /dev/tty (fails is stdin is not a terminal) */
+#define SAVE_TERM_STDIN  (1u << 1)
 /*
  * Save the terminal attributes so they can be restored later by a
  * call to restore_term(). Note that every successful call to
-- 
2.35.1

Re: [PATCH 2/4] terminal: don't assume stdin is /dev/tty

[Ramsay Jones]

On 04/03/2022 13:11, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
[snip]
> diff --git a/compat/terminal.h b/compat/terminal.h
> index f24b91390d..f5655d0d0b 100644
> --- a/compat/terminal.h
> +++ b/compat/terminal.h
> @@ -3,7 +3,8 @@
>  
>  /* Save input and output settings */
>  #define SAVE_TERM_DUPLEX (1u << 0)
> -
> +/* Save stdin rather than /dev/tty (fails is stdin is not a terminal) */

s/fails is/fails if/ ?

ATB,
Ramsay Jones

> +#define SAVE_TERM_STDIN  (1u << 1)
>  /*
>   * Save the terminal attributes so they can be restored later by a
>   * call to restore_term(). Note that every successful call to

[PATCH 3/4] terminal: work around macos poll() bug

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

On macos the builtin "add -p" does not handle keys that generate
escape sequences because poll() does not work with terminals
there. Switch to using select() on non-windows platforms to work
around this.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 249836e78f..5d516ff546 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -82,6 +82,32 @@ static int enable_non_canonical(unsigned flags)
 	return disable_bits(flags, ICANON | ECHO);
 }
 
+/*
+ * On macos it is not possible to use poll() with a terminal so use select
+ * instead.
+ */
+#include <sys/select.h>
+static int getchar_with_timeout(int timeout)
+{
+	struct timeval tv, *tvp = NULL;
+	fd_set readfds;
+	int res;
+
+	if (timeout >= 0) {
+		tv.tv_sec = timeout / 1000;
+		tv.tv_usec = (timeout % 1000) * 1000;
+		tvp = &tv;
+	}
+
+	FD_ZERO(&readfds);
+	FD_SET(0, &readfds);
+	res = select(1, &readfds, NULL, NULL, tvp);
+	if (res < 0)
+		return EOF;
+
+	return getchar();
+}
+
 #elif defined(GIT_WINDOWS_NATIVE)
 
 #define INPUT_PATH "CONIN$"
@@ -247,6 +273,16 @@ static int mingw_getchar(void)
 }
 #define getchar mingw_getchar
 
+static int getchar_with_timeout(int timeout)
+{
+	struct pollfd pfd = { .fd = 0, .events = POLLIN };
+
+	if (poll(&pfd, 1, timeout) < 1)
+		return EOF;
+
+	return getchar();
+}
+
 #endif
 
 #ifndef FORCE_TEXT
@@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)
 		 * half a second when we know that the sequence is complete.
 		 */
 		while (!is_known_escape_sequence(buf->buf)) {
-			struct pollfd pfd = { .fd = 0, .events = POLLIN };
-
-			if (poll(&pfd, 1, 500) < 1)
-				break;
-
-			ch = getchar();
+			ch = getchar_with_timeout(500);
 			if (ch == EOF)
 				return 0;
 			strbuf_addch(buf, ch);
-- 
2.35.1

[PATCH 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user suspends git while it is waiting for a keypress reset the
terminal before stopping and restore the settings when git resumes. If
the user tries to resume in the background print an error
message (taking care to use async safe functions) before stopping
again. Ideally we would reprint the prompt for the user when git
resumes but this patch just restarts the read().

The signal handler is established with sigaction() rather than using
sigchain_push() as this allows us to control the signal mask when the
handler is invoked and ensure SA_RESTART is used to restart the
read() when resuming.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 124 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 120 insertions(+), 4 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 5d516ff546..79ab54c2f8 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -1,4 +1,4 @@
-#include "git-compat-util.h"
+#include "cache.h"
 #include "compat/terminal.h"
 #include "sigchain.h"
 #include "strbuf.h"
@@ -23,6 +23,89 @@ static void restore_term_on_signal(int sig)
 static int term_fd = -1;
 static struct termios old_term;
 
+static char *background_resume_msg;
+static char *restore_error_msg;
+static volatile sig_atomic_t ttou_received;
+
+static void write_msg(const char *msg)
+{
+	write_in_full(2, msg, strlen(msg));
+	write_in_full(2, "\n", 1);
+}
+
+static void print_background_resume_msg(int signo)
+{
+	int saved_errno = errno;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+
+	ttou_received = 1;
+	write_msg(background_resume_msg);
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	errno = saved_errno;
+}
+
+static void restore_terminal_on_suspend(int signo)
+{
+	int saved_errno = errno;
+	int res;
+	struct termios t;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+	int can_restore = 1;
+
+	if (tcgetattr(term_fd, &t) < 0)
+		can_restore = 0;
+
+	if (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)
+		write_msg(restore_error_msg);
+
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	if (!can_restore) {
+		write_msg(restore_error_msg);
+		goto out;
+	}
+	/*
+	 * If we resume in the background then we receive SIGTTOU when calling
+	 * tcsetattr() below. Set up a handler to print an error message in that
+	 * case.
+	 */
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGTTOU);
+	sa.sa_mask = old_sa.sa_mask;
+	sa.sa_handler = print_background_resume_msg;
+	sa.sa_flags = SA_RESTART;
+	sigaction(SIGTTOU, &sa, &old_sa);
+ again:
+	ttou_received = 0;
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	res = tcsetattr(term_fd, TCSAFLUSH, &t);
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	if (ttou_received)
+		goto again;
+	else if (res < 0)
+		write_msg(restore_error_msg);
+	sigaction(SIGTTOU, &old_sa, NULL);
+ out:
+	errno = saved_errno;
+}
+
 void restore_term(void)
 {
 	if (term_fd < 0)
@@ -32,10 +115,19 @@ void restore_term(void)
 	close(term_fd);
 	term_fd = -1;
 	sigchain_pop_common();
+	if (restore_error_msg) {
+		signal(SIGTTIN, SIG_DFL);
+		signal(SIGTTOU, SIG_DFL);
+		signal(SIGTSTP, SIG_DFL);
+		FREE_AND_NULL(restore_error_msg);
+		FREE_AND_NULL(background_resume_msg);
+	}
 }
 
 int save_term(unsigned flags)
 {
+	struct sigaction sa;
+
 	if (term_fd < 0)
 		term_fd = (flags & SAVE_TERM_STDIN) ? 0
 						    : open("/dev/tty", O_RDWR);
@@ -44,6 +136,26 @@ int save_term(unsigned 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
+	 * 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 = xstrdup(_("error: cannot resume in the background"));
+	restore_error_msg = xstrdup(_("error: 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;
 }
@@ -93,6 +205,7 @@ static int getchar_with_timeout(int timeout)
 	fd_set readfds;
 	int res;
 
+ again:
 	if (timeout >= 0) {
 		tv.tv_sec = timeout / 1000;
 		tv.tv_usec = (timeout % 1000) * 1000;
@@ -102,9 +215,12 @@ static int getchar_with_timeout(int timeout)
 	FD_ZERO(&readfds);
 	FD_SET(0, &readfds);
 	res = select(1, &readfds, NULL, NULL, tvp);
-	if (res < 0)
-		return EOF;
-
+	if (res < 0) {
+		if (errno == EINTR)
+			goto again;
+		else
+			return EOF;
+	}
 	return getchar();
 }
 
-- 
2.35.1

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 04 2022, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the user suspends git while it is waiting for a keypress reset the
> terminal before stopping and restore the settings when git resumes. If
> the user tries to resume in the background print an error
> message (taking care to use async safe functions) before stopping
> again. Ideally we would reprint the prompt for the user when git
> resumes but this patch just restarts the read().
>
> The signal handler is established with sigaction() rather than using
> sigchain_push() as this allows us to control the signal mask when the
> handler is invoked and ensure SA_RESTART is used to restart the
> read() when resuming.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  compat/terminal.c | 124 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 5d516ff546..79ab54c2f8 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -1,4 +1,4 @@
> -#include "git-compat-util.h"
> +#include "cache.h"
>  #include "compat/terminal.h"
>  #include "sigchain.h"
>  #include "strbuf.h"
> @@ -23,6 +23,89 @@ static void restore_term_on_signal(int sig)
>  static int term_fd = -1;
>  static struct termios old_term;
>  
> +static char *background_resume_msg;
> +static char *restore_error_msg;
> +static volatile sig_atomic_t ttou_received;
> +
> +static void write_msg(const char *msg)
> +{
> +	write_in_full(2, msg, strlen(msg));
> +	write_in_full(2, "\n", 1);
> +}
> +
> +static void print_background_resume_msg(int signo)
> +{
> +	int saved_errno = errno;
> +	sigset_t mask;
> +	struct sigaction old_sa;
> +	struct sigaction sa = { .sa_handler = SIG_DFL };
> +
> +	ttou_received = 1;
> +	write_msg(background_resume_msg);
> +	sigaction(signo, &sa, &old_sa);
> +	raise(signo);
> +	sigemptyset(&mask);
> +	sigaddset(&mask, signo);
> +	sigprocmask(SIG_UNBLOCK, &mask, NULL);
> +	/* Stopped here */
> +	sigprocmask(SIG_BLOCK, &mask, NULL);
> +	sigaction(signo, &old_sa, NULL);
> +	errno = saved_errno;
> +}
> +
> +static void restore_terminal_on_suspend(int signo)
> +{
> +	int saved_errno = errno;
> +	int res;
> +	struct termios t;
> +	sigset_t mask;
> +	struct sigaction old_sa;
> +	struct sigaction sa = { .sa_handler = SIG_DFL };
> +	int can_restore = 1;
> +
> +	if (tcgetattr(term_fd, &t) < 0)
> +		can_restore = 0;
> +
> +	if (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)
> +		write_msg(restore_error_msg);
> +
> +	sigaction(signo, &sa, &old_sa);
> +	raise(signo);
> +	sigemptyset(&mask);
> +	sigaddset(&mask, signo);
> +	sigprocmask(SIG_UNBLOCK, &mask, NULL);
> +	/* Stopped here */
> +	sigprocmask(SIG_BLOCK, &mask, NULL);
> +	sigaction(signo, &old_sa, NULL);
> +	if (!can_restore) {
> +		write_msg(restore_error_msg);
> +		goto out;
> +	}
> +	/*
> +	 * If we resume in the background then we receive SIGTTOU when calling
> +	 * tcsetattr() below. Set up a handler to print an error message in that
> +	 * case.
> +	 */
> +	sigemptyset(&mask);
> +	sigaddset(&mask, SIGTTOU);
> +	sa.sa_mask = old_sa.sa_mask;
> +	sa.sa_handler = print_background_resume_msg;
> +	sa.sa_flags = SA_RESTART;
> +	sigaction(SIGTTOU, &sa, &old_sa);
> + again:
> +	ttou_received = 0;
> +	sigprocmask(SIG_UNBLOCK, &mask, NULL);
> +	res = tcsetattr(term_fd, TCSAFLUSH, &t);
> +	sigprocmask(SIG_BLOCK, &mask, NULL);
> +	if (ttou_received)
> +		goto again;
> +	else if (res < 0)
> +		write_msg(restore_error_msg);
> +	sigaction(SIGTTOU, &old_sa, NULL);
> + out:
> +	errno = saved_errno;
> +}
> +
>  void restore_term(void)
>  {
>  	if (term_fd < 0)
> @@ -32,10 +115,19 @@ void restore_term(void)
>  	close(term_fd);
>  	term_fd = -1;
>  	sigchain_pop_common();
> +	if (restore_error_msg) {
> +		signal(SIGTTIN, SIG_DFL);
> +		signal(SIGTTOU, SIG_DFL);
> +		signal(SIGTSTP, SIG_DFL);
> +		FREE_AND_NULL(restore_error_msg);
> +		FREE_AND_NULL(background_resume_msg);
> +	}
>  }
>  
>  int save_term(unsigned flags)
>  {
> +	struct sigaction sa;
> +
>  	if (term_fd < 0)
>  		term_fd = (flags & SAVE_TERM_STDIN) ? 0
>  						    : open("/dev/tty", O_RDWR);
> @@ -44,6 +136,26 @@ int save_term(unsigned 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
> +	 * 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 = xstrdup(_("error: cannot resume in the background"));
> +	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));

You don't need to xstrdup() the return values of gettext() (here _()),
you'll get a pointer to static storage that's safe to hold on to for the
duration of the program.

In this case I think it would make sense to skip "error: " from the
message itself.

Eventually we'll get to making usage.c have that prefix translated, and
can have some utility function exposed there (I have WIP patches for
this already since a while ago).

To translators it'll look like the same thing, and avoid churn when we
make the "error: " prefix translatable.

Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above
advice...) doesn't it make sense to add the "\n" here, so you'll have
one write_in_full() above?

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

Hi Ævar

On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:
> [...] 
>>   int save_term(unsigned flags)
>>   {
>> +	struct sigaction sa;
>> +
>>   	if (term_fd < 0)
>>   		term_fd = (flags & SAVE_TERM_STDIN) ? 0
>>   						    : open("/dev/tty", O_RDWR);
>> @@ -44,6 +136,26 @@ int save_term(unsigned 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
>> +	 * 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 = xstrdup(_("error: cannot resume in the background"));
>> +	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
> 
> You don't need to xstrdup() the return values of gettext() (here _()),
> you'll get a pointer to static storage that's safe to hold on to for the
> duration of the program.

I had a look at the documentation and could not see anything about the 
lifetime of the returned string, all it says is "don't alter it"

> In this case I think it would make sense to skip "error: " from the
> message itself.
> 
> Eventually we'll get to making usage.c have that prefix translated, and
> can have some utility function exposed there (I have WIP patches for
> this already since a while ago).
> 
> To translators it'll look like the same thing, and avoid churn when we
> make the "error: " prefix translatable.

Unless we add a function that returns a string rather than printing the 
message I don't see how it avoids churn in the future. Having the whole 
string with the "error: " prefix translated here does not add any extra 
burden to translators - it is still the same number of strings to translate.

> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above
> advice...) doesn't it make sense to add the "\n" here, so you'll have
> one write_in_full() above?

I decided to keep the translated string simpler by omitting the newline, 
calling write_in_full() twice isn't a bit deal (I don't think the output 
can be split by a write from another thread or signal handler in between).

Best Wishes

Phillip

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 07 2022, Phillip Wood wrote:

> Hi Ævar
>
> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:
>> [...] 
>>>   int save_term(unsigned flags)
>>>   {
>>> +	struct sigaction sa;
>>> +
>>>   	if (term_fd < 0)
>>>   		term_fd = (flags & SAVE_TERM_STDIN) ? 0
>>>   						    : open("/dev/tty", O_RDWR);
>>> @@ -44,6 +136,26 @@ int save_term(unsigned 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
>>> +	 * 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 = xstrdup(_("error: cannot resume in the background"));
>>> +	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
>> You don't need to xstrdup() the return values of gettext() (here
>> _()),
>> you'll get a pointer to static storage that's safe to hold on to for the
>> duration of the program.
>
> I had a look at the documentation and could not see anything about the
> lifetime of the returned string, all it says is "don't alter it"

I think this is overed in "11.2.7 Optimization of the *gettext
functions", a pedantic reading might suggest not, but what's meant with
the combination of that API documentation & the description of how MO
files work is that you're just getting pointers into the already-loaded
translation catalog, so it's safe to hold onto the pointer and re-use it
later.

In any case, if we're going to be paranoid about gettext() it would make
sense to propose that as some general change to how we use it, we rely
on this assumption holding in a lot of our use of the API:

    git grep '= _\('

Rather than sneak that partcular new assumption in here in this already
tricky code...

>> In this case I think it would make sense to skip "error: " from the
>> message itself.
>> Eventually we'll get to making usage.c have that prefix translated,
>> and
>> can have some utility function exposed there (I have WIP patches for
>> this already since a while ago).
>> To translators it'll look like the same thing, and avoid churn when
>> we
>> make the "error: " prefix translatable.
>
> Unless we add a function that returns a string rather than printing
> the message I don't see how it avoids churn in the future. Having the
> whole string with the "error: " prefix translated here does not add
> any extra burden to translators - it is still the same number of
> strings to translate.

Because translators translate "we failed" for most errors, not "error:
we failed".

If and when we convert it from "error: we failed" to "we failed" they'll
need to translate it again (although to be fair, the translation cache
will help).

And even then it'll be one of very few exceptions where the "error: "
currently that *is* translated.

>> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above
>> advice...) doesn't it make sense to add the "\n" here, so you'll have
>> one write_in_full() above?
>
> I decided to keep the translated string simpler by omitting the
> newline, calling write_in_full() twice isn't a bit deal (I don't think
> the output can be split by a write from another thread or signal
> handler in between).

Makes sense.

FWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you
could do:

    xstrfmt("error: %s\n", _("the error"))

And then do one call to write_in_full().

But I think just:

    msg = _("the error");

And then:

	const char *const = pfx = "error: ";
        const size_t len = strlen(pfx);

	write_in_full(2, pfx, len);
        write_in_full(2, msg, strlen(msg));
	write_in_full(2, "\n", 1);

Makes more sense :)

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 07 2022, Phillip Wood wrote:
> 
>> Hi Ævar
>>
>> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:
>>> [...]
>>>>    int save_term(unsigned flags)
>>>>    {
>>>> +	struct sigaction sa;
>>>> +
>>>>    	if (term_fd < 0)
>>>>    		term_fd = (flags & SAVE_TERM_STDIN) ? 0
>>>>    						    : open("/dev/tty", O_RDWR);
>>>> @@ -44,6 +136,26 @@ int save_term(unsigned 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
>>>> +	 * 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 = xstrdup(_("error: cannot resume in the background"));
>>>> +	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
>>> You don't need to xstrdup() the return values of gettext() (here
>>> _()),
>>> you'll get a pointer to static storage that's safe to hold on to for the
>>> duration of the program.
>>
>> I had a look at the documentation and could not see anything about the
>> lifetime of the returned string, all it says is "don't alter it"
> 
> I think this is overed in "11.2.7 Optimization of the *gettext
> functions", a pedantic reading might suggest not, but what's meant with
> the combination of that API documentation & the description of how MO
> files work is that you're just getting pointers into the already-loaded
> translation catalog, so it's safe to hold onto the pointer and re-use it
> later.

Strictly that section only shows it is safe if there are no other calls 
to gettext() before the returned string is used. I agree the 
implementation is likely to be just returning static strings but I can't 
find anywhere that says another implementation (e.g. on macos/*bsd) has 
to do that.

> In any case, if we're going to be paranoid about gettext() it would make
> sense to propose that as some general change to how we use it, we rely
> on this assumption holding in a lot of our use of the API:
> 
>      git grep '= _\('
> 
> Rather than sneak that partcular new assumption in here in this already
> tricky code...

The ones I looked at are mostly not calling gettext() again before using 
the translated string (there is one exception in builtin/remote.c).

In restore_term() I'm checking if the messages are NULL to see if job 
control is enabled, I could use a flag but I'm inclined to just keep 
coping the strings.

> 
>>> In this case I think it would make sense to skip "error: " from the
>>> message itself.
>>> Eventually we'll get to making usage.c have that prefix translated,
>>> and
>>> can have some utility function exposed there (I have WIP patches for
>>> this already since a while ago).
>>> To translators it'll look like the same thing, and avoid churn when
>>> we
>>> make the "error: " prefix translatable.
>>
>> Unless we add a function that returns a string rather than printing
>> the message I don't see how it avoids churn in the future. Having the
>> whole string with the "error: " prefix translated here does not add
>> any extra burden to translators - it is still the same number of
>> strings to translate.
> 
> Because translators translate "we failed" for most errors, not "error:
> we failed".
> 
> If and when we convert it from "error: we failed" to "we failed" they'll
> need to translate it again (although to be fair, the translation cache
> will help).
> 
> And even then it'll be one of very few exceptions where the "error: "
> currently that *is* translated.
> 
>>> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above
>>> advice...) doesn't it make sense to add the "\n" here, so you'll have
>>> one write_in_full() above?
>>
>> I decided to keep the translated string simpler by omitting the
>> newline, calling write_in_full() twice isn't a bit deal (I don't think
>> the output can be split by a write from another thread or signal
>> handler in between).
> 
> Makes sense.
> 
> FWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you
> could do:
> 
>      xstrfmt("error: %s\n", _("the error"))
> 
> And then do one call to write_in_full().
> 
> But I think just:
> 
>      msg = _("the error");
> 
> And then:
> 
> 	const char *const = pfx = "error: ";
>          const size_t len = strlen(pfx);
> 
> 	write_in_full(2, pfx, len);
>          write_in_full(2, msg, strlen(msg));
> 	write_in_full(2, "\n", 1);
> 
> Makes more sense :)

Agreed, I'll change that.

Best Wishes

Phillip

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 07 2022, Phillip Wood wrote:

> On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Mar 07 2022, Phillip Wood wrote:
>> 
>>> Hi Ævar
>>>
>>> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:
>>>> [...]
>>>>>    int save_term(unsigned flags)
>>>>>    {
>>>>> +	struct sigaction sa;
>>>>> +
>>>>>    	if (term_fd < 0)
>>>>>    		term_fd = (flags & SAVE_TERM_STDIN) ? 0
>>>>>    						    : open("/dev/tty", O_RDWR);
>>>>> @@ -44,6 +136,26 @@ int save_term(unsigned 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
>>>>> +	 * 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 = xstrdup(_("error: cannot resume in the background"));
>>>>> +	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
>>>> You don't need to xstrdup() the return values of gettext() (here
>>>> _()),
>>>> you'll get a pointer to static storage that's safe to hold on to for the
>>>> duration of the program.
>>>
>>> I had a look at the documentation and could not see anything about the
>>> lifetime of the returned string, all it says is "don't alter it"
>> I think this is overed in "11.2.7 Optimization of the *gettext
>> functions", a pedantic reading might suggest not, but what's meant with
>> the combination of that API documentation & the description of how MO
>> files work is that you're just getting pointers into the already-loaded
>> translation catalog, so it's safe to hold onto the pointer and re-use it
>> later.
>
> Strictly that section only shows it is safe if there are no other
> calls to gettext() before the returned string is used. I agree the 
> implementation is likely to be just returning static strings but I
> can't find anywhere that says another implementation (e.g. on
> macos/*bsd) has to do that.

I agree. I'm 99.99% sure this is safe & portable use of the API, but I'm
having some trouble finding documentation for that...

>> In any case, if we're going to be paranoid about gettext() it would make
>> sense to propose that as some general change to how we use it, we rely
>> on this assumption holding in a lot of our use of the API:
>>      git grep '= _\('
>> Rather than sneak that partcular new assumption in here in this
>> already
>> tricky code...
>
> The ones I looked at are mostly not calling gettext() again before
> using the translated string (there is one exception in
> builtin/remote.c).

Doesn't validate_encoding() in convert.c, process_entry() in
merge-ort.c, setup_unpack_trees_porcelain() in unpack-trees.c cmd_mv()
in builtin/mv.c etc. qualify?

I.e. for a hypothetical gettext() that always returned the same pointer
and just overwrote it with the latest message those would all emit bad
output, wouldn't they?

> In restore_term() I'm checking if the messages are NULL to see if job
> control is enabled, I could use a flag but I'm inclined to just keep 
> coping the strings.

Checking if they're NULL is orthagonal to whether we xstrdup()
them. I.e. you'd just skip the xstrdup() and replace the FREE_AND_NULL
with a "= NULL" assignment, no?

Anyway, *if* I'm right that it's not new general paranoia with how
gettext() is used I still think splitting up that part of the change
would make sense, just for future readers etc. who'd wonder why it is
that this already tricky signal handling etc. code needs that particular
bit of special behavior.

>>>> In this case I think it would make sense to skip "error: " from the
>>>> message itself.
>>>> Eventually we'll get to making usage.c have that prefix translated,
>>>> and
>>>> can have some utility function exposed there (I have WIP patches for
>>>> this already since a while ago).
>>>> To translators it'll look like the same thing, and avoid churn when
>>>> we
>>>> make the "error: " prefix translatable.
>>>
>>> Unless we add a function that returns a string rather than printing
>>> the message I don't see how it avoids churn in the future. Having the
>>> whole string with the "error: " prefix translated here does not add
>>> any extra burden to translators - it is still the same number of
>>> strings to translate.
>> Because translators translate "we failed" for most errors, not
>> "error:
>> we failed".
>> If and when we convert it from "error: we failed" to "we failed"
>> they'll
>> need to translate it again (although to be fair, the translation cache
>> will help).
>> And even then it'll be one of very few exceptions where the "error:
>> "
>> currently that *is* translated.
>> 
>>>> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above
>>>> advice...) doesn't it make sense to add the "\n" here, so you'll have
>>>> one write_in_full() above?
>>>
>>> I decided to keep the translated string simpler by omitting the
>>> newline, calling write_in_full() twice isn't a bit deal (I don't think
>>> the output can be split by a write from another thread or signal
>>> handler in between).
>> Makes sense.
>> FWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you
>> could do:
>>      xstrfmt("error: %s\n", _("the error"))
>> And then do one call to write_in_full().
>> But I think just:
>>      msg = _("the error");
>> And then:
>> 	const char *const = pfx = "error: ";
>>          const size_t len = strlen(pfx);
>> 	write_in_full(2, pfx, len);
>>          write_in_full(2, msg, strlen(msg));
>> 	write_in_full(2, "\n", 1);
>> Makes more sense :)
>
> Agreed, I'll change that.
>
> Best Wishes
>
> Phillip

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

On 07/03/2022 14:45, Ævar Arnfjörð Bjarmason wrote:
>[...] 
> On Mon, Mar 07 2022, Phillip Wood wrote:
> 
>> On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:
>>> On Mon, Mar 07 2022, Phillip Wood wrote:
>>>
>>>> Hi Ævar
>>>>
>>>> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:
>>>>> [...]
>>>>>>     int save_term(unsigned flags)
>>>>>>     {
>>>>>> +	struct sigaction sa;
>>>>>> +
>>>>>>     	if (term_fd < 0)
>>>>>>     		term_fd = (flags & SAVE_TERM_STDIN) ? 0
>>>>>>     						    : open("/dev/tty", O_RDWR);
>>>>>> @@ -44,6 +136,26 @@ int save_term(unsigned 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
>>>>>> +	 * 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 = xstrdup(_("error: cannot resume in the background"));
>>>>>> +	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
>>>>> You don't need to xstrdup() the return values of gettext() (here
>>>>> _()),
>>>>> you'll get a pointer to static storage that's safe to hold on to for the
>>>>> duration of the program.
>>>>
>>>> I had a look at the documentation and could not see anything about the
>>>> lifetime of the returned string, all it says is "don't alter it"
>>> I think this is overed in "11.2.7 Optimization of the *gettext
>>> functions", a pedantic reading might suggest not, but what's meant with
>>> the combination of that API documentation & the description of how MO
>>> files work is that you're just getting pointers into the already-loaded
>>> translation catalog, so it's safe to hold onto the pointer and re-use it
>>> later.
>>
>> Strictly that section only shows it is safe if there are no other
>> calls to gettext() before the returned string is used. I agree the
>> implementation is likely to be just returning static strings but I
>> can't find anywhere that says another implementation (e.g. on
>> macos/*bsd) has to do that.
> 
> I agree. I'm 99.99% sure this is safe & portable use of the API, but I'm
> having some trouble finding documentation for that...
> 
>>> In any case, if we're going to be paranoid about gettext() it would make
>>> sense to propose that as some general change to how we use it, we rely
>>> on this assumption holding in a lot of our use of the API:
>>>       git grep '= _\('
>>> Rather than sneak that partcular new assumption in here in this
>>> already
>>> tricky code...
>>
>> The ones I looked at are mostly not calling gettext() again before
>> using the translated string (there is one exception in
>> builtin/remote.c).
> 
> Doesn't validate_encoding() in convert.c, process_entry() in
> merge-ort.c, setup_unpack_trees_porcelain() in unpack-trees.c cmd_mv()
> in builtin/mv.c etc. qualify?

I only checked a few, cmd_mv() always assigns to the same variable so 
the previous value is overwritten anyway, some of the others such as 
unpack_trees are assuming the return value is valid after a subsequent 
call to gettext(). I found[1] which states

     The string returned must not be modified by the program and can
     be invalidated by a subsequent call to bind_textdomain_codeset()
     or setlocale(3C).

so I think we can drop the copying.

> I.e. for a hypothetical gettext() that always returned the same pointer
> and just overwrote it with the latest message those would all emit bad
> output, wouldn't they?
> 
>> In restore_term() I'm checking if the messages are NULL to see if job
>> control is enabled, I could use a flag but I'm inclined to just keep
>> coping the strings.
> 
> Checking if they're NULL is orthagonal to whether we xstrdup()
> them. I.e. you'd just skip the xstrdup() and replace the FREE_AND_NULL
> with a "= NULL" assignment, no?

Yes, I'm not sure what I was thinking when I wrote that.

Best Wishes

Phillip

[1] 
https://docs.oracle.com/cd/E88353_01/html/E37843/gettext-3c.html#REFMAN3Agettext-3c

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Johannes Schindelin]

Hi Phillip,

On Fri, 4 Mar 2022, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the user suspends git while it is waiting for a keypress reset the
> terminal before stopping and restore the settings when git resumes. If
> the user tries to resume in the background print an error
> message (taking care to use async safe functions) before stopping
> again. Ideally we would reprint the prompt for the user when git
> resumes but this patch just restarts the read().
>
> The signal handler is established with sigaction() rather than using
> sigchain_push() as this allows us to control the signal mask when the
> handler is invoked and ensure SA_RESTART is used to restart the
> read() when resuming.

This description makes sense. From my understanding of signals, the code
also does make sense, but it is unfortunate that it has to be so much code
to implement something as straight-forward as suspend/resume.

FWIW I tested the `add -p` command with these patches on Windows and it
still works as well as when I had developed it.

Thank you,
Dscho

Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

Hi Dscho

On 09/03/2022 12:19, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Fri, 4 Mar 2022, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the user suspends git while it is waiting for a keypress reset the
>> terminal before stopping and restore the settings when git resumes. If
>> the user tries to resume in the background print an error
>> message (taking care to use async safe functions) before stopping
>> again. Ideally we would reprint the prompt for the user when git
>> resumes but this patch just restarts the read().
>>
>> The signal handler is established with sigaction() rather than using
>> sigchain_push() as this allows us to control the signal mask when the
>> handler is invoked and ensure SA_RESTART is used to restart the
>> read() when resuming.
> 
> This description makes sense. From my understanding of signals, the code
> also does make sense, but it is unfortunate that it has to be so much code
> to implement something as straight-forward as suspend/resume.

Yes it is a lot of code. It would be a bit simpler if we omitted the 
warning about resuming in the background but I think that is worth 
having. There's also a lot of changing signal masks to avoid stopping 
twice if the user presses ^Z a second time while the signal handler is 
active.

> FWIW I tested the `add -p` command with these patches on Windows and it
> still works as well as when I had developed it.

Thanks for testing this on Windows, I don't think we have any meaningful 
test coverage for interactive.singlekey and it is probably tricky to add 
because it relies on having a tty.


Best Wishes

Phillip
> Thank you,
> Dscho

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

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 | 206 +++++++++++++++++++++++++++++++++++++++-------
 compat/terminal.h |   9 +-
 2 files changed, 186 insertions(+), 29 deletions(-)

Range-diff against v1:
1:  6961a4e0dc ! 1:  fac5a0f5b7 terminal: use flags for save_term()
    @@ Commit message
         terminal: use flags for save_term()
     
         The next commit will add another flag in addition to the existing
    -    full_duplex so change the function signature to take an unsigned flags
    +    full_duplex so change the function signature to take a flags
         argument. Also alter the functions that call save_term() so that they
         can pass flags down to it.
     
    +    The choice to use an enum for tho bitwise flags is because gdb will
    +    display the symbolic names of all the flags that are set rather than
    +    the integer value.
    +
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## compat/terminal.c ##
    @@ compat/terminal.c: void restore_term(void)
      }
      
     -int save_term(int full_duplex)
    -+int save_term(unsigned flags)
    ++int save_term(enum save_term_flags flags)
      {
      	if (term_fd < 0)
      		term_fd = open("/dev/tty", O_RDWR);
    @@ compat/terminal.c: int save_term(int full_duplex)
      }
      
     -static int disable_bits(tcflag_t bits)
    -+static int disable_bits(unsigned flags, tcflag_t bits)
    ++static int disable_bits(enum save_term_flags flags, tcflag_t bits)
      {
      	struct termios t;
      
    @@ compat/terminal.c: static int disable_bits(tcflag_t bits)
      }
      
     -static int disable_echo(void)
    -+static int disable_echo(unsigned flags)
    ++static int disable_echo(enum save_term_flags flags)
      {
     -	return disable_bits(ECHO);
     +	return disable_bits(flags, ECHO);
      }
      
     -static int enable_non_canonical(void)
    -+static int enable_non_canonical(unsigned flags)
    ++static int enable_non_canonical(enum save_term_flags flags)
      {
     -	return disable_bits(ICANON | ECHO);
     +	return disable_bits(flags, ICANON | ECHO);
    @@ compat/terminal.c: void restore_term(void)
      }
      
     -int save_term(int full_duplex)
    -+int save_term(unsigned flags)
    ++int save_term(enum save_term_flags flags)
      {
      	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
      	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
    @@ compat/terminal.c: int save_term(int full_duplex)
      }
      
     -static int disable_bits(DWORD bits)
    -+static int disable_bits(unsigned flags, DWORD bits)
    ++static int disable_bits(enum save_term_flags flags, DWORD bits)
      {
      	if (use_stty) {
      		struct child_process cp = CHILD_PROCESS_INIT;
    @@ compat/terminal.c: static int disable_bits(DWORD bits)
      }
      
     -static int disable_echo(void)
    -+static int disable_echo(unsigned flags)
    ++static int disable_echo(enum save_term_flags flags)
      {
     -	return disable_bits(ENABLE_ECHO_INPUT);
    -+	return disable_bits(ENABLE_ECHO_INPUT, flags);
    ++	return disable_bits(flags, ENABLE_ECHO_INPUT);
      }
      
     -static int enable_non_canonical(void)
    -+static int enable_non_canonical(unsigned flags)
    ++static int enable_non_canonical(enum save_term_flags flags)
      {
     -	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
    -+	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,
    -+			    flags);
    ++	return disable_bits(flags,
    ++			    ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
      }
      
      /*
    @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)
      #else
      
     -int save_term(int full_duplex)
    -+int save_term(unsigned flags)
    ++int save_term(enum save_term_flags flags)
      {
     -	/* full_duplex == 1, but no support available */
     -	return -full_duplex;
    @@ compat/terminal.h
      #ifndef COMPAT_TERMINAL_H
      #define COMPAT_TERMINAL_H
      
    -+/* Save input and output settings */
    -+#define SAVE_TERM_DUPLEX (1u << 0)
    ++enum save_term_flags {
    ++	/* Save input and output settings */
    ++	SAVE_TERM_DUPLEX = 1 << 0,
    ++};
     +
      /*
       * Save the terminal attributes so they can be restored later by a
2:  45315087d4 ! 2:  bf29460ec6 terminal: don't assume stdin is /dev/tty
    @@ Commit message
     
      ## compat/terminal.c ##
     @@ compat/terminal.c: void restore_term(void)
    - int save_term(unsigned flags)
    + int save_term(enum save_term_flags flags)
      {
      	if (term_fd < 0)
     -		term_fd = open("/dev/tty", O_RDWR);
    @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)
     
      ## compat/terminal.h ##
     @@
    + enum save_term_flags {
    + 	/* Save input and output settings */
    + 	SAVE_TERM_DUPLEX = 1 << 0,
    ++	/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */
    ++	SAVE_TERM_STDIN  = 1 << 1,
    + };
      
    - /* Save input and output settings */
    - #define SAVE_TERM_DUPLEX (1u << 0)
    --
    -+/* Save stdin rather than /dev/tty (fails is stdin is not a terminal) */
    -+#define SAVE_TERM_STDIN  (1u << 1)
      /*
    -  * Save the terminal attributes so they can be restored later by a
    -  * call to restore_term(). Note that every successful call to
4:  62703be209 ! 4:  dd0e1fabb1 terminal: restore settings on SIGTSTP
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
      static int term_fd = -1;
      static struct termios old_term;
      
    -+static char *background_resume_msg;
    -+static char *restore_error_msg;
    ++static const char *background_resume_msg;
    ++static const char *restore_error_msg;
     +static volatile sig_atomic_t ttou_received;
     +
    -+static void write_msg(const char *msg)
    ++static void write_err(const char *msg)
     +{
    ++	write_in_full(2, "error: ", strlen("error: "));
     +	write_in_full(2, msg, strlen(msg));
     +	write_in_full(2, "\n", 1);
     +}
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
     +	struct sigaction sa = { .sa_handler = SIG_DFL };
     +
     +	ttou_received = 1;
    -+	write_msg(background_resume_msg);
    ++	write_err(background_resume_msg);
     +	sigaction(signo, &sa, &old_sa);
     +	raise(signo);
     +	sigemptyset(&mask);
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
     +		can_restore = 0;
     +
     +	if (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)
    -+		write_msg(restore_error_msg);
    ++		write_err(restore_error_msg);
     +
     +	sigaction(signo, &sa, &old_sa);
     +	raise(signo);
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
     +	sigprocmask(SIG_BLOCK, &mask, NULL);
     +	sigaction(signo, &old_sa, NULL);
     +	if (!can_restore) {
    -+		write_msg(restore_error_msg);
    ++		write_err(restore_error_msg);
     +		goto out;
     +	}
     +	/*
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
     +	if (ttou_received)
     +		goto again;
     +	else if (res < 0)
    -+		write_msg(restore_error_msg);
    ++		write_err(restore_error_msg);
     +	sigaction(SIGTTOU, &old_sa, NULL);
     + out:
     +	errno = saved_errno;
    @@ compat/terminal.c: void restore_term(void)
     +		signal(SIGTTIN, SIG_DFL);
     +		signal(SIGTTOU, SIG_DFL);
     +		signal(SIGTSTP, SIG_DFL);
    -+		FREE_AND_NULL(restore_error_msg);
    -+		FREE_AND_NULL(background_resume_msg);
    ++		restore_error_msg = NULL;
    ++		background_resume_msg = NULL;
     +	}
      }
      
    - int save_term(unsigned flags)
    + int save_term(enum save_term_flags flags)
      {
     +	struct sigaction sa;
     +
      	if (term_fd < 0)
      		term_fd = (flags & SAVE_TERM_STDIN) ? 0
      						    : open("/dev/tty", O_RDWR);
    -@@ compat/terminal.c: int save_term(unsigned flags)
    +@@ 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);
    @@ compat/terminal.c: int save_term(unsigned flags)
     +		return 0;
     +
     +	/* avoid calling gettext() from signal handler */
    -+	background_resume_msg = xstrdup(_("error: cannot resume in the background"));
    -+	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
    ++	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);
-- 
2.35.1

[PATCH v2 1/4] terminal: use flags for save_term()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The next commit will add another flag in addition to the existing
full_duplex so change the function signature to take a flags
argument. Also alter the functions that call save_term() so that they
can pass flags down to it.

The choice to use an enum for tho bitwise flags is because gdb will
display the symbolic names of all the flags that are set rather than
the integer value.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 41 +++++++++++++++++++++--------------------
 compat/terminal.h |  7 ++++++-
 2 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index d882dfa06e..9392b068b1 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -34,7 +34,7 @@ void restore_term(void)
 	sigchain_pop_common();
 }
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
 	if (term_fd < 0)
 		term_fd = open("/dev/tty", O_RDWR);
@@ -47,11 +47,11 @@ int save_term(int full_duplex)
 	return 0;
 }
 
-static int disable_bits(tcflag_t bits)
+static int disable_bits(enum save_term_flags flags, tcflag_t bits)
 {
 	struct termios t;
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		goto error;
 
 	t = old_term;
@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)
 	return -1;
 }
 
-static int disable_echo(void)
+static int disable_echo(enum save_term_flags flags)
 {
-	return disable_bits(ECHO);
+	return disable_bits(flags, ECHO);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(enum save_term_flags flags)
 {
-	return disable_bits(ICANON | ECHO);
+	return disable_bits(flags, ICANON | ECHO);
 }
 
 #elif defined(GIT_WINDOWS_NATIVE)
@@ -126,15 +126,15 @@ void restore_term(void)
 	hconin = hconout = INVALID_HANDLE_VALUE;
 }
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
 	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
 	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
 	    FILE_ATTRIBUTE_NORMAL, NULL);
 	if (hconin == INVALID_HANDLE_VALUE)
 		return -1;
 
-	if (full_duplex) {
+	if (flags & SAVE_TERM_DUPLEX) {
 		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
 			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
 			FILE_ATTRIBUTE_NORMAL, NULL);
@@ -154,7 +154,7 @@ int save_term(int full_duplex)
 	return -1;
 }
 
-static int disable_bits(DWORD bits)
+static int disable_bits(enum save_term_flags flags, DWORD bits)
 {
 	if (use_stty) {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)
 		use_stty = 0;
 	}
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		return -1;
 
 	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)
 	return 0;
 }
 
-static int disable_echo(void)
+static int disable_echo(enum save_term_flags flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT);
+	return disable_bits(flags, ENABLE_ECHO_INPUT);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(enum save_term_flags flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
+	return disable_bits(flags,
+			    ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
 }
 
 /*
@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
 		return NULL;
 	}
 
-	if (!echo && disable_echo()) {
+	if (!echo && disable_echo(0)) {
 		fclose(input_fh);
 		fclose(output_fh);
 		return NULL;
@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical() < 0) {
+	if (warning_displayed || enable_non_canonical(0) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)
 
 #else
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
-	/* full_duplex == 1, but no support available */
-	return -full_duplex;
+	/* no duplex support available */
+	return -!!(flags & SAVE_TERM_DUPLEX);
 }
 
 void restore_term(void)
diff --git a/compat/terminal.h b/compat/terminal.h
index 0fb9fa147c..24c4df4c0e 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,14 +1,19 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+enum save_term_flags {
+	/* Save input and output settings */
+	SAVE_TERM_DUPLEX = 1 << 0,
+};
+
 /*
  * Save the terminal attributes so they can be restored later by a
  * call to restore_term(). Note that every successful call to
  * save_term() must be matched by a call to restore_term() even if the
  * attributes have not been changed. Returns 0 on success, -1 on
  * failure.
  */
-int save_term(int full_duplex);
+int save_term(unsigned flags);
 /* Restore the terminal attributes that were saved with save_term() */
 void restore_term(void);
 
-- 
2.35.1

Re: [PATCH v2 1/4] terminal: use flags for save_term()

[Carlo Arenas]

On Wed, Mar 9, 2022 at 3:04 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> diff --git a/compat/terminal.h b/compat/terminal.h
> index 0fb9fa147c..24c4df4c0e 100644
> --- a/compat/terminal.h
> +++ b/compat/terminal.h
> @@ -1,14 +1,19 @@
>  #ifndef COMPAT_TERMINAL_H
>  #define COMPAT_TERMINAL_H
>
> +enum save_term_flags {
> +       /* Save input and output settings */
> +       SAVE_TERM_DUPLEX = 1 << 0,
> +};
> +
>  /*
>   * Save the terminal attributes so they can be restored later by a
>   * call to restore_term(). Note that every successful call to
>   * save_term() must be matched by a call to restore_term() even if the
>   * attributes have not been changed. Returns 0 on success, -1 on
>   * failure.
>   */
> -int save_term(int full_duplex);
> +int save_term(unsigned flags);

s/unsigned/enum save_term_flags/

Carlo

Re: [PATCH v2 1/4] terminal: use flags for save_term()

[Phillip Wood]

On 11/03/2022 16:52, Carlo Arenas wrote:
> On Wed, Mar 9, 2022 at 3:04 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> diff --git a/compat/terminal.h b/compat/terminal.h
>> index 0fb9fa147c..24c4df4c0e 100644
>> --- a/compat/terminal.h
>> +++ b/compat/terminal.h
>> @@ -1,14 +1,19 @@
>>   #ifndef COMPAT_TERMINAL_H
>>   #define COMPAT_TERMINAL_H
>>
>> +enum save_term_flags {
>> +       /* Save input and output settings */
>> +       SAVE_TERM_DUPLEX = 1 << 0,
>> +};
>> +
>>   /*
>>    * Save the terminal attributes so they can be restored later by a
>>    * call to restore_term(). Note that every successful call to
>>    * save_term() must be matched by a call to restore_term() even if the
>>    * attributes have not been changed. Returns 0 on success, -1 on
>>    * failure.
>>    */
>> -int save_term(int full_duplex);
>> +int save_term(unsigned flags);
> 
> s/unsigned/enum save_term_flags/

Well spotted!

Thanks

Phillip

> Carlo

[PATCH v2 2/4] terminal: don't assume stdin is /dev/tty

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

read_key_without_echo() reads from stdin but uses /dev/tty when it
disables echo. This is unfortunate as there no guarantee that stdin is
the same device as /dev/tty. The perl version of "add -p" uses stdin
when it sets the terminal mode, this commit does the same for the
builtin version. There is still a difference between the perl and
builtin versions though - the perl version will ignore any errors when
setting the terminal mode[1] and will still read single bytes when
stdin is not a terminal. The builtin version displays a warning if
setting the terminal mode fails and switches to reading a line at a
time.

[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 5 +++--
 compat/terminal.h | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 9392b068b1..cb653419c3 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -37,7 +37,8 @@ void restore_term(void)
 int save_term(enum save_term_flags flags)
 {
 	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 (tcgetattr(term_fd, &old_term) < 0)
@@ -362,7 +363,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical(0) < 0) {
+	if (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
diff --git a/compat/terminal.h b/compat/terminal.h
index 24c4df4c0e..3217611b08 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -4,6 +4,8 @@
 enum save_term_flags {
 	/* Save input and output settings */
 	SAVE_TERM_DUPLEX = 1 << 0,
+	/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */
+	SAVE_TERM_STDIN  = 1 << 1,
 };
 
 /*
-- 
2.35.1

[PATCH v2 3/4] terminal: work around macos poll() bug

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

On macos the builtin "add -p" does not handle keys that generate
escape sequences because poll() does not work with terminals
there. Switch to using select() on non-windows platforms to work
around this.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index cb653419c3..4189cbb12c 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -82,6 +82,32 @@ static int enable_non_canonical(enum save_term_flags flags)
 	return disable_bits(flags, ICANON | ECHO);
 }
 
+/*
+ * On macos it is not possible to use poll() with a terminal so use select
+ * instead.
+ */
+#include <sys/select.h>
+static int getchar_with_timeout(int timeout)
+{
+	struct timeval tv, *tvp = NULL;
+	fd_set readfds;
+	int res;
+
+	if (timeout >= 0) {
+		tv.tv_sec = timeout / 1000;
+		tv.tv_usec = (timeout % 1000) * 1000;
+		tvp = &tv;
+	}
+
+	FD_ZERO(&readfds);
+	FD_SET(0, &readfds);
+	res = select(1, &readfds, NULL, NULL, tvp);
+	if (res < 0)
+		return EOF;
+
+	return getchar();
+}
+
 #elif defined(GIT_WINDOWS_NATIVE)
 
 #define INPUT_PATH "CONIN$"
@@ -247,6 +273,16 @@ static int mingw_getchar(void)
 }
 #define getchar mingw_getchar
 
+static int getchar_with_timeout(int timeout)
+{
+	struct pollfd pfd = { .fd = 0, .events = POLLIN };
+
+	if (poll(&pfd, 1, timeout) < 1)
+		return EOF;
+
+	return getchar();
+}
+
 #endif
 
 #ifndef FORCE_TEXT
@@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)
 		 * half a second when we know that the sequence is complete.
 		 */
 		while (!is_known_escape_sequence(buf->buf)) {
-			struct pollfd pfd = { .fd = 0, .events = POLLIN };
-
-			if (poll(&pfd, 1, 500) < 1)
-				break;
-
-			ch = getchar();
+			ch = getchar_with_timeout(500);
 			if (ch == EOF)
 				return 0;
 			strbuf_addch(buf, ch);
-- 
2.35.1

Re: [PATCH v2 3/4] terminal: work around macos poll() bug

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 09 2022, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> On macos the builtin "add -p" does not handle keys that generate
> escape sequences because poll() does not work with terminals
> there. Switch to using select() on non-windows platforms to work
> around this.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  compat/terminal.c | 43 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/compat/terminal.c b/compat/terminal.c
> index cb653419c3..4189cbb12c 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -82,6 +82,32 @@ static int enable_non_canonical(enum save_term_flags flags)
>  	return disable_bits(flags, ICANON | ECHO);
>  }
>  
> +/*
> + * On macos it is not possible to use poll() with a terminal so use select
> + * instead.
> + */
> +#include <sys/select.h>

I don't think this breaks anything in practice due to the platforms we
define HAVE_DEV_TTY and NO_SYS_SELECT_H on, but it does look redundant &
confusing, and will break if the current users of HAVE_DEV_TTY and
NO_SYS_SELECT_H change.

I.e. isn't sys/select.h already pulled in by the relevant include in
git-compat-util.h? Why is it needed again here?

Re: [PATCH v2 3/4] terminal: work around macos poll() bug

[Phillip Wood]

Hi Ævar

On 10/03/2022 13:35, Ævar Arnfjörð Bjarmason wrote:
> [...]
>>   
>> +/*
>> + * On macos it is not possible to use poll() with a terminal so use select
>> + * instead.
>> + */
>> +#include <sys/select.h>
> 
> I don't think this breaks anything in practice due to the platforms we
> define HAVE_DEV_TTY and NO_SYS_SELECT_H on, but it does look redundant &
> confusing, and will break if the current users of HAVE_DEV_TTY and
> NO_SYS_SELECT_H change.
> 
> I.e. isn't sys/select.h already pulled in by the relevant include in
> git-compat-util.h? Why is it needed again here?

I didn't realize that git-compat-util.h already included that header, 
I'll remove it.

Thanks

Phillip

Re: [PATCH v2 3/4] terminal: work around macos poll() bug

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Ævar
>
> On 10/03/2022 13:35, Ævar Arnfjörð Bjarmason wrote:
>> [...]
>>>   +/*
>>> + * On macos it is not possible to use poll() with a terminal so use select
>>> + * instead.
>>> + */
>>> +#include <sys/select.h>
>> I don't think this breaks anything in practice due to the platforms
>> we
>> define HAVE_DEV_TTY and NO_SYS_SELECT_H on, but it does look redundant &
>> confusing, and will break if the current users of HAVE_DEV_TTY and
>> NO_SYS_SELECT_H change.
>> I.e. isn't sys/select.h already pulled in by the relevant include in
>> git-compat-util.h? Why is it needed again here?
>
> I didn't realize that git-compat-util.h already included that header,
> I'll remove it.

Thanks, both.  The removal of that line is very much appreciated.

[PATCH v2 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user suspends git while it is waiting for a keypress reset the
terminal before stopping and restore the settings when git resumes. If
the user tries to resume in the background print an error
message (taking care to use async safe functions) before stopping
again. Ideally we would reprint the prompt for the user when git
resumes but this patch just restarts the read().

The signal handler is established with sigaction() rather than using
sigchain_push() as this allows us to control the signal mask when the
handler is invoked and ensure SA_RESTART is used to restart the
read() when resuming.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 125 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 121 insertions(+), 4 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 4189cbb12c..f425f7b339 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -1,4 +1,4 @@
-#include "git-compat-util.h"
+#include "cache.h"
 #include "compat/terminal.h"
 #include "sigchain.h"
 #include "strbuf.h"
@@ -23,6 +23,90 @@ static void restore_term_on_signal(int sig)
 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;
+
+static void write_err(const char *msg)
+{
+	write_in_full(2, "error: ", strlen("error: "));
+	write_in_full(2, msg, strlen(msg));
+	write_in_full(2, "\n", 1);
+}
+
+static void print_background_resume_msg(int signo)
+{
+	int saved_errno = errno;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+
+	ttou_received = 1;
+	write_err(background_resume_msg);
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	errno = saved_errno;
+}
+
+static void restore_terminal_on_suspend(int signo)
+{
+	int saved_errno = errno;
+	int res;
+	struct termios t;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+	int can_restore = 1;
+
+	if (tcgetattr(term_fd, &t) < 0)
+		can_restore = 0;
+
+	if (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)
+		write_err(restore_error_msg);
+
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	if (!can_restore) {
+		write_err(restore_error_msg);
+		goto out;
+	}
+	/*
+	 * If we resume in the background then we receive SIGTTOU when calling
+	 * tcsetattr() below. Set up a handler to print an error message in that
+	 * case.
+	 */
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGTTOU);
+	sa.sa_mask = old_sa.sa_mask;
+	sa.sa_handler = print_background_resume_msg;
+	sa.sa_flags = SA_RESTART;
+	sigaction(SIGTTOU, &sa, &old_sa);
+ again:
+	ttou_received = 0;
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	res = tcsetattr(term_fd, TCSAFLUSH, &t);
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	if (ttou_received)
+		goto again;
+	else if (res < 0)
+		write_err(restore_error_msg);
+	sigaction(SIGTTOU, &old_sa, NULL);
+ out:
+	errno = saved_errno;
+}
+
 void restore_term(void)
 {
 	if (term_fd < 0)
@@ -32,10 +116,19 @@ void restore_term(void)
 	close(term_fd);
 	term_fd = -1;
 	sigchain_pop_common();
+	if (restore_error_msg) {
+		signal(SIGTTIN, SIG_DFL);
+		signal(SIGTTOU, SIG_DFL);
+		signal(SIGTSTP, SIG_DFL);
+		restore_error_msg = NULL;
+		background_resume_msg = NULL;
+	}
 }
 
 int save_term(enum save_term_flags flags)
 {
+	struct sigaction sa;
+
 	if (term_fd < 0)
 		term_fd = (flags & SAVE_TERM_STDIN) ? 0
 						    : open("/dev/tty", O_RDWR);
@@ -44,6 +137,26 @@ 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
+	 * 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;
 }
@@ -93,6 +206,7 @@ static int getchar_with_timeout(int timeout)
 	fd_set readfds;
 	int res;
 
+ again:
 	if (timeout >= 0) {
 		tv.tv_sec = timeout / 1000;
 		tv.tv_usec = (timeout % 1000) * 1000;
@@ -102,9 +216,12 @@ static int getchar_with_timeout(int timeout)
 	FD_ZERO(&readfds);
 	FD_SET(0, &readfds);
 	res = select(1, &readfds, NULL, NULL, tvp);
-	if (res < 0)
-		return EOF;
-
+	if (res < 0) {
+		if (errno == EINTR)
+			goto again;
+		else
+			return EOF;
+	}
 	return getchar();
 }
 
-- 
2.35.1

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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'

Is it still true, or does the base only apply to v1?

$ git checkout --detach pw/single-key-interactive
HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set
$ git am -s ./+pw4-v2-add-p-single
Applying: terminal: use flags for save_term()
Applying: terminal: don't assume stdin is /dev/tty
Applying: terminal: work around macos poll() bug
error: patch failed: compat/terminal.c:397
error: compat/terminal.c: patch does not apply
Patch failed at 0003 terminal: work around macos poll() bug
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
$ exit

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> These patches are based on 'pw/single-key-interactive'
>
> Is it still true, or does the base only apply to v1?
>
> $ git checkout --detach pw/single-key-interactive
> HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set
> $ git am -s ./+pw4-v2-add-p-single
> Applying: terminal: use flags for save_term()
> Applying: terminal: don't assume stdin is /dev/tty
> Applying: terminal: work around macos poll() bug
> error: patch failed: compat/terminal.c:397
> error: compat/terminal.c: patch does not apply
> Patch failed at 0003 terminal: work around macos poll() bug
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> $ exit

I think I figured it out.  A merge of pw/single-key-interactive into
a recent tip of 'master' wants the "return 0" in the preimage below
to be "break" in compat/terminal.c


> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)
>  		 * half a second when we know that the sequence is complete.
>  		 */
>  		while (!is_known_escape_sequence(buf->buf)) {
> -			struct pollfd pfd = { .fd = 0, .events = POLLIN };
> -
> -			if (poll(&pfd, 1, 500) < 1)
> -				break;
> -
> -			ch = getchar();
> +			ch = getchar_with_timeout(500);
>  			if (ch == EOF)
>  				return 0;
>  			strbuf_addch(buf, ch);

I'll wiggle it in to see how well it fares with other topics in
flight.

Thanks.

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

[Phillip Wood]

Hi Junio

On 09/03/2022 23:37, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>>> These patches are based on 'pw/single-key-interactive'
>>
>> Is it still true, or does the base only apply to v1?

The base is unchanged but does not seem to match
pw/single-key-interactive. I'm not sure what happened there. They are
based on 300db53b37 ("add -p: disable stdin buffering when
interactive.singlekey is set", 2022-02-16) which is the second parent
of e53fb7aa3f ("Merge branch 'pw/single-key-interactive' into seen",
2022-02-20)

>> $ git checkout --detach pw/single-key-interactive
>> HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set
>> $ git am -s ./+pw4-v2-add-p-single
>> Applying: terminal: use flags for save_term()
>> Applying: terminal: don't assume stdin is /dev/tty
>> Applying: terminal: work around macos poll() bug
>> error: patch failed: compat/terminal.c:397
>> error: compat/terminal.c: patch does not apply
>> Patch failed at 0003 terminal: work around macos poll() bug
>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>> When you have resolved this problem, run "git am --continue".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort".
>> $ exit
> 
> I think I figured it out.  A merge of pw/single-key-interactive into
> a recent tip of 'master' wants the "return 0" in the preimage below
> to be "break" in compat/terminal.c
> 
> 
>> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)
>>   		 * half a second when we know that the sequence is complete.
>>   		 */
>>   		while (!is_known_escape_sequence(buf->buf)) {
>> -			struct pollfd pfd = { .fd = 0, .events = POLLIN };
>> -
>> -			if (poll(&pfd, 1, 500) < 1)
>> -				break;
>> -
>> -			ch = getchar();
>> +			ch = getchar_with_timeout(500);
>>   			if (ch == EOF)
>>   				return 0;
>>   			strbuf_addch(buf, ch);

That looks good to me. However unfortunately there are some semantic
conflicts as well. The patch below is based on 6b1f77214c ("Merge branch
'pw/add-p-single-key' into seen", 2022-03-09), hopefully Thunderbird wont
mangle it. Whilst preparing the fixup I realized I need to reroll to
fix a closing stdin in patch 2 and resetting the job signals on error in
patch 4. What's the best base to use when rerolling?

Best Wishes

Phillip

---- >8 ----

From: Phillip Wood <phillip.wood@dunelm.org.uk>
Date: Thu, 10 Mar 2022 11:05:26 +0000
Subject: [PATCH] fixup! Merge branch 'pw/add-p-single-key' into seen

---
  compat/terminal.c | 3 ++-
  editor.c          | 2 +-
  2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 3172f4f408..d1ed5c07dc 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -141,7 +141,8 @@ int save_term(enum save_term_flags flags)
         if (term_fd < 0)
                 return -1;
         if ((flags & SAVE_TERM_DUPLEX) && !is_controlling_terminal(term_fd)) {
-               close(term_fd);
+               if (term_fd) /* avoid closing stdin */
+                       close(term_fd);
                 term_fd = -1;
                 return -1;
         }
diff --git a/editor.c b/editor.c
index 6c5c95e6a2..192d6ea75d 100644
--- a/editor.c
+++ b/editor.c
@@ -55,7 +55,7 @@ static int prepare_term(const char *editor)
  
         git_config_get_bool("editor.stty", &need_saverestore);
         if (need_saverestore)
-               return save_term(1);
+               return save_term(SAVE_TERM_DUPLEX);
         return 0;
  }
  
-- 
2.33.0.342.g580a6b0edd

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

[Phillip Wood]

On 10/03/2022 13:28, Phillip Wood wrote:
> Hi Junio
> 
> On 09/03/2022 23:37, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> $ git checkout --detach pw/single-key-interactive
>>> HEAD is now at ac618c418e add -p: disable stdin buffering when 
>>> interactive.singlekey is set
>>> $ git am -s ./+pw4-v2-add-p-single
>>> Applying: terminal: use flags for save_term()
>>> Applying: terminal: don't assume stdin is /dev/tty
>>> Applying: terminal: work around macos poll() bug
>>> error: patch failed: compat/terminal.c:397
>>> error: compat/terminal.c: patch does not apply
>>> Patch failed at 0003 terminal: work around macos poll() bug
>>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>>> When you have resolved this problem, run "git am --continue".
>>> If you prefer to skip this patch, run "git am --skip" instead.
>>> To restore the original branch and stop patching, run "git am --abort".
>>> $ exit
>>
>> I think I figured it out.  A merge of pw/single-key-interactive into
>> a recent tip of 'master' wants the "return 0" in the preimage below
>> to be "break" in compat/terminal.c
>>
>>
>>> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)
>>>            * half a second when we know that the sequence is complete.
>>>            */
>>>           while (!is_known_escape_sequence(buf->buf)) {
>>> -            struct pollfd pfd = { .fd = 0, .events = POLLIN };
>>> -
>>> -            if (poll(&pfd, 1, 500) < 1)
>>> -                break;
>>> -
>>> -            ch = getchar();
>>> +            ch = getchar_with_timeout(500);
>>>               if (ch == EOF)
>>>                   return 0;

Looking more closely that should be "break" not "return 0". I think what 
has happened is that I accidentally based these on an old version of 
pw/single-key-interactive which did not contain 24d7ce383a ("terminal: 
always reset terminal when reading without echo", 2022-02-22)

Sorry for the confusion

Phillip

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Looking more closely that should be "break" not "return 0". I think
> what has happened is that I accidentally based these on an old version
> of pw/single-key-interactive which did not contain 24d7ce383a
> ("terminal: always reset terminal when reading without echo",
> 2022-02-22)

I thought the base pw/single-key-interactive topic is solid enough
and ready for 'next'?  It probably is a good idea to rebase these
follow-on patches on top of a merge of the base topic into 'master'.

I'll keep v2 in 'seen' but mark the topic as "Expecting a reroll"
in the draft of the next issue of the "What's cooking" report.

Thanks.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

Hi Phillip,

On Wed, 9 Mar 2022, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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

While I did not ask for any of these changes, they look good to me. I had
a look over the range-diff and found it reasonable.

Onwards to a bright built-in `add -p` future!

Thanks,
Dscho

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

[Phillip Wood]

Hi Dscho

On 10/03/2022 13:25, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 9 Mar 2022, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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
> 
> While I did not ask for any of these changes, they look good to me. I had
> a look over the range-diff and found it reasonable.
> 
> Onwards to a bright built-in `add -p` future!

Hopefully! I spotted a couple more small issues earlier so there will be 
a v3 soonish

Best Wishes

Phillip

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

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 | 226 +++++++++++++++++++++++++++++++++++++++-------
 compat/terminal.h |   9 +-
 2 files changed, 202 insertions(+), 33 deletions(-)

Range-diff against v2:
1:  fac5a0f5b7 ! 1:  2c4c5424d7 terminal: use flags for save_term()
    @@ compat/terminal.h
       * failure.
       */
     -int save_term(int full_duplex);
    -+int save_term(unsigned flags);
    ++int save_term(enum save_term_flags flags);
      /* Restore the terminal attributes that were saved with save_term() */
      void restore_term(void);
      
2:  bf29460ec6 ! 2:  cf9275c40c terminal: don't assume stdin is /dev/tty
    @@ Commit message
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## compat/terminal.c ##
    -@@ compat/terminal.c: void restore_term(void)
    +@@ compat/terminal.c: static void restore_term_on_signal(int sig)
    + static int term_fd = -1;
    + static struct termios old_term;
    + 
    ++static void close_term_fd(void)
    ++{
    ++	if (term_fd)
    ++		close(term_fd);
    ++	term_fd = -1;
    ++}
    ++
    + void restore_term(void)
    + {
    + 	if (term_fd < 0)
    + 		return;
    + 
    + 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
    +-	close(term_fd);
    +-	term_fd = -1;
    ++	close_term_fd();
    + 	sigchain_pop_common();
    + }
    + 
      int save_term(enum save_term_flags flags)
      {
      	if (term_fd < 0)
    @@ compat/terminal.c: void restore_term(void)
      	if (term_fd < 0)
      		return -1;
      	if (tcgetattr(term_fd, &old_term) < 0)
    +@@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)
    + 
    + 	sigchain_pop_common();
    + error:
    +-	close(term_fd);
    +-	term_fd = -1;
    ++	close_term_fd();
    + 	return -1;
    + }
    + 
     @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)
      	static int warning_displayed;
      	int ch;
3:  1815606e82 ! 3:  4828d63ce5 terminal: work around macos poll() bug
    @@ compat/terminal.c: static int enable_non_canonical(enum save_term_flags flags)
     + * On macos it is not possible to use poll() with a terminal so use select
     + * instead.
     + */
    -+#include <sys/select.h>
     +static int getchar_with_timeout(int timeout)
     +{
     +	struct timeval tv, *tvp = NULL;
    @@ compat/terminal.c: static int enable_non_canonical(enum save_term_flags flags)
     +	FD_ZERO(&readfds);
     +	FD_SET(0, &readfds);
     +	res = select(1, &readfds, NULL, NULL, tvp);
    -+	if (res < 0)
    ++	if (res <= 0)
     +		return EOF;
     +
     +	return getchar();
    @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)
     -			ch = getchar();
     +			ch = getchar_with_timeout(500);
      			if (ch == EOF)
    - 				return 0;
    + 				break;
      			strbuf_addch(buf, ch);
4:  dd0e1fabb1 ! 4:  3185bc5223 terminal: restore settings on SIGTSTP
    @@ compat/terminal.c: static void restore_term_on_signal(int sig)
     +	errno = saved_errno;
     +}
     +
    - void restore_term(void)
    - {
    - 	if (term_fd < 0)
    -@@ compat/terminal.c: void restore_term(void)
    - 	close(term_fd);
    - 	term_fd = -1;
    - 	sigchain_pop_common();
    ++static void reset_job_signals(void)
    ++{
     +	if (restore_error_msg) {
     +		signal(SIGTTIN, SIG_DFL);
     +		signal(SIGTTOU, SIG_DFL);
     +		signal(SIGTSTP, SIG_DFL);
     +		restore_error_msg = NULL;
     +		background_resume_msg = NULL;
     +	}
    ++}
    ++
    + static void close_term_fd(void)
    + {
    + 	if (term_fd)
    +@@ compat/terminal.c: void restore_term(void)
    + 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
    + 	close_term_fd();
    + 	sigchain_pop_common();
    ++	reset_job_signals();
      }
      
      int save_term(enum save_term_flags flags)
    @@ compat/terminal.c: int save_term(enum save_term_flags flags)
      
      	return 0;
      }
    +@@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)
    + 		return 0;
    + 
    + 	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;
    @@ compat/terminal.c: static int getchar_with_timeout(int timeout)
      	FD_ZERO(&readfds);
      	FD_SET(0, &readfds);
      	res = select(1, &readfds, NULL, NULL, tvp);
    --	if (res < 0)
    --		return EOF;
    +-	if (res <= 0)
    ++	if (!res)
    + 		return EOF;
     -
     +	if (res < 0) {
     +		if (errno == EINTR)

Semantic conflict resolution:
---- >8 ----
diff --git a/compat/terminal.c b/compat/terminal.c
index bffb1340ab..9a64b9a431 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -150,9 +150,8 @@ int save_term(enum save_term_flags flags)
                                                    : 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;
+       if (flags & SAVE_TERM_DUPLEX && !is_controlling_terminal(term_fd)) {
+               close_term_fd();
                return -1;
        }
        if (tcgetattr(term_fd, &old_term) < 0)
diff --git a/editor.c b/editor.c
index 6c5c95e6a2..192d6ea75d 100644
--- a/editor.c
+++ b/editor.c
@@ -55,7 +55,7 @@ static int prepare_term(const char *editor)

        git_config_get_bool("editor.stty", &need_saverestore);
        if (need_saverestore)
-               return save_term(1);
+               return save_term(SAVE_TERM_DUPLEX);
        return 0;
 }

-- 
2.35.1

[PATCH v3 1/4] terminal: use flags for save_term()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The next commit will add another flag in addition to the existing
full_duplex so change the function signature to take a flags
argument. Also alter the functions that call save_term() so that they
can pass flags down to it.

The choice to use an enum for tho bitwise flags is because gdb will
display the symbolic names of all the flags that are set rather than
the integer value.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 41 +++++++++++++++++++++--------------------
 compat/terminal.h |  7 ++++++-
 2 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 3620184e79..da2f788137 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -34,7 +34,7 @@ void restore_term(void)
 	sigchain_pop_common();
 }
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
 	if (term_fd < 0)
 		term_fd = open("/dev/tty", O_RDWR);
@@ -47,11 +47,11 @@ int save_term(int full_duplex)
 	return 0;
 }
 
-static int disable_bits(tcflag_t bits)
+static int disable_bits(enum save_term_flags flags, tcflag_t bits)
 {
 	struct termios t;
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		goto error;
 
 	t = old_term;
@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)
 	return -1;
 }
 
-static int disable_echo(void)
+static int disable_echo(enum save_term_flags flags)
 {
-	return disable_bits(ECHO);
+	return disable_bits(flags, ECHO);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(enum save_term_flags flags)
 {
-	return disable_bits(ICANON | ECHO);
+	return disable_bits(flags, ICANON | ECHO);
 }
 
 #elif defined(GIT_WINDOWS_NATIVE)
@@ -126,15 +126,15 @@ void restore_term(void)
 	hconin = hconout = INVALID_HANDLE_VALUE;
 }
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
 	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
 	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
 	    FILE_ATTRIBUTE_NORMAL, NULL);
 	if (hconin == INVALID_HANDLE_VALUE)
 		return -1;
 
-	if (full_duplex) {
+	if (flags & SAVE_TERM_DUPLEX) {
 		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
 			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
 			FILE_ATTRIBUTE_NORMAL, NULL);
@@ -154,7 +154,7 @@ int save_term(int full_duplex)
 	return -1;
 }
 
-static int disable_bits(DWORD bits)
+static int disable_bits(enum save_term_flags flags, DWORD bits)
 {
 	if (use_stty) {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)
 		use_stty = 0;
 	}
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		return -1;
 
 	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)
 	return 0;
 }
 
-static int disable_echo(void)
+static int disable_echo(enum save_term_flags flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT);
+	return disable_bits(flags, ENABLE_ECHO_INPUT);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(enum save_term_flags flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
+	return disable_bits(flags,
+			    ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
 }
 
 /*
@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
 		return NULL;
 	}
 
-	if (!echo && disable_echo()) {
+	if (!echo && disable_echo(0)) {
 		fclose(input_fh);
 		fclose(output_fh);
 		return NULL;
@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical() < 0) {
+	if (warning_displayed || enable_non_canonical(0) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)
 
 #else
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
-	/* full_duplex == 1, but no support available */
-	return -full_duplex;
+	/* no duplex support available */
+	return -!!(flags & SAVE_TERM_DUPLEX);
 }
 
 void restore_term(void)
diff --git a/compat/terminal.h b/compat/terminal.h
index 0fb9fa147c..aeb24c9478 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,14 +1,19 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+enum save_term_flags {
+	/* Save input and output settings */
+	SAVE_TERM_DUPLEX = 1 << 0,
+};
+
 /*
  * Save the terminal attributes so they can be restored later by a
  * call to restore_term(). Note that every successful call to
  * save_term() must be matched by a call to restore_term() even if the
  * attributes have not been changed. Returns 0 on success, -1 on
  * failure.
  */
-int save_term(int full_duplex);
+int save_term(enum save_term_flags flags);
 /* Restore the terminal attributes that were saved with save_term() */
 void restore_term(void);
 
-- 
2.35.1

[PATCH v3 2/4] terminal: don't assume stdin is /dev/tty

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

read_key_without_echo() reads from stdin but uses /dev/tty when it
disables echo. This is unfortunate as there no guarantee that stdin is
the same device as /dev/tty. The perl version of "add -p" uses stdin
when it sets the terminal mode, this commit does the same for the
builtin version. There is still a difference between the perl and
builtin versions though - the perl version will ignore any errors when
setting the terminal mode[1] and will still read single bytes when
stdin is not a terminal. The builtin version displays a warning if
setting the terminal mode fails and switches to reading a line at a
time.

[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 18 ++++++++++++------
 compat/terminal.h |  2 ++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index da2f788137..bfbde3c1af 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig)
 static int term_fd = -1;
 static struct termios old_term;
 
+static void close_term_fd(void)
+{
+	if (term_fd)
+		close(term_fd);
+	term_fd = -1;
+}
+
 void restore_term(void)
 {
 	if (term_fd < 0)
 		return;
 
 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
-	close(term_fd);
-	term_fd = -1;
+	close_term_fd();
 	sigchain_pop_common();
 }
 
 int save_term(enum save_term_flags flags)
 {
 	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 (tcgetattr(term_fd, &old_term) < 0)
@@ -66,8 +73,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)
 
 	sigchain_pop_common();
 error:
-	close(term_fd);
-	term_fd = -1;
+	close_term_fd();
 	return -1;
 }
 
@@ -362,7 +368,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical(0) < 0) {
+	if (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
diff --git a/compat/terminal.h b/compat/terminal.h
index aeb24c9478..79ed00cf61 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -4,6 +4,8 @@
 enum save_term_flags {
 	/* Save input and output settings */
 	SAVE_TERM_DUPLEX = 1 << 0,
+	/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */
+	SAVE_TERM_STDIN  = 1 << 1,
 };
 
 /*
-- 
2.35.1

Re: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> diff --git a/compat/terminal.c b/compat/terminal.c
> index da2f788137..bfbde3c1af 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig)
>  static int term_fd = -1;

The variable can be -1 to signal "no valid file descriptor".

>  static struct termios old_term;
>  
> +static void close_term_fd(void)
> +{
> +	if (term_fd)
> +		close(term_fd);
> +	term_fd = -1;
> +}
> +

And we use that negative value after closing it.

The check before closing it is wrong.  It should be

	if (0 <= term_fd)

instead.  Or we could mimick the beginning of restore_term(), i.e.

	if (term_fd < 0)
		return;
	close(term_fd);
	term_fd = -1;

>  void restore_term(void)
>  {
>  	if (term_fd < 0)
>  		return;
>  
>  	tcsetattr(term_fd, TCSAFLUSH, &old_term);
> -	close(term_fd);
> -	term_fd = -1;
> +	close_term_fd();

Because we come this far only when term_fd is valid, this change is
a no-op.  If we are adding more calls to close_term_fd(), it may be
a good abstraction.

>  	sigchain_pop_common();
>  }
>  
>  int save_term(enum save_term_flags flags)
>  {
>  	if (term_fd < 0)
> -		term_fd = open("/dev/tty", O_RDWR);
> +		term_fd = (flags & SAVE_TERM_STDIN) ? 0
> +						    : open("/dev/tty", O_RDWR);

We can avoid overly long line by wrapping differently:

		term_fd = ((flags & SAVE_TERM_STDIN)
			   ? 0
			   : open("/dev/tty", O_RDWR));

We can turn our head sideways to see the parse tree this way.

> @@ -66,8 +73,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)
>  
>  	sigchain_pop_common();
>  error:
> -	close(term_fd);
> -	term_fd = -1;
> +	close_term_fd();

OK.

> @@ -362,7 +368,7 @@ int read_key_without_echo(struct strbuf *buf)
>  	static int warning_displayed;
>  	int ch;
>  
> -	if (warning_displayed || enable_non_canonical(0) < 0) {
> +	if (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {
>  		if (!warning_displayed) {
>  			warning("reading single keystrokes not supported on "
>  				"this platform; reading line instead");

The validity of this change is harder to see with only what we have
in the patch, but the control flow is

   enable_non_canonical(bits)
   -> disable_bits(bits, ...)
      -> save_term(bits)

And we are passing SAVE_TERM_STDIN to say "reuse fd #0 and save the
terminal settings of it, instead of opening /dev/tty anew".

What happens if FD#0 is *not* connected to a tty, by the way?
tcgetattr() in save_term() would fail, without clearing term_fd
(i.e. term_fd is still 0 when save_term() returns a failure), and
goes to the error code path, where close_term_fd() is called.

Because we have the "if (term_fd)" bug in save_term(), this bug is
hidden, but I think save_term() upon seeing a failure from tcgetattr()
should clear term_fd to limit the damage, especially when it is trying
to futz with caller supplied FD#0, not the tty it opened itself?

> diff --git a/compat/terminal.h b/compat/terminal.h
> index aeb24c9478..79ed00cf61 100644
> --- a/compat/terminal.h
> +++ b/compat/terminal.h
> @@ -4,6 +4,8 @@
>  enum save_term_flags {
>  	/* Save input and output settings */
>  	SAVE_TERM_DUPLEX = 1 << 0,
> +	/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */
> +	SAVE_TERM_STDIN  = 1 << 1,
>  };
>  
>  /*

RE: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty

[rsbecker]

On March 15, 2022 1:43 PM, Junio C Hamano wrote:
>Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> diff --git a/compat/terminal.c b/compat/terminal.c index
>> da2f788137..bfbde3c1af 100644
>> --- a/compat/terminal.c
>> +++ b/compat/terminal.c
>> @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig)
>> static int term_fd = -1;
>
>The variable can be -1 to signal "no valid file descriptor".
>
>>  static struct termios old_term;
>>
>> +static void close_term_fd(void)
>> +{
>> +	if (term_fd)
>> +		close(term_fd);
>> +	term_fd = -1;
>> +}
>> +
>
>And we use that negative value after closing it.
>
>The check before closing it is wrong.  It should be
>
>	if (0 <= term_fd)

Should this expression succeed if term_fd == stdin? I might be missing the point here.
--Randall

Re: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

>>The check before closing it is wrong.  It should be
>>
>>	if (0 <= term_fd)
>
> Should this expression succeed if term_fd == stdin? I might be missing the point here.

We could use "if (0 < term_fd)" to make this guard both about
avoiding to call close() on an uninitialized FD and also about
avoiding to close standard input.  I'd prefer to see them handled
separately as these live at different conceptual levels
(i.e. closing -1 is a no-no no matter what, closing 0 is bad if it
is what we did not open but what the caller supplied us via the
SAVE_TERM_STDIN bit, but it may be warranted if it was what we
obtained from an earlier call to open("/dev/tty") we did ourselves).

RE: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty

[rsbecker]

On March 15, 2022 3:05 PM, Junio C Hamano wrote:
>To: rsbecker@nexbridge.com
>Cc: 'Phillip Wood' <phillip.wood123@gmail.com>; 'Git Mailing List'
><git@vger.kernel.org>; 'Phillip Wood' <phillip.wood@dunelm.org.uk>; 'Ævar
>Arnfjörð Bjarmason' <avarab@gmail.com>; 'Carlo Arenas' <carenas@gmail.com>;
>'Johannes Schindelin' <Johannes.Schindelin@gmx.de>; 'Ramsay Jones'
><ramsay@ramsayjones.plus.com>
>Subject: Re: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty
>
><rsbecker@nexbridge.com> writes:
>
>>>The check before closing it is wrong.  It should be
>>>
>>>	if (0 <= term_fd)
>>
>> Should this expression succeed if term_fd == stdin? I might be missing the point
>here.
>
>We could use "if (0 < term_fd)" to make this guard both about avoiding to call
>close() on an uninitialized FD and also about avoiding to close standard input.  I'd
>prefer to see them handled separately as these live at different conceptual levels
>(i.e. closing -1 is a no-no no matter what, closing 0 is bad if it is what we did not
>open but what the caller supplied us via the SAVE_TERM_STDIN bit, but it may be
>warranted if it was what we obtained from an earlier call to open("/dev/tty") we
>did ourselves).

Thanks. This is one of those situations where explaining magic numbers is important. I appreciate the insight.

Regards,
Randall

[PATCH v3 3/4] terminal: work around macos poll() bug

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

On macos the builtin "add -p" does not handle keys that generate
escape sequences because poll() does not work with terminals
there. Switch to using select() on non-windows platforms to work
around this.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index bfbde3c1af..89f326adc1 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -87,6 +87,31 @@ static int enable_non_canonical(enum save_term_flags flags)
 	return disable_bits(flags, ICANON | ECHO);
 }
 
+/*
+ * On macos it is not possible to use poll() with a terminal so use select
+ * instead.
+ */
+static int getchar_with_timeout(int timeout)
+{
+	struct timeval tv, *tvp = NULL;
+	fd_set readfds;
+	int res;
+
+	if (timeout >= 0) {
+		tv.tv_sec = timeout / 1000;
+		tv.tv_usec = (timeout % 1000) * 1000;
+		tvp = &tv;
+	}
+
+	FD_ZERO(&readfds);
+	FD_SET(0, &readfds);
+	res = select(1, &readfds, NULL, NULL, tvp);
+	if (res <= 0)
+		return EOF;
+
+	return getchar();
+}
+
 #elif defined(GIT_WINDOWS_NATIVE)
 
 #define INPUT_PATH "CONIN$"
@@ -252,6 +277,16 @@ static int mingw_getchar(void)
 }
 #define getchar mingw_getchar
 
+static int getchar_with_timeout(int timeout)
+{
+	struct pollfd pfd = { .fd = 0, .events = POLLIN };
+
+	if (poll(&pfd, 1, timeout) < 1)
+		return EOF;
+
+	return getchar();
+}
+
 #endif
 
 #ifndef FORCE_TEXT
@@ -402,12 +437,7 @@ int read_key_without_echo(struct strbuf *buf)
 		 * half a second when we know that the sequence is complete.
 		 */
 		while (!is_known_escape_sequence(buf->buf)) {
-			struct pollfd pfd = { .fd = 0, .events = POLLIN };
-
-			if (poll(&pfd, 1, 500) < 1)
-				break;
-
-			ch = getchar();
+			ch = getchar_with_timeout(500);
 			if (ch == EOF)
 				break;
 			strbuf_addch(buf, ch);
-- 
2.35.1

[PATCH v3 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user suspends git while it is waiting for a keypress reset the
terminal before stopping and restore the settings when git resumes. If
the user tries to resume in the background print an error
message (taking care to use async safe functions) before stopping
again. Ideally we would reprint the prompt for the user when git
resumes but this patch just restarts the read().

The signal handler is established with sigaction() rather than using
sigchain_push() as this allows us to control the signal mask when the
handler is invoked and ensure SA_RESTART is used to restart the
read() when resuming.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 131 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 128 insertions(+), 3 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 89f326adc1..45a9084bb3 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -1,4 +1,4 @@
-#include "git-compat-util.h"
+#include "cache.h"
 #include "compat/terminal.h"
 #include "sigchain.h"
 #include "strbuf.h"
@@ -23,6 +23,101 @@ static void restore_term_on_signal(int sig)
 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;
+
+static void write_err(const char *msg)
+{
+	write_in_full(2, "error: ", strlen("error: "));
+	write_in_full(2, msg, strlen(msg));
+	write_in_full(2, "\n", 1);
+}
+
+static void print_background_resume_msg(int signo)
+{
+	int saved_errno = errno;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+
+	ttou_received = 1;
+	write_err(background_resume_msg);
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	errno = saved_errno;
+}
+
+static void restore_terminal_on_suspend(int signo)
+{
+	int saved_errno = errno;
+	int res;
+	struct termios t;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+	int can_restore = 1;
+
+	if (tcgetattr(term_fd, &t) < 0)
+		can_restore = 0;
+
+	if (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)
+		write_err(restore_error_msg);
+
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	if (!can_restore) {
+		write_err(restore_error_msg);
+		goto out;
+	}
+	/*
+	 * If we resume in the background then we receive SIGTTOU when calling
+	 * tcsetattr() below. Set up a handler to print an error message in that
+	 * case.
+	 */
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGTTOU);
+	sa.sa_mask = old_sa.sa_mask;
+	sa.sa_handler = print_background_resume_msg;
+	sa.sa_flags = SA_RESTART;
+	sigaction(SIGTTOU, &sa, &old_sa);
+ again:
+	ttou_received = 0;
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	res = tcsetattr(term_fd, TCSAFLUSH, &t);
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	if (ttou_received)
+		goto again;
+	else if (res < 0)
+		write_err(restore_error_msg);
+	sigaction(SIGTTOU, &old_sa, NULL);
+ out:
+	errno = saved_errno;
+}
+
+static void reset_job_signals(void)
+{
+	if (restore_error_msg) {
+		signal(SIGTTIN, SIG_DFL);
+		signal(SIGTTOU, SIG_DFL);
+		signal(SIGTSTP, SIG_DFL);
+		restore_error_msg = NULL;
+		background_resume_msg = NULL;
+	}
+}
+
 static void close_term_fd(void)
 {
 	if (term_fd)
@@ -38,10 +133,13 @@ void restore_term(void)
 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
 	close_term_fd();
 	sigchain_pop_common();
+	reset_job_signals();
 }
 
 int save_term(enum save_term_flags flags)
 {
+	struct sigaction sa;
+
 	if (term_fd < 0)
 		term_fd = (flags & SAVE_TERM_STDIN) ? 0
 						    : open("/dev/tty", O_RDWR);
@@ -50,6 +148,26 @@ 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
+	 * 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;
 }
@@ -72,6 +190,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)
 		return 0;
 
 	sigchain_pop_common();
+	reset_job_signals();
 error:
 	close_term_fd();
 	return -1;
@@ -97,6 +216,7 @@ static int getchar_with_timeout(int timeout)
 	fd_set readfds;
 	int res;
 
+ again:
 	if (timeout >= 0) {
 		tv.tv_sec = timeout / 1000;
 		tv.tv_usec = (timeout % 1000) * 1000;
@@ -106,9 +226,14 @@ static int getchar_with_timeout(int timeout)
 	FD_ZERO(&readfds);
 	FD_SET(0, &readfds);
 	res = select(1, &readfds, NULL, NULL, tvp);
-	if (res <= 0)
+	if (!res)
 		return EOF;
-
+	if (res < 0) {
+		if (errno == EINTR)
+			goto again;
+		else
+			return EOF;
+	}
 	return getchar();
 }
 
-- 
2.35.1

Re: [PATCH v3 4/4] terminal: restore settings on SIGTSTP

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> @@ -23,6 +23,101 @@ static void restore_term_on_signal(int sig)
>  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;

It is a good idea to have a comment here to say why we had to
reinvent a subset of error(), instead of forcing curious readers to
"git blame" the log message for this commit (I am assuming that
"this is called from a signal handler and uses only functions that
are safe in that context" is the reason).

> +static void write_err(const char *msg)
> +{
> +	write_in_full(2, "error: ", strlen("error: "));
> +	write_in_full(2, msg, strlen(msg));
> +	write_in_full(2, "\n", 1);
> +}
> +
> +static void print_background_resume_msg(int signo)
> +{
> +	int saved_errno = errno;
> +	sigset_t mask;
> +	struct sigaction old_sa;
> +	struct sigaction sa = { .sa_handler = SIG_DFL };
> +
> +	ttou_received = 1;
> +	write_err(background_resume_msg);
> +	sigaction(signo, &sa, &old_sa);
> +	raise(signo);
> +	sigemptyset(&mask);
> +	sigaddset(&mask, signo);
> +	sigprocmask(SIG_UNBLOCK, &mask, NULL);
> +	/* Stopped here */
> +	sigprocmask(SIG_BLOCK, &mask, NULL);
> +	sigaction(signo, &old_sa, NULL);
> +	errno = saved_errno;
> +}
>  ...
> +	/* 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");

Nice to see such an attention to detail here.

Thanks.

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

[Phillip Wood]

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

[PATCH v4 1/4] terminal: use flags for save_term()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The next commit will add another flag in addition to the existing
full_duplex so change the function signature to take a flags
argument. Also alter the functions that call save_term() so that they
can pass flags down to it.

The choice to use an enum for tho bitwise flags is because gdb will
display the symbolic names of all the flags that are set rather than
the integer value.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 41 +++++++++++++++++++++--------------------
 compat/terminal.h |  7 ++++++-
 2 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 3620184e79..da2f788137 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -34,7 +34,7 @@ void restore_term(void)
 	sigchain_pop_common();
 }
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
 	if (term_fd < 0)
 		term_fd = open("/dev/tty", O_RDWR);
@@ -47,11 +47,11 @@ int save_term(int full_duplex)
 	return 0;
 }
 
-static int disable_bits(tcflag_t bits)
+static int disable_bits(enum save_term_flags flags, tcflag_t bits)
 {
 	struct termios t;
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		goto error;
 
 	t = old_term;
@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)
 	return -1;
 }
 
-static int disable_echo(void)
+static int disable_echo(enum save_term_flags flags)
 {
-	return disable_bits(ECHO);
+	return disable_bits(flags, ECHO);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(enum save_term_flags flags)
 {
-	return disable_bits(ICANON | ECHO);
+	return disable_bits(flags, ICANON | ECHO);
 }
 
 #elif defined(GIT_WINDOWS_NATIVE)
@@ -126,15 +126,15 @@ void restore_term(void)
 	hconin = hconout = INVALID_HANDLE_VALUE;
 }
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
 	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
 	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
 	    FILE_ATTRIBUTE_NORMAL, NULL);
 	if (hconin == INVALID_HANDLE_VALUE)
 		return -1;
 
-	if (full_duplex) {
+	if (flags & SAVE_TERM_DUPLEX) {
 		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
 			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
 			FILE_ATTRIBUTE_NORMAL, NULL);
@@ -154,7 +154,7 @@ int save_term(int full_duplex)
 	return -1;
 }
 
-static int disable_bits(DWORD bits)
+static int disable_bits(enum save_term_flags flags, DWORD bits)
 {
 	if (use_stty) {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)
 		use_stty = 0;
 	}
 
-	if (save_term(0) < 0)
+	if (save_term(flags) < 0)
 		return -1;
 
 	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)
 	return 0;
 }
 
-static int disable_echo(void)
+static int disable_echo(enum save_term_flags flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT);
+	return disable_bits(flags, ENABLE_ECHO_INPUT);
 }
 
-static int enable_non_canonical(void)
+static int enable_non_canonical(enum save_term_flags flags)
 {
-	return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
+	return disable_bits(flags,
+			    ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);
 }
 
 /*
@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
 		return NULL;
 	}
 
-	if (!echo && disable_echo()) {
+	if (!echo && disable_echo(0)) {
 		fclose(input_fh);
 		fclose(output_fh);
 		return NULL;
@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical() < 0) {
+	if (warning_displayed || enable_non_canonical(0) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)
 
 #else
 
-int save_term(int full_duplex)
+int save_term(enum save_term_flags flags)
 {
-	/* full_duplex == 1, but no support available */
-	return -full_duplex;
+	/* no duplex support available */
+	return -!!(flags & SAVE_TERM_DUPLEX);
 }
 
 void restore_term(void)
diff --git a/compat/terminal.h b/compat/terminal.h
index 0fb9fa147c..aeb24c9478 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,14 +1,19 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+enum save_term_flags {
+	/* Save input and output settings */
+	SAVE_TERM_DUPLEX = 1 << 0,
+};
+
 /*
  * Save the terminal attributes so they can be restored later by a
  * call to restore_term(). Note that every successful call to
  * save_term() must be matched by a call to restore_term() even if the
  * attributes have not been changed. Returns 0 on success, -1 on
  * failure.
  */
-int save_term(int full_duplex);
+int save_term(enum save_term_flags flags);
 /* Restore the terminal attributes that were saved with save_term() */
 void restore_term(void);
 
-- 
2.35.1

[PATCH v4 2/4] terminal: don't assume stdin is /dev/tty

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

read_key_without_echo() reads from stdin but uses /dev/tty when it
disables echo. This is unfortunate as there no guarantee that stdin is
the same device as /dev/tty. The perl version of "add -p" uses stdin
when it sets the terminal mode, this commit does the same for the
builtin version. There is still a difference between the perl and
builtin versions though - the perl version will ignore any errors when
setting the terminal mode[1] and will still read single bytes when
stdin is not a terminal. The builtin version displays a warning if
setting the terminal mode fails and switches to reading a line at a
time.

[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 29 ++++++++++++++++++++---------
 compat/terminal.h |  2 ++
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index da2f788137..4893294eb6 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -20,28 +20,41 @@ 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_needs_closing)
+		close(term_fd);
+	term_fd_needs_closing = 0;
+	term_fd = -1;
+}
+
 void restore_term(void)
 {
 	if (term_fd < 0)
 		return;
 
 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
-	close(term_fd);
-	term_fd = -1;
+	close_term_fd();
 	sigchain_pop_common();
 }
 
 int save_term(enum save_term_flags flags)
 {
 	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 (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;
@@ -52,7 +65,7 @@ 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;
 
@@ -65,9 +78,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)
 		return 0;
 
 	sigchain_pop_common();
-error:
-	close(term_fd);
-	term_fd = -1;
+	close_term_fd();
 	return -1;
 }
 
@@ -362,7 +373,7 @@ int read_key_without_echo(struct strbuf *buf)
 	static int warning_displayed;
 	int ch;
 
-	if (warning_displayed || enable_non_canonical(0) < 0) {
+	if (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {
 		if (!warning_displayed) {
 			warning("reading single keystrokes not supported on "
 				"this platform; reading line instead");
diff --git a/compat/terminal.h b/compat/terminal.h
index aeb24c9478..79ed00cf61 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -4,6 +4,8 @@
 enum save_term_flags {
 	/* Save input and output settings */
 	SAVE_TERM_DUPLEX = 1 << 0,
+	/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */
+	SAVE_TERM_STDIN  = 1 << 1,
 };
 
 /*
-- 
2.35.1

[PATCH v4 3/4] terminal: work around macos poll() bug

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

On macos the builtin "add -p" does not handle keys that generate
escape sequences because poll() does not work with terminals
there. Switch to using select() on non-windows platforms to work
around this.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 4893294eb6..2ae8a6f13e 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -92,6 +92,31 @@ static int enable_non_canonical(enum save_term_flags flags)
 	return disable_bits(flags, ICANON | ECHO);
 }
 
+/*
+ * On macos it is not possible to use poll() with a terminal so use select
+ * instead.
+ */
+static int getchar_with_timeout(int timeout)
+{
+	struct timeval tv, *tvp = NULL;
+	fd_set readfds;
+	int res;
+
+	if (timeout >= 0) {
+		tv.tv_sec = timeout / 1000;
+		tv.tv_usec = (timeout % 1000) * 1000;
+		tvp = &tv;
+	}
+
+	FD_ZERO(&readfds);
+	FD_SET(0, &readfds);
+	res = select(1, &readfds, NULL, NULL, tvp);
+	if (res <= 0)
+		return EOF;
+
+	return getchar();
+}
+
 #elif defined(GIT_WINDOWS_NATIVE)
 
 #define INPUT_PATH "CONIN$"
@@ -257,6 +282,16 @@ static int mingw_getchar(void)
 }
 #define getchar mingw_getchar
 
+static int getchar_with_timeout(int timeout)
+{
+	struct pollfd pfd = { .fd = 0, .events = POLLIN };
+
+	if (poll(&pfd, 1, timeout) < 1)
+		return EOF;
+
+	return getchar();
+}
+
 #endif
 
 #ifndef FORCE_TEXT
@@ -407,12 +442,7 @@ int read_key_without_echo(struct strbuf *buf)
 		 * half a second when we know that the sequence is complete.
 		 */
 		while (!is_known_escape_sequence(buf->buf)) {
-			struct pollfd pfd = { .fd = 0, .events = POLLIN };
-
-			if (poll(&pfd, 1, 500) < 1)
-				break;
-
-			ch = getchar();
+			ch = getchar_with_timeout(500);
 			if (ch == EOF)
 				break;
 			strbuf_addch(buf, ch);
-- 
2.35.1

[PATCH v4 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user suspends git while it is waiting for a keypress reset the
terminal before stopping and restore the settings when git resumes. If
the user tries to resume in the background print an error
message (taking care to use async safe functions) before stopping
again. Ideally we would reprint the prompt for the user when git
resumes but this patch just restarts the read().

The signal handler is established with sigaction() rather than using
sigchain_push() as this allows us to control the signal mask when the
handler is invoked and ensure SA_RESTART is used to restart the
read() when resuming.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 132 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 129 insertions(+), 3 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 2ae8a6f13e..7db330c52d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -1,4 +1,4 @@
-#include "git-compat-util.h"
+#include "cache.h"
 #include "compat/terminal.h"
 #include "sigchain.h"
 #include "strbuf.h"
@@ -24,6 +24,102 @@ 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: "));
+	write_in_full(2, msg, strlen(msg));
+	write_in_full(2, "\n", 1);
+}
+
+static void print_background_resume_msg(int signo)
+{
+	int saved_errno = errno;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+
+	ttou_received = 1;
+	write_err(background_resume_msg);
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	errno = saved_errno;
+}
+
+static void restore_terminal_on_suspend(int signo)
+{
+	int saved_errno = errno;
+	int res;
+	struct termios t;
+	sigset_t mask;
+	struct sigaction old_sa;
+	struct sigaction sa = { .sa_handler = SIG_DFL };
+	int can_restore = 1;
+
+	if (tcgetattr(term_fd, &t) < 0)
+		can_restore = 0;
+
+	if (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)
+		write_err(restore_error_msg);
+
+	sigaction(signo, &sa, &old_sa);
+	raise(signo);
+	sigemptyset(&mask);
+	sigaddset(&mask, signo);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	/* Stopped here */
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	sigaction(signo, &old_sa, NULL);
+	if (!can_restore) {
+		write_err(restore_error_msg);
+		goto out;
+	}
+	/*
+	 * If we resume in the background then we receive SIGTTOU when calling
+	 * tcsetattr() below. Set up a handler to print an error message in that
+	 * case.
+	 */
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGTTOU);
+	sa.sa_mask = old_sa.sa_mask;
+	sa.sa_handler = print_background_resume_msg;
+	sa.sa_flags = SA_RESTART;
+	sigaction(SIGTTOU, &sa, &old_sa);
+ again:
+	ttou_received = 0;
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+	res = tcsetattr(term_fd, TCSAFLUSH, &t);
+	sigprocmask(SIG_BLOCK, &mask, NULL);
+	if (ttou_received)
+		goto again;
+	else if (res < 0)
+		write_err(restore_error_msg);
+	sigaction(SIGTTOU, &old_sa, NULL);
+ out:
+	errno = saved_errno;
+}
+
+static void reset_job_signals(void)
+{
+	if (restore_error_msg) {
+		signal(SIGTTIN, SIG_DFL);
+		signal(SIGTTOU, SIG_DFL);
+		signal(SIGTSTP, SIG_DFL);
+		restore_error_msg = NULL;
+		background_resume_msg = NULL;
+	}
+}
+
 static void close_term_fd(void)
 {
 	if (term_fd_needs_closing)
@@ -40,10 +136,13 @@ void restore_term(void)
 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
 	close_term_fd();
 	sigchain_pop_common();
+	reset_job_signals();
 }
 
 int save_term(enum save_term_flags flags)
 {
+	struct sigaction sa;
+
 	if (term_fd < 0)
 		term_fd = ((flags & SAVE_TERM_STDIN)
 			   ? 0
@@ -56,6 +155,26 @@ int save_term(enum save_term_flags flags)
 		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;
 }
@@ -78,6 +197,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)
 		return 0;
 
 	sigchain_pop_common();
+	reset_job_signals();
 	close_term_fd();
 	return -1;
 }
@@ -102,6 +222,7 @@ static int getchar_with_timeout(int timeout)
 	fd_set readfds;
 	int res;
 
+ again:
 	if (timeout >= 0) {
 		tv.tv_sec = timeout / 1000;
 		tv.tv_usec = (timeout % 1000) * 1000;
@@ -111,9 +232,14 @@ static int getchar_with_timeout(int timeout)
 	FD_ZERO(&readfds);
 	FD_SET(0, &readfds);
 	res = select(1, &readfds, NULL, NULL, tvp);
-	if (res <= 0)
+	if (!res)
 		return EOF;
-
+	if (res < 0) {
+		if (errno == EINTR)
+			goto again;
+		else
+			return EOF;
+	}
 	return getchar();
 }
 
-- 
2.35.1