Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS

[Junio C Hamano <gitster@pobox.com>]

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> git-svn reads passwords from an interactive terminal or by using
> GIT_ASKPASS helper tool. But if GIT_ASKPASS environment variable is not
> set, git-svn does not try to use SSH_ASKPASS as git-core does. This
> cause GUIs (w/o STDIN connected) to hang waiting forever for git-svn to
> complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
>
> Commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795 tried to solve this
> issue, but was incomplete as described above.
>
> Instead of using hand-rolled prompt-response code that only works with
> the interactive terminal, a reusable prompt() method is introduced in
> this commit. This change also adds a fallback to SSH_ASKPASS.
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

Thanks. Vastly more readable ;-)

I only have a few minor nits, and request for extra set of eyeballs from
Perl-y people.

>  sub _read_password {
>  	my ($prompt, $realm) = @_;
> -	my $password = '';
> -	if (exists $ENV{GIT_ASKPASS}) {
> -		open(PH, "-|", $ENV{GIT_ASKPASS}, $prompt);
> -		$password = <PH>;
> -		$password =~ s/[\012\015]//; # \n\r
> - ...
> -		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> -			last if $key =~ /[\012\015]/; # \n\r
> -			$password .= $key;
> -		}
> - ...
> +	my $password = Git->prompt($prompt);
>  	$password;
>  }
> ...
> +Check if GIT_ASKPASS or SSH_ASKPASS is set, use first matching for querying
> +user and return answer. If no *_ASKPASS variable is set, the variable is
> +empty or an error occoured, the terminal is tried as a fallback.

Looks like a description that is correct, but I feel a slight hiccup when
trying to read the first sentence aloud.  Perhaps other reviewers on the
list can offer an easier to read alternative?

> +sub prompt {
> +	my ($self, $prompt) = _maybe_self(@_);
> +	my $ret;
> +	if (exists $ENV{'GIT_ASKPASS'}) {
> +		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
> +	}
> +	if (!defined $ret && exists $ENV{'SSH_ASKPASS'}) {
> +		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
> +	}
> +	if (!defined $ret) {
> +		print STDERR $prompt;
> +		STDERR->flush;
> +		require Term::ReadKey;
> +		Term::ReadKey::ReadMode('noecho');
> +		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> +			last if $key =~ /[\012\015]/; # \n\r
> +			$ret .= $key;

Unlike the original in _read_password, $ret ($password over there) is left
"undef" here; I am wondering if "$ret .= $key" might trigger a warning and
if that is the case, probably we should have an explicit "$ret = '';"
before going into the while loop.

> +sub _prompt {
> +	my ($askpass, $prompt) = @_;
> +	unless ($askpass) {
> +		return undef;
> +	}

Perl gurus on the list might prefer to rewrite this with statement
modifier as "return undef unless (...);" but I am not one of them.

> +	my $ret;
> +	open my $fh, "-|", $askpass, $prompt || return undef;

I am so used see this spelled with the lower-precedence "or" like this

	open my $fh, "-|", $askpass, $prompt
        	or return undef;

that I am no longer sure if the use of "||" is Ok here. Help from Perl
gurus on the list?

> +	$ret = <$fh>;
> +	$ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected

The original reads one line from the helper process, removes the first \n
or \r (expecting there is only one), and returns the result. The new code
reads one line, removes all \n and \r everywhere, and returns the result.

I do not think it makes any difference in practice, but shouldn't this
logically be more like "s/\r?\n$//", that is "remove the CRLF or LF at the
end"?

> +	close ($fh);

It seems that we aquired a SP after "close" compared to the
original. What's the prevailing coding style in our Perl code?

This close() of pipe to the subprocess is where a lot of error checking
happens, no? Can this return an error?

I can see the original ignored an error condition, but do we care, or not
care?