Re: [PATCH] compat: add a getpass() compatibility function

[Erik Faye-Lund <kusmabite@gmail.com>]

On Thu, May 19, 2011 at 7:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Rafael Gieschke <rafael@gieschke.de> writes:
>
>>> Windows doesn't have /dev/tty, but the logic in this version handles
>>> that by using stdin/stderr instead. The signal-stuff has a comment
>>> that indicates it might not even be correct. tcgetattr/tcsetattr isn't
>>> supported on Windows, but it's not needed if we use getch (as the
>>> version in compat/mingw.c does). POSIX/curses getch respects the
>>> echo-setting, while Windows getch never echo.
>
> Probably a properly abstracted common version would look like a function
> that calls four platform-dependent helper funcions in this order:
>
>        0. prompt
>        1. start "noecho" mode
>        2. get whole line
>        3. exit "noecho" mode
>
> where Windows may use stderr for 0, have noop() implementation for 1 and
> 3, use _getch() that does not echo for 2, while POSIX may write to
> /dev/tty for 0, use tc[gs]etattr() with perhaps some signal settings
> sprinkled in for 1 and 3.
>
> So I don't see a need for Windows to emulate tc[g]setattr nor curses in
> order to get a generic getpass() abstraction between two platforms.
>

When I think about it a bit more, it feels a bit pointless:
0. is identical (fputs)
1. is different (tc[gs]etattr vs nop)
2. is different (getc vs _getch)
3. is different (tcsetattr vs nop)

So there's probably not much code to share here. There's a bit of
logic, but I'm not entirely sure this should be the same either,
because on Windows we have to take care of '\r' (since we open stdin
in binary mode at start-up).

But looking at the implementations, there's one thing that strike me:
both the netbsd and uclibc implementations fill a static buffer, while
our windows-version returns an allocated buffer. Reading POSIX, it's
not entirely clear if the returned pointer should be free'd or not:
http://pubs.opengroup.org/onlinepubs/007908799/xsh/getpass.html

But there is a hint: the limit of PASS_MAX bytes indicate that the
buffer is expected to be statically allocated. So perhaps we should do
this to prevent a leak?

diff --git a/compat/mingw.c b/compat/mingw.c
index 878b1de..dba3e64 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1668,17 +1668,19 @@ int link(const char *oldpath, const char *newpath)

 char *getpass(const char *prompt)
 {
-	struct strbuf buf = STRBUF_INIT;
+	static char buf[PASS_MAX];
+	int i;

 	fputs(prompt, stderr);
-	for (;;) {
+	for (i = 0; i < PASS_MAX - 1; ++i) {
 		char c = _getch();
 		if (c == '\r' || c == '\n')
 			break;
-		strbuf_addch(&buf, c);
+		buf[i] = c;
 	}
+	buf[i] = '\0';
 	fputs("\n", stderr);
-	return strbuf_detach(&buf, NULL);
+	return buf;
 }

 pid_t waitpid(pid_t pid, int *status, unsigned options)
diff --git a/compat/mingw.h b/compat/mingw.h
index 62eccd3..e37d557 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -48,6 +48,8 @@ typedef int socklen_t;
 #define EAFNOSUPPORT WSAEAFNOSUPPORT
 #define ECONNABORTED WSAECONNABORTED

+#define PASS_MAX 512
+
 struct passwd {
 	char *pw_name;
 	char *pw_gecos;