| Message ID | d8c5e920-aff7-4e4b-af77-0d3193466b57@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | compat/terminal: mark parameter of git_terminal_prompt() UNUSED | expand |
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. 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
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
On Sat, Aug 31, 2024 at 02:40:22AM +0100, Ramsay Jones wrote: > > 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! > > 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
diff --git a/compat/terminal.c b/compat/terminal.c index 0afda730f2..d54efa1c5d 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -594,7 +594,7 @@ void restore_term(void) { } -char *git_terminal_prompt(const char *prompt, int echo) +char *git_terminal_prompt(const char *prompt, int echo UNUSED) { return getpass(prompt); }
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Hi, 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! :( ATB, Ramsay Jones compat/terminal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)