threads / patch / 62022

patchcompat/terminal: mark parameter of git_terminal_prompt() UNUSED

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

## tl;dr

4 messages between Aug 29, 2024 and Aug 31, 2024. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Ramsay Jones· Aug 29, 2024, 21:57 UTC · lore

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(-)

Show changes to compat/terminal.c +1 −1

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);
 }

-- 
2.46.0

Jeff King· Aug 29, 2024, 22:26 UTC · re: Ramsay Jones · lore

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

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

Ramsay Jones· Aug 31, 2024, 01:40 UTC · re: Jeff King · lore

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

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

Jeff King· Aug 31, 2024, 02:40 UTC · re: Ramsay Jones · lore

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

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