On 29/08/2024 23:26, Jeff King wrote: > 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
