| Message ID | 20211004072600.74241-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | protect git from a rogue editor | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > diff --git a/compat/terminal.c b/compat/terminal.c > index 43b73ddc75..1fadbfd6b6 100644 > --- a/compat/terminal.c > +++ b/compat/terminal.c > @@ -8,7 +8,7 @@ > > #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) > > -static void restore_term(void); > +void restore_term(void); Curious why you need this because (1) we do not have the same for save_term() here, and (2) we include compat/terminal.h where these two are declared next to each other.
On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > diff --git a/compat/terminal.c b/compat/terminal.c > > index 43b73ddc75..1fadbfd6b6 100644 > > --- a/compat/terminal.c > > +++ b/compat/terminal.c > > @@ -8,7 +8,7 @@ > > > > #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) > > > > -static void restore_term(void); > > +void restore_term(void); > > Curious why you need this because (1) we do not have the same for > save_term() here, and (2) we include compat/terminal.h where these > two are declared next to each other. It is in preparation for the next patch where we will call these newly public functions from editor.c I'll be reusing restore_term(), while save_term() is new, hence why only one had this change. Carlo
Carlo Arenas <carenas@gmail.com> writes: > On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote: >> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: >> > diff --git a/compat/terminal.c b/compat/terminal.c >> > index 43b73ddc75..1fadbfd6b6 100644 >> > --- a/compat/terminal.c >> > +++ b/compat/terminal.c >> > @@ -8,7 +8,7 @@ >> > >> > #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) >> > >> > -static void restore_term(void); >> > +void restore_term(void); >> >> Curious why you need this because (1) we do not have the same for >> save_term() here, and (2) we include compat/terminal.h where these >> two are declared next to each other. > > It is in preparation for the next patch where we will call these newly > public functions from editor.c > I'll be reusing restore_term(), while save_term() is new, hence why > only one had this change. I think I understand all that correctly. I was curious why the patch left a forward declaration, instead of just removing it, which it can do because now we have the necessary declaration in the header file it includes. With only [1/2]: - we already have save_term() and restore_term() externally declared, so even outside users can use them (which is a good thing to do), as long as they include <compat/terminal.h>. - we include <compat/terminal.h> in compat/terminal.c; I do not see why the patch needs to turn a static forward declaration into an extern one in the hunk in question. Thanks.
On Mon, Oct 4, 2021 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote: > Carlo Arenas <carenas@gmail.com> writes: > > On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote: > >> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > >> > diff --git a/compat/terminal.c b/compat/terminal.c > >> > index 43b73ddc75..1fadbfd6b6 100644 > >> > --- a/compat/terminal.c > >> > +++ b/compat/terminal.c > >> > @@ -8,7 +8,7 @@ > >> > > >> > #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) > >> > > >> > -static void restore_term(void); > >> > +void restore_term(void); > >> > >> Curious why you need this because (1) we do not have the same for > >> save_term() here, and (2) we include compat/terminal.h where these > >> two are declared next to each other. > > > > It is in preparation for the next patch where we will call these newly > > public functions from editor.c > > I'll be reusing restore_term(), while save_term() is new, hence why > > only one had this change. > > I think I understand all that correctly. > > I was curious why the patch left a forward declaration, instead of > just removing it, which it can do because now we have the necessary > declaration in the header file it includes. of course, just sloppy coding on my part; will remove in a reroll thanks, Carlo
diff --git a/compat/terminal.c b/compat/terminal.c index 43b73ddc75..1fadbfd6b6 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -8,7 +8,7 @@ #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) -static void restore_term(void); +void restore_term(void); static void restore_term_on_signal(int sig) { @@ -25,7 +25,7 @@ static void restore_term_on_signal(int sig) static int term_fd = -1; static struct termios old_term; -static void restore_term(void) +void restore_term(void) { if (term_fd < 0) return; @@ -35,15 +35,22 @@ static void restore_term(void) term_fd = -1; } +int save_term(int full_duplex) +{ + if (term_fd < 0) + term_fd = open("/dev/tty", O_RDWR); + + return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term); +} + static int disable_bits(tcflag_t bits) { struct termios t; - term_fd = open("/dev/tty", O_RDWR); - if (tcgetattr(term_fd, &t) < 0) + if (save_term(0) < 0) goto error; - old_term = t; + t = old_term; sigchain_push_common(restore_term_on_signal); t.c_lflag &= ~bits; @@ -75,9 +82,10 @@ static int enable_non_canonical(void) static int use_stty = 1; static struct string_list stty_restore = STRING_LIST_INIT_DUP; static HANDLE hconin = INVALID_HANDLE_VALUE; -static DWORD cmode; +static HANDLE hconout = INVALID_HANDLE_VALUE; +static DWORD cmode_in, cmode_out; -static void restore_term(void) +void restore_term(void) { if (use_stty) { int i; @@ -97,9 +105,42 @@ static void restore_term(void) if (hconin == INVALID_HANDLE_VALUE) return; - SetConsoleMode(hconin, cmode); + SetConsoleMode(hconin, cmode_in); + CloseHandle(hconin); + if (cmode_out) { + assert(hconout != INVALID_HANDLE_VALUE); + SetConsoleMode(hconout, cmode_out); + CloseHandle(hconout); + } + + hconin = hconout = INVALID_HANDLE_VALUE; +} + +int save_term(int full_duplex) +{ + 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) { + hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_WRITE, NULL, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, NULL); + if (hconout == INVALID_HANDLE_VALUE) + goto error; + + GetConsoleMode(hconout, &cmode_out); + } + + GetConsoleMode(hconin, &cmode_in); + use_stty = 0; + return 0; +error: CloseHandle(hconin); hconin = INVALID_HANDLE_VALUE; + return -1; } static int disable_bits(DWORD bits) @@ -135,15 +176,11 @@ static int disable_bits(DWORD bits) use_stty = 0; } - hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ, NULL, OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, NULL); - if (hconin == INVALID_HANDLE_VALUE) + if (save_term(0) < 0) return -1; - GetConsoleMode(hconin, &cmode); sigchain_push_common(restore_term_on_signal); - if (!SetConsoleMode(hconin, cmode & ~bits)) { + if (!SetConsoleMode(hconin, cmode_in & ~bits)) { CloseHandle(hconin); hconin = INVALID_HANDLE_VALUE; return -1; @@ -361,6 +398,16 @@ int read_key_without_echo(struct strbuf *buf) #else +int save_term(int full_duplex) +{ + /* full_duplex == 1, but no support available */ + return -full_duplex; +} + +void restore_term(void) +{ +} + char *git_terminal_prompt(const char *prompt, int echo) { return getpass(prompt); diff --git a/compat/terminal.h b/compat/terminal.h index a9d52b8464..e1770c575b 100644 --- a/compat/terminal.h +++ b/compat/terminal.h @@ -1,6 +1,9 @@ #ifndef COMPAT_TERMINAL_H #define COMPAT_TERMINAL_H +int save_term(int full_duplex); +void restore_term(void); + char *git_terminal_prompt(const char *prompt, int echo); /* Read a single keystroke, without echoing it to the terminal */
Currently, git will share its console with all its children (unless they create their own), and is therefore possible that any of them that might change settings of them could affect its operations once completed. Refactor the platform specific functionality to save the terminal settings and expand it to also do so for the output handler. This will allow for the state of the terminal to be saved and restored around a child that might misbehave (ex vi) which will be implemented next. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- compat/terminal.c | 75 ++++++++++++++++++++++++++++++++++++++--------- compat/terminal.h | 3 ++ 2 files changed, 64 insertions(+), 14 deletions(-)