Message ID | 20220304131126.8293-2-phillip.wood123@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | builtin add -p: hopefully final readkey fixes | expand |
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); >
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.
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
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); >>
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...), ....
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
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
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);