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

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano wrote:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> 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?

Perhaps

  Query user for password with given PROMPT and return answer.  It respects
  GIT_ASKPASS and SSH_ASKPASS environment variables, with terminal in a
  password mode (no echo) as a fallback.  Returns undef if it cannot ask
  for password. 

> > +sub prompt {
> > +	my ($self, $prompt) = _maybe_self(@_);
> > +	my $ret;
> > +	if (exists $ENV{'GIT_ASKPASS'}) {

Wouldn't it be simpler and more resilent to just check for $ENV{'GIT_ASKPASS'}?
Assuming that nobody uses command named '0' it would cover both GIT_ASKPASS
not being set (!exists) and being set to empty value (eq '').

> > +		$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;

I wonder if the last part wouldn't be better to be refactored into
a separate subroutine, e.g. _prompt_readkey.

> 
> 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.

No that is not a problem.  In Perl undefined variable functions as 0 in
numeric context ($foo++), as '' in string context ($foo .= $key), and []
in arrayref context (push @$foo, $key).

> > +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?

It is incorrect, which you can check with B::Deparse.

$ perl -MO=Deparse,-p -e 'open my $fh, "-|", $askpass, $prompt || return undef;'

  open(my $fh, '-|', $askpass, ($prompt || return(undef)));
 
Anyway, wouldn't it be simpler and better to use command_oneline or its
backend here?

> > +	$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?
 
If we use command_oneline or its backend we wouldn't have to worry
about this.

-- 
Jakub Narebski
Poland