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

From: Jeff King <peff@peff.net>
Date: Aug 29, 2024, 22:26 UTC
Message-ID: <20240829222612.GA445751@coredump.intra.peff.net>
In-Reply-To: <d8c5e920-aff7-4e4b-af77-0d3193466b57@ramsayjones.plus.com>

On Thu, Aug 29, 2024 at 10:57:50PM +0100, Ramsay Jones wrote:

Show 7 quoted lines

> The 'seen' branch fails to compile on cygwin (but its fine on Linux), due
> to an unused parameter. I haven't looked too hard at the code (at first
> blush, it seemed to me that it should not even be trying to compile that
> code, but ...), I simply added an UNUSED to fix the build. ;)
> 
> So, this may not be the correct 'fix' for this, but I thought I should
> report it here, since I don't have time to look into this now. sorry! :(

Thanks, this is definitely the right fix. I have to rely on CI for catching cases outside of what I build locally, and it looks like we don't trigger this fallback code at all in CI (we hit HAVE_DEV_TTY for Linux and macOS, and then GIT_WINDOWS_NATIVE for Windows).

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.

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.

I actually kind of wonder if we could get away with assuming that every non-Windows platform is Unix-y enough to have /dev/tty, and just delete the fallback code entirely. But that is probably wishful thinking.

-Peff