{"thread":{"id":"62041","subject":"[RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section","startedAt":"2024-09-02T22:18:47Z","lastAt":"2024-09-07T13:51:15Z","messageCount":5,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501994","messageId":"a59f53af-58f7-42f5-aefb-50a4d9f344c4@ramsayjones.plus.com","threadId":"62041","inReplyTo":null,"subject":"[RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-09-02T22:15:35Z","receivedAt":"2024-09-02T22:18:47Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n\nHi,\n\nI found a few minutes to try building with HAVE_DEV_TTY on cygwin tonight\nand had a quick test:\n\n  $ printf \"%s\\n\" protocol=https host=example.com path=git | ./git credential fill\n  Username for 'https://example.com': user\n  Password for 'https://user@example.com': \n  protocol=https\n  host=example.com\n  username=user\n  password=pass\n  $ \n\nSo, that seems to work. Actually, when I first tried, it didn't work at all,\nbecause I typed 'git credential ...' not './git credential ...'! :)\n\nAlso, somewhat surprising, is that job control also works:\n\n  $ printf \"%s\\n\" protocol=https host=example.com path=git | ./git credential fill\n  Username for 'https://example.com': \n  [1]+  Stopped                 printf \"%s\\n\" protocol=https host=example.com path=git | ./git credential fill\n  $ echo hhh\n  hhh\n  $ fg\n  printf \"%s\\n\" protocol=https host=example.com path=git | ./git credential fill\n  user\n  Password for 'https://user@example.com': \n  protocol=https\n  host=example.com\n  username=user\n  password=pass\n  $ \n\nThe only difference between Linux and cygwin seems to be that cygwin does\nnot echo ^Z when back-grounding.\n\nStill, I need to do a full test suite run, just to check for any regressions.\n(Unfortunately, that takes about 6 hours to run, so I can't get to that soon).\n\nAlso, I should check other uses of these routines:\n\n  $ git grep -n read_key_without_echo\n  add-patch.c:1226:               int res = read_key_without_echo(&s->answer);\n  compat/terminal.c:535:int read_key_without_echo(struct strbuf *buf)\n  compat/terminal.c:602:int read_key_without_echo(struct strbuf *buf)\n  compat/terminal.h:25:int read_key_without_echo(struct strbuf *buf);\n  $ \n\n  $ git grep -n git_terminal_prompt\n  compat/terminal.c:428:char *git_terminal_prompt(const char *prompt, int echo)\n  compat/terminal.c:597:char *git_terminal_prompt(const char *prompt, int echo UNUSED)\n  compat/terminal.h:22:char *git_terminal_prompt(const char *prompt, int echo);\n  prompt.c:65:                    r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);\n  $ \n\n  $ git grep -n git_prompt\n  builtin/bisect.c:401:           yesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n  builtin/bisect.c:913:   yesno = git_prompt(_(\"Do you want me to do it for you \"\n  credential.c:247:       r = git_prompt(prompt.buf, flags);\n  help.c:712:                     answer = git_prompt(msg.buf, PROMPT_ECHO);\n  prompt.c:45:char *git_prompt(const char *prompt, int flags)\n  prompt.h:7:char *git_prompt(const char *prompt, int flags);\n  $ \n  \nSo, 'add-patch', bisect and help (in addition to git-credential).\n\n[Note: save_term() and restore_term() are not called outside of that TU, so\nthey could be marked static!]\n\nAnyhow, just a quick heads up.\n\nATB,\nRamsay Jones\n\n\n config.mak.uname | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex d0cb2b8244..693dcd4714 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -250,6 +250,7 @@ ifeq ($(uname_O),Cygwin)\n         else\n \t\tNO_REGEX = UnfortunatelyYes\n         endif\n+\tHAVE_DEV_TTY = YesPlease\n \tHAVE_ALLOCA_H = YesPlease\n \tNEEDS_LIBICONV = YesPlease\n \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n-- \n2.46.0\n"},{"id":"502231","messageId":"20240905104331.GD2556395@coredump.intra.peff.net","threadId":"62041","inReplyTo":"a59f53af-58f7-42f5-aefb-50a4d9f344c4@ramsayjones.plus.com","subject":"Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-05T10:43:31Z","receivedAt":"2024-09-05T10:43:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 02, 2024 at 11:15:35PM +0100, Ramsay Jones wrote:\n\n> Still, I need to do a full test suite run, just to check for any regressions.\n> (Unfortunately, that takes about 6 hours to run, so I can't get to that soon).\n\nI'm not sure if we cover this at all in the test suite, since it implies\naccess to an actual terminal. All of the auth tests rely on the askpass\ninterface to simulate the user typing a password.\n\nWe do have test_terminal, which simulates a pty, but I don't think it\nwould work for this case. For one thing, I don't know that it counts as\nthe controlling terminal, so opening /dev/tty wouldn't find it. And two,\nthe stdin handling had race problems that caused us to remove it\nrecently. So it's really only good for checking isatty() for\nstdout/stderr.\n\nOn the plus side, if it works for you in a single manual test then I\nsuspect that's enough. The key thing is really \"can we get something\ntty-ish that responds to termios\", and it sounds like you can.\n\nCertainly doesn't hurt to test the single-key mode of \"add -p\", etc,\nthough.\n\n> Anyhow, just a quick heads up.\n\nThanks for looking into it. :)\n\n-Peff\n"},{"id":"502382","messageId":"c18fc95a-4dec-4e7b-8ec3-ba9747e9c464@ramsayjones.plus.com","threadId":"62041","inReplyTo":"20240905104331.GD2556395@coredump.intra.peff.net","subject":"Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-09-06T23:53:11Z","receivedAt":"2024-09-06T23:56:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/09/2024 11:43, Jeff King wrote:\n> On Mon, Sep 02, 2024 at 11:15:35PM +0100, Ramsay Jones wrote:\n> \n>> Still, I need to do a full test suite run, just to check for any regressions.\n>> (Unfortunately, that takes about 6 hours to run, so I can't get to that soon).\n> \n> I'm not sure if we cover this at all in the test suite, since it implies\n> access to an actual terminal. All of the auth tests rely on the askpass\n> interface to simulate the user typing a password.\n> \n> We do have test_terminal, which simulates a pty, but I don't think it\n> would work for this case. For one thing, I don't know that it counts as\n> the controlling terminal, so opening /dev/tty wouldn't find it. And two,\n> the stdin handling had race problems that caused us to remove it\n> recently. So it's really only good for checking isatty() for\n> stdout/stderr.\n\nHeh, yes I suspect you are correct - it's just that I have been tripped\nup too many times thinking \"well, these changes can't possibly affect\nthe test-suite ...\" :)\n\nAnyway, I confirmed tonight that there are no regressions noticed by\nthe test-suite. (Which is not to say there aren't any ;) ).\n\n> On the plus side, if it works for you in a single manual test then I\n> suspect that's enough. The key thing is really \"can we get something\n> tty-ish that responds to termios\", and it sounds like you can.\n> \n> Certainly doesn't hurt to test the single-key mode of \"add -p\", etc,\n> though.\n\nYeah, I still need to check the 'other uses' out. Hmm, I have never\nused 'add -p' (never felt the need), so I guess I will have to read\nup on how to use it ...\n\nATB,\nRamsay Jones\n\n\n"},{"id":"502386","messageId":"20240907062112.GA2585439@coredump.intra.peff.net","threadId":"62041","inReplyTo":"c18fc95a-4dec-4e7b-8ec3-ba9747e9c464@ramsayjones.plus.com","subject":"Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-07T06:21:12Z","receivedAt":"2024-09-07T06:21:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 07, 2024 at 12:53:11AM +0100, Ramsay Jones wrote:\n\n> > Certainly doesn't hurt to test the single-key mode of \"add -p\", etc,\n> > though.\n> \n> Yeah, I still need to check the 'other uses' out. Hmm, I have never\n> used 'add -p' (never felt the need), so I guess I will have to read\n> up on how to use it ...\n\nTry:\n\n  git init\n  echo foo >file\n  git add file\n  echo bar >file\n  git -c interactive.singleKey=true add -p\n\nHitting 'y' or 'n' should immediately be accepted (and exit the program\neither with the hunk staged or not), whereas I suspect it would not\nwithout HAVE_DEV_TTY.\n\n-Peff\n"},{"id":"502388","messageId":"4e18ec3b-c536-4bc3-9883-77e35686f5d3@ramsayjones.plus.com","threadId":"62041","inReplyTo":"20240907062112.GA2585439@coredump.intra.peff.net","subject":"Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-09-07T13:48:05Z","receivedAt":"2024-09-07T13:51:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 07/09/2024 07:21, Jeff King wrote:\n> On Sat, Sep 07, 2024 at 12:53:11AM +0100, Ramsay Jones wrote:\n> \n>>> Certainly doesn't hurt to test the single-key mode of \"add -p\", etc,\n>>> though.\n>>\n>> Yeah, I still need to check the 'other uses' out. Hmm, I have never\n>> used 'add -p' (never felt the need), so I guess I will have to read\n>> up on how to use it ...\n> \n> Try:\n> \n>   git init\n>   echo foo >file\n>   git add file\n>   echo bar >file\n>   git -c interactive.singleKey=true add -p\n> \n> Hitting 'y' or 'n' should immediately be accepted (and exit the program\n> either with the hunk staged or not), whereas I suspect it would not\n> without HAVE_DEV_TTY.\n\nOh, that is much better! Everything works exactly as described and is\na much better user experience than:\n\n    \"warning: reading single keystrokes not supported ...\"\n\n:)\n\nLooking good.\n\nThanks.\n\nATB,\nRamsay Jones\n\n"}]}