Re: [PATCH] Avoid crippled getpass function on Solaris

Excerpts from Jeff King's message of Mon Aug 06 15:39:58 -0400 2012:

On Sun, Aug 05, 2012 at 10:35:06PM -0400, Ben Walton wrote:

quoted

Excerpts from Junio C Hamano's message of Sun Aug 05 21:59:48 -0400 2012:

quoted

Wouldn't

    #if solaris
        #define getpass getpassphrase
        #endif

without anything else be more than sufficient?

Yes, it would, but I was hoping to make it more explicit that the
function getpass may be substituted with something else.

I don't think that's important. Either the thing is a drop-in replica of
getpass, or it is not. In the former case, it's OK for it to be
transparent that it has been replaced. In the latter case, it should not
be a #define replacement at all, but should be its own alternative in
compat/terminal.c (just like HAVE_DEV_TTY is).  From my reading of
getpassphrase, it does seem to be a drop-in replacement.

It is a drop in replacement (identical signature) so that's no issue.
I can simplify this to the form suggested by Junio if that's
preferrable.  My idea of explicitness was for the programmer though.
I think it's easier for someone else reading this code later to
realize that the GETPASS function may change depending on conditions.

But, as I said, I'm not adverse to changing this in any way, I simply
want the improved functionality and am not married to the approach.
(I considered about three different ways of doing this before
submitting.)

So I'm OK conceptually with the patch if we can't do any better. But
getpass still sucks. It doesn't handle echoing, and it may or may
not fall back to reading from stdin if the tty isn't available
(which is disastrous for remote-curl, whose stdin is speaking the
remote-helper protocol to git). So I'd really prefer to make
HAVE_DEV_TTY work with Solaris if we can.

I poked through getpass in the opensolaris code and it did fall back
to reading from stdin[1] if there were issues opening /dev/tty so
there is room for trouble here.  That doesn't mean that other versions
of Solaris did it this way, but it's fairly likely that they did given
the culture.

I'm happy to spend a few cycles on it.  I don't have access to any
real Solaris boxes these days, but I imagine I can get OpenSolaris
running under VirtualBox without too much trouble...

I'm also happy to do this work and have ready access to Solaris 8-11.
Would it be reasonable to support using getpassphrase() if
!HAVE_DEV_TTY in addition to making the HAVE_DEV_TTY code function on
Solaris too?  That would provide a harm reduction for Solaris users
that (for some reason) disabled the nicer interface...but maybe it's
too much?

PS If we do go the getpassphrase route, does it make sense to
   introduce HAVE_GETPASSPHRASE? We usually try to provide one layer
   of indirection by naming our #defines after features, and then
   connecting systems to the feature defines via the Makefile. But
   maybe Solaris is the only system that has getpassphrase.

I had considered this but thought that it was too heavy weight given
that I'm not aware of other platforms that have this naming split
depending on desired buffer size and that getpass on most platforms
won't have this crippling size limitation.

If it's worth improving both paths for the HAVE_DEV_TTY code on
Solaris, I can add this support if that's considered the better
approach.

Thanks
-Ben

[1] http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libbc/libc/gen/common/getpass.c#65
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302