From: Phillip Wood Date: Mon, 07 Mar 2022 11:11:14 GMT Subject: Re: [PATCH 1/4] terminal: use flags for save_term() Message-ID: In-Reply-To: <95cbca4c-ec26-853e-243f-461a365d9f71@ramsayjones.plus.com> 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 >> >> 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 >> --- >> 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); >>