Re: [PATCH] Enable HAVE_DEV_TTY for Solaris

Ben Walton [off-list ref] writes:

Now that git_terminal_prompt can cleanly interact with /dev/tty on
Solaris, enable HAVE_DEV_TTY so that this code path is used for
credential reading instead of relying on the crippled getpass().

Signed-off-by: Ben Walton <redacted>
---

This is a follow up to Jeff's patch that fixes git_terminal_prompt on
Solaris.  I don't have 5.6 or 5.7 for testing but I believe this
should be valid for both of those releases as well.

So which direction do you guys want to go?  Use the "bidirectional
stdio with fseek()" for now, with the expectation that Tay's other
series will rewrite it to fd based one?

On Mon, Aug 06, 2012 at 08:43:02PM -0700, Junio C Hamano wrote:

Ben Walton [off-list ref] writes:

quoted

Now that git_terminal_prompt can cleanly interact with /dev/tty on
Solaris, enable HAVE_DEV_TTY so that this code path is used for
credential reading instead of relying on the crippled getpass().

Signed-off-by: Ben Walton <redacted>
---

This is a follow up to Jeff's patch that fixes git_terminal_prompt on
Solaris.  I don't have 5.6 or 5.7 for testing but I believe this
should be valid for both of those releases as well.

So which direction do you guys want to go?  Use the "bidirectional
stdio with fseek()" for now, with the expectation that Tay's other
series will rewrite it to fd based one?

I think so. The stdio fix is short and obviously correct, and then Tay
can either refactor or not as he sees fit for his topic (although if we
do switch to just a terminal_can_prompt() interface and get rid of the
term_t ugliness, then there is not even any need to do the rewrite).

-Peff

On Tue, Aug 07, 2012 at 12:03:26AM -0400, Jeff King wrote:

quoted

So which direction do you guys want to go?  Use the "bidirectional
stdio with fseek()" for now, with the expectation that Tay's other
series will rewrite it to fd based one?

I think so. The stdio fix is short and obviously correct, and then Tay
can either refactor or not as he sees fit for his topic (although if we
do switch to just a terminal_can_prompt() interface and get rid of the
term_t ugliness, then there is not even any need to do the rewrite).

And here it is again, this time with a signed-off-by (I fixed my script
after our last discussion, but accidentally copied an old version to the
Solaris VM I just installed. ;) ).

-- >8 --
Subject: [PATCH] terminal: seek when switching between reading and writing

When a stdio stream is opened in update mode (e.g., "w+"),
the C standard forbids switching between reading or writing
without an intervening positioning function. Many
implementations are lenient about this, but Solaris libc
will flush the recently-read contents to the output buffer.
In this instance, that meant writing the non-echoed password
that the user just typed to the terminal.

Fix it by inserting a no-op fseek between the read and
write.

The opposite direction (writing followed by reading) is also
disallowed, but our intervening fflush is an acceptable
positioning function for that alternative.

Signed-off-by: Jeff King <redacted>
---
 compat/terminal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/compat/terminal.c b/compat/terminal.c
index 6d16c8f..bbb038d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c

@@ -59,6 +59,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
 
 	r = strbuf_getline(&buf, fh, '\n');
 	if (!echo) {
+		fseek(fh, SEEK_CUR, 0);
 		putc('\n', fh);
 		fflush(fh);
 	}

Excerpts from Jeff King's message of Tue Aug 07 00:10:26 -0400 2012:

Signed-off-by: Jeff King <redacted>

Acked-by: Ben Walton <redacted>

I agree with your assesment that this is the right way to go (vs
migrating out of stdio) for now.  If the changes Tay needs to make
require the migration then it can become part of that series.
Otherwise those changes would just be change for change's sake at this
time.

If my HAVE_DEV_TTY enabling patch for Solaris is added on top of this,
I'm a happy camper.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

On Tue, Aug 7, 2012 at 6:10 AM, Jeff King [off-list ref] wrote:

Subject: [PATCH] terminal: seek when switching between reading and writing

When a stdio stream is opened in update mode (e.g., "w+"),
the C standard forbids switching between reading or writing
without an intervening positioning function. Many
implementations are lenient about this, but Solaris libc
will flush the recently-read contents to the output buffer.
In this instance, that meant writing the non-echoed password
that the user just typed to the terminal.

Fix it by inserting a no-op fseek between the read and
write.

My Windows-patches for git_terminal_prompt would probably also solve
this problem. Instead of opening a read-write handle to /dev/tty, they
open two handles to the terminal instead; one for reading and one for
writing. This is because the terminal cannot be opened in read-write
mode on Windows (we need to open "CONIN$" and "CONOUT$" separately).

You can have a look at the series here if you're interested:
https://github.com/kusma/git/tree/work/terminal-cleanup

That last patch is the reason why I haven't submitted the series yet,
but perhaps some of the preparatory patches could be worth-while for
other platforms in the mean time?

On Wed, Aug 08, 2012 at 04:13:03PM +0200, Erik Faye-Lund wrote:

On Tue, Aug 7, 2012 at 6:10 AM, Jeff King [off-list ref] wrote:

quoted

Subject: [PATCH] terminal: seek when switching between reading and writing

When a stdio stream is opened in update mode (e.g., "w+"),
the C standard forbids switching between reading or writing
without an intervening positioning function. Many
implementations are lenient about this, but Solaris libc
will flush the recently-read contents to the output buffer.
In this instance, that meant writing the non-echoed password
that the user just typed to the terminal.

Fix it by inserting a no-op fseek between the read and
write.

My Windows-patches for git_terminal_prompt would probably also solve
this problem. Instead of opening a read-write handle to /dev/tty, they
open two handles to the terminal instead; one for reading and one for
writing. This is because the terminal cannot be opened in read-write
mode on Windows (we need to open "CONIN$" and "CONOUT$" separately).

Yeah, it would solve it, although it means opening /dev/tty twice (which
is probably not a big deal, though). I'm fine if we go that way in the
long run to share implementations, but let's treat it as a separate
topic.  This fix is an obvious one-liner, and is just fixing me being
stupid about actually following the C standard. So it's a no-brainer for
as a maintenance fix.

You can have a look at the series here if you're interested:
https://github.com/kusma/git/tree/work/terminal-cleanup

That last patch is the reason why I haven't submitted the series yet,
but perhaps some of the preparatory patches could be worth-while for
other platforms in the mean time?

Yeah, that last patch is really gross. There's no explanation of the
race issue, so I'll refrain from thinking about it until you are ready
to post a series. :)

-Peff