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

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Aug 31, 2024, 01:40 UTC
Message-ID: <44804f4c-26af-4d23-b044-ec32a13b549c@ramsayjones.plus.com>
In-Reply-To: <20240829222612.GA445751@coredump.intra.peff.net>

On 29/08/2024 23:26, Jeff King wrote:

Show 22 quoted lines

> On Thu, Aug 29, 2024 at 10:57:50PM +0100, Ramsay Jones wrote:
> 
>> 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.

Yes, this reads well. Do you want to send an updated patch or shall I?

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

ATB, Ramsay Jones