{"thread":{"id":"62022","subject":"[PATCH] compat/terminal: mark parameter of git_terminal_prompt() UNUSED","startedAt":"2024-08-29T22:01:01Z","lastAt":"2024-08-31T02:40:05Z","messageCount":4,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501877","messageId":"d8c5e920-aff7-4e4b-af77-0d3193466b57@ramsayjones.plus.com","threadId":"62022","inReplyTo":null,"subject":"[PATCH] compat/terminal: mark parameter of git_terminal_prompt() UNUSED","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-08-29T21:57:50Z","receivedAt":"2024-08-29T22:01:01Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones \n---\n\nHi,\n\nThe 'seen' branch fails to compile on cygwin (but its fine on Linux), due\nto an unused parameter. I haven't looked too hard at the code (at first\nblush, it seemed to me that it should not even be trying to compile that\ncode, but ...), I simply added an UNUSED to fix the build. ;)\n\nSo, this may not be the correct 'fix' for this, but I thought I should\nreport it here, since I don't have time to look into this now. sorry! :(\n\nATB,\nRamsay Jones\n\n compat/terminal.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 0afda730f2..d54efa1c5d 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -594,7 +594,7 @@ void restore_term(void)\n {\n }\n \n-char *git_terminal_prompt(const char *prompt, int echo)\n+char *git_terminal_prompt(const char *prompt, int echo UNUSED)\n {\n \treturn getpass(prompt);\n }\n-- \n2.46.0\n"},{"id":"501880","messageId":"20240829222612.GA445751@coredump.intra.peff.net","threadId":"62022","inReplyTo":"d8c5e920-aff7-4e4b-af77-0d3193466b57@ramsayjones.plus.com","subject":"Re: [PATCH] compat/terminal: mark parameter of git_terminal_prompt() UNUSED","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-29T22:26:12Z","receivedAt":"2024-08-29T22:26:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2024 at 10:57:50PM +0100, Ramsay Jones wrote:\n\n> The 'seen' branch fails to compile on cygwin (but its fine on Linux), due\n> to an unused parameter. I haven't looked too hard at the code (at first\n> blush, it seemed to me that it should not even be trying to compile that\n> code, but ...), I simply added an UNUSED to fix the build. ;)\n> \n> So, this may not be the correct 'fix' for this, but I thought I should\n> report it here, since I don't have time to look into this now. sorry! :(\n\nThanks, this is definitely the right fix. I have to rely on CI for\ncatching cases outside of what I build locally, and it looks like we\ndon't trigger this fallback code at all in CI (we hit HAVE_DEV_TTY for\nLinux and macOS, and then GIT_WINDOWS_NATIVE for Windows).\n\nHere's a potential commit message for the patch:\n\n If neither HAVE_DEV_TTY nor GIT_WINDOWS_NATIVE is set, the fallback\n code calls the system getpass(). This unfortunately ignores the \"echo\"\n boolean parameter, as we have no way to implement that functionality.\n But we still have to keep the unused parameter, since our interface\n has to match the other implementations.\n\nAs an aside, I wonder if cygwin could be using either /dev/tty or the\nWindows variant. But that's obviously a separate patch, and either way\nwe'd want to fix this fallback code in the meantime.\n\nI actually kind of wonder if we could get away with assuming that every\nnon-Windows platform is Unix-y enough to have /dev/tty, and just delete\nthe fallback code entirely. But that is probably wishful thinking.\n\n-Peff\n"},{"id":"501948","messageId":"44804f4c-26af-4d23-b044-ec32a13b549c@ramsayjones.plus.com","threadId":"62022","inReplyTo":"20240829222612.GA445751@coredump.intra.peff.net","subject":"Re: [PATCH] compat/terminal: mark parameter of git_terminal_prompt() UNUSED","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-08-31T01:40:22Z","receivedAt":"2024-08-31T01:43:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 29/08/2024 23:26, Jeff King wrote:\n> On Thu, Aug 29, 2024 at 10:57:50PM +0100, Ramsay Jones wrote:\n> \n>> The 'seen' branch fails to compile on cygwin (but its fine on Linux), due\n>> to an unused parameter. I haven't looked too hard at the code (at first\n>> blush, it seemed to me that it should not even be trying to compile that\n>> code, but ...), I simply added an UNUSED to fix the build. ;)\n>>\n>> So, this may not be the correct 'fix' for this, but I thought I should\n>> report it here, since I don't have time to look into this now. sorry! :(\n> \n> Thanks, this is definitely the right fix. I have to rely on CI for\n> catching cases outside of what I build locally, and it looks like we\n> don't trigger this fallback code at all in CI (we hit HAVE_DEV_TTY for\n> Linux and macOS, and then GIT_WINDOWS_NATIVE for Windows).\n> \n> Here's a potential commit message for the patch:\n> \n> If neither HAVE_DEV_TTY nor GIT_WINDOWS_NATIVE is set, the fallback\n> code calls the system getpass(). This unfortunately ignores the \"echo\"\n> boolean parameter, as we have no way to implement that functionality.\n> But we still have to keep the unused parameter, since our interface\n> has to match the other implementations.\n\nYes, this reads well. Do you want to send an updated patch or shall I?\n\n> As an aside, I wonder if cygwin could be using either /dev/tty or the\n> Windows variant. But that's obviously a separate patch, and either way\n> we'd want to fix this fallback code in the meantime.\n\nYes, this is what I meant by '... it should not even be trying to compile\nthat code ...' ;) ie I was expecting HAVE_DEV_TTY to be set on cygwin (which\ndoes have /dev/tty).\n\nHowever, there may be reasons for it not being set - I haven't had time to\nlook into it yet.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"501949","messageId":"20240831024002.GA2150243@coredump.intra.peff.net","threadId":"62022","inReplyTo":"44804f4c-26af-4d23-b044-ec32a13b549c@ramsayjones.plus.com","subject":"Re: [PATCH] compat/terminal: mark parameter of git_terminal_prompt() UNUSED","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-31T02:40:02Z","receivedAt":"2024-08-31T02:40:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 31, 2024 at 02:40:22AM +0100, Ramsay Jones wrote:\n\n> > Here's a potential commit message for the patch:\n> > \n> > If neither HAVE_DEV_TTY nor GIT_WINDOWS_NATIVE is set, the fallback\n> > code calls the system getpass(). This unfortunately ignores the \"echo\"\n> > boolean parameter, as we have no way to implement that functionality.\n> > But we still have to keep the unused parameter, since our interface\n> > has to match the other implementations.\n> \n> Yes, this reads well. Do you want to send an updated patch or shall I?\n\nI assumed you would. Thanks!\n\n> > As an aside, I wonder if cygwin could be using either /dev/tty or the\n> > Windows variant. But that's obviously a separate patch, and either way\n> > we'd want to fix this fallback code in the meantime.\n> \n> Yes, this is what I meant by '... it should not even be trying to compile\n> that code ...' ;) ie I was expecting HAVE_DEV_TTY to be set on cygwin (which\n> does have /dev/tty).\n> \n> However, there may be reasons for it not being set - I haven't had time to\n> look into it yet.\n\nI doubt there is a good reason. When I introduced HAVE_DEV_TTY ages ago\n(2011, apparently!) I kept the conservative default as-is to avoid\ndisrupting other platforms. So I suspect it is simply that nobody on\nCygwin noticed the lousy fallback behavior, or knew that there was an\nalternative (the most obvious problem is that when we prompt for a\nusername, what the user types is not visible).\n\nTesting welcome, of course. :)\n\n-Peff\n"}]}