Re: [PATCH] compat/terminal: mark parameter of git_terminal_prompt() UNUSED

From: Jeff King <peff@peff.net>
Date: Aug 31, 2024, 02:40 UTC
Message-ID: <20240831024002.GA2150243@coredump.intra.peff.net>
In-Reply-To: <44804f4c-26af-4d23-b044-ec32a13b549c@ramsayjones.plus.com>

On Sat, Aug 31, 2024 at 02:40:22AM +0100, Ramsay Jones wrote:

Show 9 quoted lines

> > Here's a potential commit message for the patch:
> > 
> >   If neither HAVE_DEV_TTY nor GIT_WINDOWS_NATIVE is set, the fallback
> >   code calls the system getpass(). This unfortunately ignores the "echo"
> >   boolean parameter, as we have no way to implement that functionality.
> >   But we still have to keep the unused parameter, since our interface
> >   has to match the other implementations.
> 
> Yes, this reads well. Do you want to send an updated patch or shall I?

I assumed you would. Thanks!

Show 10 quoted lines

> > As an aside, I wonder if cygwin could be using either /dev/tty or the
> > Windows variant. But that's obviously a separate patch, and either way
> > we'd want to fix this fallback code in the meantime.
> 
> Yes, this is what I meant by '... it should not even be trying to compile
> that code ...' ;) ie I was expecting HAVE_DEV_TTY to be set on cygwin (which
> does have /dev/tty).
> 
> However, there may be reasons for it not being set - I haven't had time to
> look into it yet.

I doubt there is a good reason. When I introduced HAVE_DEV_TTY ages ago (2011, apparently!) I kept the conservative default as-is to avoid disrupting other platforms. So I suspect it is simply that nobody on Cygwin noticed the lousy fallback behavior, or knew that there was an alternative (the most obvious problem is that when we prompt for a username, what the user types is not visible).

Testing welcome, of course. :)

-Peff