| Message ID | a59f53af-58f7-42f5-aefb-50a4d9f344c4@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] config.mak.uname: add HAVE_DEV_TTY to cygwin config section | expand |
On Mon, Sep 02, 2024 at 11:15:35PM +0100, Ramsay Jones wrote: > Still, I need to do a full test suite run, just to check for any regressions. > (Unfortunately, that takes about 6 hours to run, so I can't get to that soon). I'm not sure if we cover this at all in the test suite, since it implies access to an actual terminal. All of the auth tests rely on the askpass interface to simulate the user typing a password. We do have test_terminal, which simulates a pty, but I don't think it would work for this case. For one thing, I don't know that it counts as the controlling terminal, so opening /dev/tty wouldn't find it. And two, the stdin handling had race problems that caused us to remove it recently. So it's really only good for checking isatty() for stdout/stderr. On the plus side, if it works for you in a single manual test then I suspect that's enough. The key thing is really "can we get something tty-ish that responds to termios", and it sounds like you can. Certainly doesn't hurt to test the single-key mode of "add -p", etc, though. > Anyhow, just a quick heads up. Thanks for looking into it. :) -Peff
On 05/09/2024 11:43, Jeff King wrote: > On Mon, Sep 02, 2024 at 11:15:35PM +0100, Ramsay Jones wrote: > >> Still, I need to do a full test suite run, just to check for any regressions. >> (Unfortunately, that takes about 6 hours to run, so I can't get to that soon). > > I'm not sure if we cover this at all in the test suite, since it implies > access to an actual terminal. All of the auth tests rely on the askpass > interface to simulate the user typing a password. > > We do have test_terminal, which simulates a pty, but I don't think it > would work for this case. For one thing, I don't know that it counts as > the controlling terminal, so opening /dev/tty wouldn't find it. And two, > the stdin handling had race problems that caused us to remove it > recently. So it's really only good for checking isatty() for > stdout/stderr. Heh, yes I suspect you are correct - it's just that I have been tripped up too many times thinking "well, these changes can't possibly affect the test-suite ..." :) Anyway, I confirmed tonight that there are no regressions noticed by the test-suite. (Which is not to say there aren't any ;) ). > On the plus side, if it works for you in a single manual test then I > suspect that's enough. The key thing is really "can we get something > tty-ish that responds to termios", and it sounds like you can. > > Certainly doesn't hurt to test the single-key mode of "add -p", etc, > though. Yeah, I still need to check the 'other uses' out. Hmm, I have never used 'add -p' (never felt the need), so I guess I will have to read up on how to use it ... ATB, Ramsay Jones
On Sat, Sep 07, 2024 at 12:53:11AM +0100, Ramsay Jones wrote: > > Certainly doesn't hurt to test the single-key mode of "add -p", etc, > > though. > > Yeah, I still need to check the 'other uses' out. Hmm, I have never > used 'add -p' (never felt the need), so I guess I will have to read > up on how to use it ... Try: git init echo foo >file git add file echo bar >file git -c interactive.singleKey=true add -p Hitting 'y' or 'n' should immediately be accepted (and exit the program either with the hunk staged or not), whereas I suspect it would not without HAVE_DEV_TTY. -Peff
On 07/09/2024 07:21, Jeff King wrote: > On Sat, Sep 07, 2024 at 12:53:11AM +0100, Ramsay Jones wrote: > >>> Certainly doesn't hurt to test the single-key mode of "add -p", etc, >>> though. >> >> Yeah, I still need to check the 'other uses' out. Hmm, I have never >> used 'add -p' (never felt the need), so I guess I will have to read >> up on how to use it ... > > Try: > > git init > echo foo >file > git add file > echo bar >file > git -c interactive.singleKey=true add -p > > Hitting 'y' or 'n' should immediately be accepted (and exit the program > either with the hunk staged or not), whereas I suspect it would not > without HAVE_DEV_TTY. Oh, that is much better! Everything works exactly as described and is a much better user experience than: "warning: reading single keystrokes not supported ..." :) Looking good. Thanks. ATB, Ramsay Jones
diff --git a/config.mak.uname b/config.mak.uname index d0cb2b8244..693dcd4714 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -250,6 +250,7 @@ ifeq ($(uname_O),Cygwin) else NO_REGEX = UnfortunatelyYes endif + HAVE_DEV_TTY = YesPlease HAVE_ALLOCA_H = YesPlease NEEDS_LIBICONV = YesPlease NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Hi, I found a few minutes to try building with HAVE_DEV_TTY on cygwin tonight and had a quick test: $ printf "%s\n" protocol=https host=example.com path=git | ./git credential fill Username for 'https://example.com': user Password for 'https://user@example.com': protocol=https host=example.com username=user password=pass $ So, that seems to work. Actually, when I first tried, it didn't work at all, because I typed 'git credential ...' not './git credential ...'! :) Also, somewhat surprising, is that job control also works: $ printf "%s\n" protocol=https host=example.com path=git | ./git credential fill Username for 'https://example.com': [1]+ Stopped printf "%s\n" protocol=https host=example.com path=git | ./git credential fill $ echo hhh hhh $ fg printf "%s\n" protocol=https host=example.com path=git | ./git credential fill user Password for 'https://user@example.com': protocol=https host=example.com username=user password=pass $ The only difference between Linux and cygwin seems to be that cygwin does not echo ^Z when back-grounding. Still, I need to do a full test suite run, just to check for any regressions. (Unfortunately, that takes about 6 hours to run, so I can't get to that soon). Also, I should check other uses of these routines: $ git grep -n read_key_without_echo add-patch.c:1226: int res = read_key_without_echo(&s->answer); compat/terminal.c:535:int read_key_without_echo(struct strbuf *buf) compat/terminal.c:602:int read_key_without_echo(struct strbuf *buf) compat/terminal.h:25:int read_key_without_echo(struct strbuf *buf); $ $ git grep -n git_terminal_prompt compat/terminal.c:428:char *git_terminal_prompt(const char *prompt, int echo) compat/terminal.c:597:char *git_terminal_prompt(const char *prompt, int echo UNUSED) compat/terminal.h:22:char *git_terminal_prompt(const char *prompt, int echo); prompt.c:65: r = git_terminal_prompt(prompt, flags & PROMPT_ECHO); $ $ git grep -n git_prompt builtin/bisect.c:401: yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO); builtin/bisect.c:913: yesno = git_prompt(_("Do you want me to do it for you " credential.c:247: r = git_prompt(prompt.buf, flags); help.c:712: answer = git_prompt(msg.buf, PROMPT_ECHO); prompt.c:45:char *git_prompt(const char *prompt, int flags) prompt.h:7:char *git_prompt(const char *prompt, int flags); $ So, 'add-patch', bisect and help (in addition to git-credential). [Note: save_term() and restore_term() are not called outside of that TU, so they could be marked static!] Anyhow, just a quick heads up. ATB, Ramsay Jones config.mak.uname | 1 + 1 file changed, 1 insertion(+)