Sven Strickroth <sven.strickroth@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> > --- 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? -- 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