[RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section

[RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section

[Ramsay Jones]

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(+)

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
-- 
2.46.0

Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section

[Jeff King]

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

Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section

[Ramsay Jones]

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

Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section

[Jeff King]

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

Re: [RFC PATCH] config.mak.uname: add HAVE_DEV_TTY to cygwin config section

[Ramsay Jones]

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