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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Dec 28, 2011 at 01:11, Sven Strickroth
> <sven.strickroth@tu-clausthal.de> wrote:
>
> Nom nom, some Perl. Thanks for tackling this. Reviewing it as
> requested by Junio.
>

As I'd like to have this in 1.7.9-rc-something, here is my attempt to
rewrite the patch based on your comments, modulo the "chomp()" bit, to
expedite the cycle. Major fixes are:

 * make "prompt" just a helper subroutine. It does not have to be tied to
   any particular repository object anyway.

 * Move the "ah, the environment is there but the value is not set to
   anything sensible" logic from the caller "prompt" to the helper
   "_prompt". For now, forget about an executable whose name is "0" for
   simplicity.

 * Do so using Perl-ish idiom "return unless ($foo)" to avoid potential
   issues with callers who might call it in the list context (I do not
   think it is reasonable to call "prompt" in the list context to begin
   with, though. It is not like the function is to return a list of zero
   or more answers, in which case "@answers = function()" makes
   sense. This is "ask to get a single answer" interface).

 * "open ... or return", not "||".

Sven, does it look agreeable? And more importantly, does it still work? ;-)

Thanks.

 git-svn.perl |   20 +-------------------
 perl/Git.pm  |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index e30df22..6a01176 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4415,25 +4415,7 @@ sub username {
 
 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
-		close(PH);
-	} else {
-		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
-			$password .= $key;
-		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
-	}
+	my $password = Git::prompt($prompt);
 	$password;
 }
 
diff --git a/perl/Git.pm b/perl/Git.pm
index f7ce511..abf9de9 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -58,7 +58,7 @@ require Exporter;
                 command_output_pipe command_input_pipe command_close_pipe
                 command_bidi_pipe command_close_bidi_pipe
                 version exec_path html_path hash_object git_cmd_try
-                remote_refs
+                remote_refs prompt
                 temp_acquire temp_release temp_reset temp_path);
 
 
@@ -512,6 +512,55 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }
 
 
+=item prompt ( PROMPT )
+
+Query user C<PROMPT> and return answer from user.
+
+If an external helper is specified via GIT_ASKPASS or SSH_ASKPASS, it
+is used to interact with the user; otherwise the prompt is given to
+and the answer is read from the terminal.
+
+=cut
+
+sub prompt {
+	my ($prompt) = @_;
+	my $ret;
+	if (!defined $ret) {
+		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
+	}
+	if (!defined $ret) {
+		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	}
+	if (!defined $ret) {
+		$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;
+		}
+		Term::ReadKey::ReadMode('restore');
+		print STDERR "\n";
+		STDERR->flush;
+	}
+	return $ret;
+}
+
+sub _prompt {
+	my ($askpass, $prompt) = @_;
+	return unless ($askpass);
+
+	open my $fh, "-|", $askpass, $prompt
+		or return;
+	my $ret = <$fh>;
+	$ret =~ s/[\012\015]//g; # \n\r
+	close ($fh);
+	return $ret;
+}
+
+
 =item repo_path ()
 
 Return path to the git repository. Must be called on a repository instance.