Junio C Hamano wrote:
> Sven Strickroth <sven.strickroth@xxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html