[PATCH] honour GIT_ASKPASS for querying username in git-svn

[PATCH] honour GIT_ASKPASS for querying username in git-svn

[Sven Strickroth]

>From 8e576705ca949c32ff22d3216006073ee70652eb Mon Sep 17 00:00:00 2001
From: Sven Strickroth <email@cs-ware.de>
Date: Thu, 17 Nov 2011 15:43:25 +0100
Subject: [PATCH 1/2] honour GIT_ASKPASS for querying username

git-svn reads usernames from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to
complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).

Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index e30df22..8ec3dfc 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4403,6 +4403,11 @@ sub username {
 	my $username;
 	if (defined $_username) {
 		$username = $_username;
+	} else if (exists $ENV{GIT_ASKPASS}) {
+		open(PH, "-|", $ENV{GIT_ASKPASS}, "Username: ");
+		$username = <PH>;
+		$username =~ s/[\012\015]//; # \n\r
+		close(PH);
 	} else {
 		print STDERR "Username: ";
 		STDERR->flush;
-- 
1.7.7.1.msysgit.0

-- 
Best regards,
 Sven Strickroth

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Erik Faye-Lund]

On Thu, Nov 17, 2011 at 4:15 PM, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> From 8e576705ca949c32ff22d3216006073ee70652eb Mon Sep 17 00:00:00 2001
> From: Sven Strickroth <email@cs-ware.de>
> Date: Thu, 17 Nov 2011 15:43:25 +0100
> Subject: [PATCH 1/2] honour GIT_ASKPASS for querying username
>
> git-svn reads usernames from an interactive terminal.
> This behavior cause GUIs to hang waiting for git-svn to
> complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
>
> Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>

IIUC, GIT_ASKPASS is intended for passwords and not usernames. Won't
this cause console-users to not see their username prompted anymore?

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Sven Strickroth]

Am 18.11.2011 12:36 schrieb Erik Faye-Lund:
> On Thu, Nov 17, 2011 at 4:15 PM, Sven Strickroth
> <sven.strickroth@tu-clausthal.de> wrote:
>> From 8e576705ca949c32ff22d3216006073ee70652eb Mon Sep 17 00:00:00 2001
>> From: Sven Strickroth <email@cs-ware.de>
>> Date: Thu, 17 Nov 2011 15:43:25 +0100
>> Subject: [PATCH 1/2] honour GIT_ASKPASS for querying username
>>
>> git-svn reads usernames from an interactive terminal.
>> This behavior cause GUIs to hang waiting for git-svn to
>> complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
>>
>> Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.
>>
>> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> 
> IIUC, GIT_ASKPASS is intended for passwords and not usernames. Won't
> this cause console-users to not see their username prompted anymore?

git also asks for username using the GIT_ASKPASS tool (if GIT_ASKPASS is
set).

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Erik Faye-Lund]

On Fri, Nov 18, 2011 at 2:30 PM, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> Am 18.11.2011 12:36 schrieb Erik Faye-Lund:
>> On Thu, Nov 17, 2011 at 4:15 PM, Sven Strickroth
>> <sven.strickroth@tu-clausthal.de> wrote:
>>> From 8e576705ca949c32ff22d3216006073ee70652eb Mon Sep 17 00:00:00 2001
>>> From: Sven Strickroth <email@cs-ware.de>
>>> Date: Thu, 17 Nov 2011 15:43:25 +0100
>>> Subject: [PATCH 1/2] honour GIT_ASKPASS for querying username
>>>
>>> git-svn reads usernames from an interactive terminal.
>>> This behavior cause GUIs to hang waiting for git-svn to
>>> complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
>>>
>>> Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.
>>>
>>> Signed-off-by: Sven Strickroth <email@cs-ware.de>
>>
>> IIUC, GIT_ASKPASS is intended for passwords and not usernames. Won't
>> this cause console-users to not see their username prompted anymore?
>
> git also asks for username using the GIT_ASKPASS tool (if GIT_ASKPASS is
> set).
>

You are right, it does. Documentation/config.txt documents it as being
for passwords without mentioning that it also affects usernames,
that's why I wondered. I've also verified what happens here on my
config, and git-svn doesn't prompt my username here without the patch
either. So consider my comment withdrawn ;)

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Sven Strickroth]

Hi,

there's also another point where git-svn doesn't honour GIT_ASKPASS:

>From 632c264d0de127c35fbe45866ed81e832f357d56 Mon Sep 17 00:00:00 2001
From: Sven Strickroth <email@cs-ware.de>
Date: Sat, 26 Nov 2011 12:01:19 +0100
Subject: [PATCH] honour GIT_ASKPASS for querying further actions on unknown
 certificates

git-svn reads user answers from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to
complete.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   9 +++++++++--
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index e30df22..d7aeb11 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4361,7 +4361,14 @@ prompt:
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
 	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	if (exists $ENV{GIT_ASKPASS}) {
+		print STDERR "\n";
+		open(PH, "-|", $ENV{GIT_ASKPASS}, "Certificate unknown");
+		$choice = lc(substr(<PH> || 'R', 0, 1));
+		close(PH);
+	} else {
+		$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	}
 	if ($choice =~ /^t$/i) {
 		$cred->may_save(undef);
 	} elsif ($choice =~ /^r$/i) {
-- 
1.7.7.1.msysgit.0

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Jeff King]

On Sat, Nov 26, 2011 at 12:33:31PM +0100, Sven Strickroth wrote:

> diff --git a/git-svn.perl b/git-svn.perl
> index e30df22..d7aeb11 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -4361,7 +4361,14 @@ prompt:
>  	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
>  	      "(R)eject or accept (t)emporarily? ";
>  	STDERR->flush;
> -	$choice = lc(substr(<STDIN> || 'R', 0, 1));
> +	if (exists $ENV{GIT_ASKPASS}) {
> +		print STDERR "\n";
> +		open(PH, "-|", $ENV{GIT_ASKPASS}, "Certificate unknown");
> +		$choice = lc(substr(<PH> || 'R', 0, 1));
> +		close(PH);
> +	} else {
> +		$choice = lc(substr(<STDIN> || 'R', 0, 1));
> +	}

Why is the prompt simply "Certificate unknown"? Shouldn't it mention
that the right answers are "(R)eject, accept (t)emporarily, ..."?

That aside, I think this is an improvement over the current code. But
having just been looking at regular git's askpass code, I notice there
are some subtle differences:

  1. Regular git will also respect SSH_ASKPASS

  2. Regular git will ignore an askpass variable that is set but empty.

Perhaps git-svn should be refactored to have a reusable "prompt"
function that respects askpass and tries to behave like C git? It could
even go into the Git perl module.

-Peff

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Sven Strickroth]

Hi,

Am 30.11.2011 07:44 schrieb Jeff King:
> That aside, I think this is an improvement over the current code.
>   1. Regular git will also respect SSH_ASKPASS
>   2. Regular git will ignore an askpass variable that is set but empty.
> Perhaps git-svn should be refactored to have a reusable "prompt"
> function that respects askpass and tries to behave like C git? It could
> even go into the Git perl module.

I honoured all your ideas. Hopefully the patches can be applied now. The new patches
follow (you can also pull from git://github.com/csware/git.git askpass-prompt):

>From b760546c59d1b9982296c19f8eaea6dc225b5a4f Mon Sep 17 00:00:00 2001
From: Sven Strickroth <email@cs-ware.de>
Date: Tue, 27 Dec 2011 00:33:46 +0100
Subject: [PATCH 1/4] add central method for prompting a user using
 GIT_ASKPASS or SSH_ASKPASS

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index f7ce511..8176d47 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,35 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


+=item prompt ( PROMPT)
+
+Checks if GIT_ASKPASS or SSH_ASKPASS is set, and if yes
+use it and return answer from user.
+
+=cut
+
+sub prompt {
+	my ($self, $prompt) = _maybe_self(@_);
+	if (exists $ENV{'GIT_ASKPASS'}) {
+		return _prompt($ENV{'GIT_ASKPASS'}, $prompt);
+	} elsif (exists $ENV{'SSH_ASKPASS'}) {
+		return _prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	} else {
+		return undef;
+	}
+}
+
+sub _prompt {
+	my ($self, $askpass, $prompt) = _maybe_self(@_);
+	my $ret;
+	open(PH, "-|", $askpass, $prompt);
+	$ret = <PH>;
+	$ret =~ s/[\012\015]//g; # strip \n\r
+	close(PH);
+	return $ret;
+}
+
+
 =item repo_path ()

 Return path to the git repository. Must be called on a repository instance.
-- 
1.7.7.1.msysgit.0

>From ef4c6557d1b0e33440d13c64742d44b2a22143f3 Mon Sep 17 00:00:00 2001
From: Sven Strickroth <email@cs-ware.de>
Date: Tue, 27 Dec 2011 00:34:09 +0100
Subject: [PATCH 2/4] switch to central prompt method

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index eeb83d3..4fd4eca 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4415,13 +4415,8 @@ 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 {
+	my $password = Git->prompt($prompt);;
+	if (!defined $password) {
 		print STDERR $prompt;
 		STDERR->flush;
 		require Term::ReadKey;
-- 
1.7.7.1.msysgit.0

>From d58f41d7b9b8e690c9839f6f7539774da88aa3a4 Mon Sep 17 00:00:00 2001
From: Sven Strickroth <email@cs-ware.de>
Date: Tue, 27 Dec 2011 00:37:43 +0100
Subject: [PATCH 3/4] honour *_ASKPASS for querying username and for querying
 further actions on unknown certificates

git-svn reads usernames (and answers for certificate errors) from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).

Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 4fd4eca..b85a7de 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4357,11 +4357,15 @@ sub ssl_server_trust {
 	                               issuer_dname fingerprint);
 	my $choice;
 prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	$choice = Git->prompt("Certificate unknown. " . $options);
+	if (!defined $choice) {
+		print STDERR $options;
+		STDERR->flush;
+		$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	}
 	if ($choice =~ /^t$/i) {
 		$cred->may_save(undef);
 	} elsif ($choice =~ /^r$/i) {
@@ -4404,6 +4408,9 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
+		$username = Git->prompt("Username");
+	}
+	if (!defined $username) {
 		print STDERR "Username: ";
 		STDERR->flush;
 		chomp($username = <STDIN>);
-- 
1.7.7.1.msysgit.0

>From 2c1dbdae8024f28d17abfbdc7e45865a1277151a Mon Sep 17 00:00:00 2001
From: Sven Strickroth <email@cs-ware.de>
Date: Tue, 27 Dec 2011 00:42:07 +0100
Subject: [PATCH 4/4] ignore empty *_ASKPASS variables

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 8176d47..fade617 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -532,6 +532,9 @@ sub prompt {

 sub _prompt {
 	my ($self, $askpass, $prompt) = _maybe_self(@_);
+	unless ($askpass) {
+		return undef;
+	}
 	my $ret;
 	open(PH, "-|", $askpass, $prompt);
 	$ret = <PH>;
-- 
1.7.7.1.msysgit.0

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Jakub Narebski]

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

> +=item prompt ( PROMPT)
> +
> +Checks if GIT_ASKPASS or SSH_ASKPASS is set, and if yes
> +use it and return answer from user.
> +
> +=cut

I think it would be good idea to describe what this function is for...

> +sub prompt {
> +	my ($self, $prompt) = _maybe_self(@_);
> +	if (exists $ENV{'GIT_ASKPASS'}) {
> +		return _prompt($ENV{'GIT_ASKPASS'}, $prompt);
> +	} elsif (exists $ENV{'SSH_ASKPASS'}) {
> +		return _prompt($ENV{'SSH_ASKPASS'}, $prompt);
> +	} else {
> +		return undef;
> +	}
> +}

...and provide some kind of fallback even if neither of GIT_ASKPASS
nor SSH_ASKPASS are set (perhaps assuming that some Perl packages from
CPAN are installed).

> +sub _prompt {
> +	my ($self, $askpass, $prompt) = _maybe_self(@_);
> +	my $ret;
> +	open(PH, "-|", $askpass, $prompt);
> +	$ret = <PH>;
> +	$ret =~ s/[\012\015]//g; # strip \n\r
> +	close(PH);
> +	return $ret;
> +}

Please, use modern Perl, in particula use lexical filehandles instead
of typeglobs (which are global variables), i.e.

  +	open my $fh, "-|", $askpass, $prompt
  +		or die "...";
  +	$ret = <$fh>;
  +	chomp($ret);
  +	close($fh)
  +		or die "...";


> -- 
> 1.7.7.1.msysgit.0
> 
> From ef4c6557d1b0e33440d13c64742d44b2a22143f3 Mon Sep 17 00:00:00 2001
> From: Sven Strickroth <email@cs-ware.de>
> Date: Tue, 27 Dec 2011 00:34:09 +0100
> Subject: [PATCH 2/4] switch to central prompt method
> 
> Signed-off-by: Sven Strickroth <email@cs-ware.de>

Please send those patches as a separate emails, not concatenated in a
single email (perhaps even with cover letter).

See Documentation/SubmittingPatches

[...]
-- 
Jakub Narebski

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Sven Strickroth]

Am 27.12.2011 15:33 schrieb Jakub Narebski:
>> +sub prompt {
>> +	my ($self, $prompt) = _maybe_self(@_);
>> +	if (exists $ENV{'GIT_ASKPASS'}) {
>> +		return _prompt($ENV{'GIT_ASKPASS'}, $prompt);
>> +	} elsif (exists $ENV{'SSH_ASKPASS'}) {
>> +		return _prompt($ENV{'SSH_ASKPASS'}, $prompt);
>> +	} else {
>> +		return undef;
>> +	}
>> +}
> 
> ...and provide some kind of fallback even if neither of GIT_ASKPASS
> nor SSH_ASKPASS are set (perhaps assuming that some Perl packages from
> CPAN are installed).

If neither of GIT_ASKPASS nor SSH_ASKPASS are set the caller has to
handle the request. This has to be done this way, because of lots of
different needs (username, password (no echo) and so on).

>> +sub _prompt {
>> +	my ($self, $askpass, $prompt) = _maybe_self(@_);
>> +	my $ret;
>> +	open(PH, "-|", $askpass, $prompt);
>> +	$ret = <PH>;
>> +	$ret =~ s/[\012\015]//g; # strip \n\r
>> +	close(PH);
>> +	return $ret;
>> +}
> 
> Please, use modern Perl, in particula use lexical filehandles instead
> of typeglobs (which are global variables), i.e.

I used the same style as I found in Git.pm (see lines I removed in patch 2).

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn

[Jakub Narebski]

On Tue, 27 Dec 2011, Sven Strickroth wrote:
> Am 27.12.2011 15:33 schrieb Jakub Narebski:

>>> +sub prompt {
>>> +	my ($self, $prompt) = _maybe_self(@_);
>>> +	if (exists $ENV{'GIT_ASKPASS'}) {
>>> +		return _prompt($ENV{'GIT_ASKPASS'}, $prompt);
>>> +	} elsif (exists $ENV{'SSH_ASKPASS'}) {
>>> +		return _prompt($ENV{'SSH_ASKPASS'}, $prompt);
>>> +	} else {
>>> +		return undef;
>>> +	}
>>> +}
>> 
>> ...and provide some kind of fallback even if neither of GIT_ASKPASS
>> nor SSH_ASKPASS are set (perhaps assuming that some Perl packages from
>> CPAN are installed).
> 
> If neither of GIT_ASKPASS nor SSH_ASKPASS are set the caller has to
> handle the request. This has to be done this way, because of lots of
> different needs (username, password (no echo) and so on).

I think that Git.pm and therefore git commands written in Perl should
behave the same as git command written in C; and I think builtins do
use common gitprompt fallback.
 
>>> +sub _prompt {
>>> +	my ($self, $askpass, $prompt) = _maybe_self(@_);
>>> +	my $ret;
>>> +	open(PH, "-|", $askpass, $prompt);
>>> +	$ret = <PH>;
>>> +	$ret =~ s/[\012\015]//g; # strip \n\r
>>> +	close(PH);
>>> +	return $ret;
>>> +}
>> 
>> Please, use modern Perl, in particula use lexical filehandles instead
>> of typeglobs (which are global variables), i.e.
> 
> I used the same style as I found in Git.pm (see lines I removed in patch 2).

Yes, that should be fixed (together with host of other issues), but
one should use modern and _better_ way (no possibility of action at
distance).

-- 
Jakub Narebski
Poland

[PATCH 0/5] honour *_ASKPASS for querying user in git-svn

[Sven Strickroth]

Hi,

git-svn reads usernames (and other stuff) from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to complete
(http://code.google.com/p/tortoisegit/issues/detail?id=967).

Again I honoured all your ideas. Hopefully the patches can be applied
now. The new patches follow in the next mails (you can also pull from
git://github.com/csware/git.git askpass-prompt).

The first four patches implement the raw functionality. The last and
fifth patch extends the prompt-method so that it can be used for all
purposes in order to query users.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

[PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS

[Sven Strickroth]

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index f7ce511..7fdf805 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 askpass_prompt
                 temp_acquire temp_release temp_reset temp_path);


@@ -512,6 +512,40 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


+=item askpass_prompt ( PROMPT)
+
+Asks user using *_ASKPASS programs and return answer from user.
+
+Checks if GIT_ASKPASS or SSH_ASKPASS is set, and use first matching for querying
+user and returns answer.
+
+If no *_ASKPASS variable is set, the variable is empty or an error occours,
+it returns undef and the caller has to ask the user (e.g. on terminal).
+
+=cut
+
+sub askpass_prompt {
+	my ($self, $prompt) = _maybe_self(@_);
+	if (exists $ENV{'GIT_ASKPASS'}) {
+		return _askpass_prompt($ENV{'GIT_ASKPASS'}, $prompt);
+	} elsif (exists $ENV{'SSH_ASKPASS'}) {
+		return _askpass_prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	} else {
+		return undef;
+	}
+}
+
+sub _askpass_prompt {
+	my ($self, $askpass, $prompt) = _maybe_self(@_);
+	my $ret;
+	open my $fh, "-|", $askpass, $prompt || return undef;
+	$ret = <$fh>;
+	$ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
+	close ($fh);
+	return $ret;
+}
+
+
 =item repo_path ()

 Return path to the git repository. Must be called on a repository instance.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

[PATCH 2/5] switch to central prompt method

[Sven Strickroth]

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index eeb83d3..25d5da7 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4415,13 +4415,8 @@ 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 {
+	my $password = Git->askpass_prompt($prompt);;
+	if (!defined $password) {
 		print STDERR $prompt;
 		STDERR->flush;
 		require Term::ReadKey;
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

[PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates

[Sven Strickroth]

git-svn reads usernames (and other stuff) from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).

Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 25d5da7..2c99aaa 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4357,11 +4357,15 @@ sub ssl_server_trust {
 	                               issuer_dname fingerprint);
 	my $choice;
 prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	$choice = Git->askpass_prompt("Certificate unknown. " . $options);
+	if (!defined $choice) {
+		print STDERR $options;
+		STDERR->flush;
+		$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	}
 	if ($choice =~ /^t$/i) {
 		$cred->may_save(undef);
 	} elsif ($choice =~ /^r$/i) {
@@ -4378,9 +4382,12 @@ prompt:
 sub ssl_client_cert {
 	my ($cred, $realm, $may_save, $pool) = @_;
 	$may_save = undef if $_no_auth_cache;
-	print STDERR "Client certificate filename: ";
-	STDERR->flush;
-	chomp(my $filename = <STDIN>);
+	my $filename = Git->askpass_prompt("Client certificate filename:");
+	if (!defined $filename) {
+		print STDERR "Client certificate filename: ";
+		STDERR->flush;
+		chomp($filename = <STDIN>);
+	}
 	$cred->cert_file($filename);
 	$cred->may_save($may_save);
 	$SVN::_Core::SVN_NO_ERROR;
@@ -4404,6 +4411,9 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
+		$username = Git->askpass_prompt("Username");
+	}
+	if (!defined $username) {
 		print STDERR "Username: ";
 		STDERR->flush;
 		chomp($username = <STDIN>);
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

[PATCH 4/5] ignore empty *_ASKPASS variables

[Sven Strickroth]

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 7fdf805..c6b3e11 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -537,6 +537,9 @@ sub askpass_prompt {

 sub _askpass_prompt {
 	my ($self, $askpass, $prompt) = _maybe_self(@_);
+	unless ($askpass) {
+		return undef;
+	}
 	my $ret;
 	open my $fh, "-|", $askpass, $prompt || return undef;
 	$ret = <$fh>;
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

[PATCH 5/5] make askpass_prompt a global prompt method for asking users

[Sven Strickroth]

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   37 ++++---------------------------------
 perl/Git.pm  |   43 +++++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 45 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 2c99aaa..1f30dc2 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4360,12 +4360,7 @@ prompt:
 	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	$choice = Git->askpass_prompt("Certificate unknown. " . $options);
-	if (!defined $choice) {
-		print STDERR $options;
-		STDERR->flush;
-		$choice = lc(substr(<STDIN> || 'R', 0, 1));
-	}
+	$choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);
 	if ($choice =~ /^t$/i) {
 		$cred->may_save(undef);
 	} elsif ($choice =~ /^r$/i) {
@@ -4382,13 +4377,7 @@ prompt:
 sub ssl_client_cert {
 	my ($cred, $realm, $may_save, $pool) = @_;
 	$may_save = undef if $_no_auth_cache;
-	my $filename = Git->askpass_prompt("Client certificate filename:");
-	if (!defined $filename) {
-		print STDERR "Client certificate filename: ";
-		STDERR->flush;
-		chomp($filename = <STDIN>);
-	}
-	$cred->cert_file($filename);
+	$cred->cert_file(Git->prompt("Client certificate filename: "));
 	$cred->may_save($may_save);
 	$SVN::_Core::SVN_NO_ERROR;
 }
@@ -4411,12 +4400,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		$username = Git->askpass_prompt("Username");
-	}
-	if (!defined $username) {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git->prompt("Username: ");
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -4425,20 +4409,7 @@ sub username {

 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git->askpass_prompt($prompt);;
-	if (!defined $password) {
-		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, 1);
 	$password;
 }

diff --git a/perl/Git.pm b/perl/Git.pm
index c6b3e11..acc00b4 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 askpass_prompt
+                remote_refs prompt
                 temp_acquire temp_release temp_reset temp_path);


@@ -512,30 +512,49 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


-=item askpass_prompt ( PROMPT)
+=item prompt ( PROMPT , ISPASSWORD )

-Asks user using *_ASKPASS programs and return answer from user.
+Asks user using *_ASKPASS programs or terminal the prompt C<PROMPT> and return answer from user.

 Checks if GIT_ASKPASS or SSH_ASKPASS is set, and use first matching for querying
 user and returns answer.

 If no *_ASKPASS variable is set, the variable is empty or an error occours,
-it returns undef and the caller has to ask the user (e.g. on terminal).
+the querying the user using terminal is tried. If C<ISPASSWORD> is set and true,
+the terminal disables echo.

 =cut

-sub askpass_prompt {
-	my ($self, $prompt) = _maybe_self(@_);
+sub prompt {
+	my ($self, $prompt, $isPassword) = _maybe_self(@_);
+	my $ret;
 	if (exists $ENV{'GIT_ASKPASS'}) {
-		return _askpass_prompt($ENV{'GIT_ASKPASS'}, $prompt);
-	} elsif (exists $ENV{'SSH_ASKPASS'}) {
-		return _askpass_prompt($ENV{'SSH_ASKPASS'}, $prompt);
-	} else {
-		return undef;
+		$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;
+		if (defined $isPassword && $isPassword) {
+			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;
+		} else {
+			chomp($ret = <STDIN>);
+		}
+	}
+	return $ret;
 }

-sub _askpass_prompt {
+sub _prompt {
 	my ($self, $askpass, $prompt) = _maybe_self(@_);
 	unless ($askpass) {
 		return undef;
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS

[Junio C Hamano]

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

> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

Thanks.

Please indicate what area you are touching on the Subject, e.g.

	Subject: [PATCH 1/5] perl/Git.pm: add askpass_prompt() method

and explain what problem it tries to solve, how it tries to solve it, and
why this particular way of solving that problem is a good thing to have in
the proposed commit log message (but see the comments to 2/5 before doing
so).

>  perl/Git.pm |   36 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 35 insertions(+), 1 deletions(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index f7ce511..7fdf805 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 askpass_prompt
>                  temp_acquire temp_release temp_reset temp_path);
>
>
> @@ -512,6 +512,40 @@ C<git --html-path>). Useful mostly only internally.
>  sub html_path { command_oneline('--html-path') }
>
>
> +=item askpass_prompt ( PROMPT)

Is the unbalanced spacing around parentheses your finger slippage?

> +
> +Asks user using *_ASKPASS programs and return answer from user.

Other =item entries in this file (I only checked a couple of earlier ones)
begin with "Construct a new ...", "Execute the given git command ...",
i.e. in imperative mood.

Also I agree with Jakub that it would be more appropriate to start the
description with the purpose and the effect, i.e. what the function is
for, than the internal implementation, i.e. what the function does.

> +Checks if GIT_ASKPASS or SSH_ASKPASS is set, and use first matching for querying
> +user and returns answer.
> +
> +If no *_ASKPASS variable is set, the variable is empty or an error occours,
> +it returns undef and the caller has to ask the user (e.g. on terminal).
> +
> +=cut
> +
> +sub askpass_prompt {
> +	my ($self, $prompt) = _maybe_self(@_);
> +	if (exists $ENV{'GIT_ASKPASS'}) {
> +		return _askpass_prompt($ENV{'GIT_ASKPASS'}, $prompt);
> +	} elsif (exists $ENV{'SSH_ASKPASS'}) {
> +		return _askpass_prompt($ENV{'SSH_ASKPASS'}, $prompt);
> +	} else {
> +		return undef;

Two problems with this if/elsif/else cascade.

 - If _askpass_prompt() fails to open the pipe to ENV{'GIT_ASKPASS'}, it
   will return 'undef' to us. Don't we want to fall back to SSH_ASKPASS in
   such a case?

 - The last "return undef" makes all callers of this method to implement a
   fall-back way somehow. I find it very likely that they will want to use
   the fall-back code that uses Term::ReadKey found in _read_password, and
   they will hate the above askpass_prompt implementation for focing them
   to duplicate the code more than they will appreciate the flexibility
   that they could implement a different fall-back.

> +}
> +
> +sub _askpass_prompt {
> +	my ($self, $askpass, $prompt) = _maybe_self(@_);

I am not sure why you would want _maybe_self() here for this internal
helper function _askpass_prompt that is not even called as a method of
anything.

> +	my $ret;
> +	open my $fh, "-|", $askpass, $prompt || return undef;
> +	$ret = <$fh>;
> +	$ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
> +	close ($fh);
> +	return $ret;
> +}
> +
> +
>  =item repo_path ()
>
>  Return path to the git repository. Must be called on a repository instance.

Re: [PATCH 2/5] switch to central prompt method

[Junio C Hamano]

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

> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

It would be better to have this and the previous patch squashed into one
commit, for three reasons:

 - It is far easier to review the patch that way, because it will show the
   old way to get the password from the user as the removed code and new
   way to do so as the added helper function, enabling reviewers to
   compare the two in a single review session, to see if the change keeps
   the feature bug-to-bug compatible, introduces any regression, and/or
   offers improvements over existing code.

 - If there were a bug in the implementation of askpass_prompt method
   introduced in patch 1 without being used, and the calling codepath
   introduced in patch 2 is bug-free (i.e. it correctly follows the
   calling convention of the new method, only the implementation of the
   method is buggy), bisection will still point at patch 2 that dumped the
   old proven working way and started using the buggy new implementation.

   Of course, it is possible that patch 1 perfectly implements the new
   method and a bug exists in the way the caller introduced in patch 2
   calls the method and in such a case bisection will correctly point out
   that the caller is at fault, but the point of this refactoring is to
   make it harder for callers to make such mistakes.

 - As we can see above the three-dash line, even the author of the series
   could not come up with any justification why the proposed change is a
   good thing in the proposed commit log message for this patch alone (or
   the previous patch alone for that matter). Combining these patches
   together would make it clearer why it may be a good thing, which would
   make it easier to come up with a better log message.

>  git-svn.perl |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index eeb83d3..25d5da7 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -4415,13 +4415,8 @@ 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 {
> +	my $password = Git->askpass_prompt($prompt);;
> +	if (!defined $password) {
>  		print STDERR $prompt;
>  		STDERR->flush;
>  		require Term::ReadKey;

Re: [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates

[Junio C Hamano]

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

> git-svn reads usernames (and other stuff) from an interactive terminal.
> This behavior cause GUIs to hang waiting for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).

(style) Wrap the line perhaps after "to complete".

Does the above mean "GUIs hang, until the user goes back to the terminal
and authenticates"?  Where is the "interactive terminal" connected when
running the GUI?

With that bit information, I think the above is a decent problem
description (i.e. "what problem is this change trying to solve? is it
worth solving?").

The second paragraph (missing) should then discuss what approach is taken
by the proposed patch to solve that problem. Something like

    Instead of using hand-rolled prompt-response code that only works with
    the interactive terminal, use the git_prompt() method introduced in
    the earlier commit.

would suffice (I didn't check what method name you used, though).

> Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.

I checked that commit, and what you wanted to say is unclear. Are you
saying this patch attempts to fix the breakage by that commit? That commit
tried to go in a right direction but did not go far enough and you are
trying to enhance it? Somerthing else?

Which means that you shouldn't have said "Also see..." at all and instead
directly said what you wanted to say here.

Re: [PATCH 4/5] ignore empty *_ASKPASS variables

[Junio C Hamano]

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

> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

I *suspect* that this is a fix-up to a bug in patch 1/5 that lets callers
call _askpass_prompt helper without checking the value of the "askpass",
and if that is the case, this patch should be squashed there.

But there is no justification in the proposed log message above, so I
cannot tell.

>  perl/Git.pm |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 7fdf805..c6b3e11 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -537,6 +537,9 @@ sub askpass_prompt {
>
>  sub _askpass_prompt {
>  	my ($self, $askpass, $prompt) = _maybe_self(@_);
> +	unless ($askpass) {
> +		return undef;
> +	}
>  	my $ret;
>  	open my $fh, "-|", $askpass, $prompt || return undef;
>  	$ret = <$fh>;

Re: [PATCH 5/5] make askpass_prompt a global prompt method for asking users

[Junio C Hamano]

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

> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

I think the end result of having a prompt function that can be used with
or without echoing like Peff's git_prompt() series does is a very good
thing. But then we just should introduce "prompt()" from day one of the
series, instead of introducing a half-featured one "askpass_prompt()" and
then later renaming the callers like this patch does.

It may a good idea to take the stepwise approach like this series does,
but in that case, the proposed log message must explain what the new
"prompt()" function is and does. It is derived from askpass_prompt() and
apparently it does more than its ancestor, but what are differences
between the two?

For example, it is totally unclear why these two are equivalent without
any explanation in the commit log message.

> -	$choice = Git->askpass_prompt("Certificate unknown. " . $options);
> -	if (!defined $choice) {
> -		print STDERR $options;
> -		STDERR->flush;
> -		$choice = lc(substr(<STDIN> || 'R', 0, 1));
> -	}
> +	$choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);

or this:

> -	my $filename = Git->askpass_prompt("Client certificate filename:");
> -	if (!defined $filename) {
> -		print STDERR "Client certificate filename: ";
> -		STDERR->flush;
> -		chomp($filename = <STDIN>);
> -	}
> -	$cred->cert_file($filename);
> +	$cred->cert_file(Git->prompt("Client certificate filename: "));

I *suspect* the difference is that you discarded that "return false at the
end to let the caller do whatever they want" found in patch 1/5 and have
the fallback inside the prompt() funtion now. And if that is the primary
difference between the old "askpass_prompt()" and the new "prompt()", I
tend to think that the series should be restructured to use the "prompt()"
semantics from the beginning. No reason to start with a known-to-be-wrong
way to do a thing and then fix it in a series that is new to the codebase.

Thanks.

Re: [PATCH 5/5] make askpass_prompt a global prompt method for asking users

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I *suspect* the difference is that you discarded that "return false at the
> end to let the caller do whatever they want" found in patch 1/5 and have
> the fallback inside the prompt() funtion now. And if that is the primary
> difference between the old "askpass_prompt()" and the new "prompt()", I
> tend to think that the series should be restructured to use the "prompt()"
> semantics from the beginning. No reason to start with a known-to-be-wrong
> way to do a thing and then fix it in a series that is new to the codebase.

After reading the series again, I think the right structure of this patch
series should be more like this:

 (1/3) Add Git->prompt($prompt) and make _read_password in git-svn.perl to
       use it. The prompt method should implement the Term::ReadKey based
       fallback, so that _read_password do not have to roll its own. IOW,
       a squash of your 1/5, 2/5, and a part of 5/5, plus possibly 4/5.

 (2/3) Enhance Git->prompt($prompt, $is_password), and convert the various
       existing terminal interacters to use it. The fallback in the prompt
       method, when it is not reading in a noecho mode, should read a
       single line from the standard input in cooked mode like your 5/5
       does. IOW, a squash of your 3/5 and a part of 5/5.

 (3/3) Possibly tests and docs.

Thanks.

Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS

[Thomas Adam]

On 27 December 2011 20:47, Junio C Hamano <gitster@pobox.com> wrote:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>> +sub askpass_prompt {
>> +     my ($self, $prompt) = _maybe_self(@_);
>> +     if (exists $ENV{'GIT_ASKPASS'}) {
>> +             return _askpass_prompt($ENV{'GIT_ASKPASS'}, $prompt);
>> +     } elsif (exists $ENV{'SSH_ASKPASS'}) {
>> +             return _askpass_prompt($ENV{'SSH_ASKPASS'}, $prompt);
>> +     } else {
>> +             return undef;
>
> Two problems with this if/elsif/else cascade.
>
>  - If _askpass_prompt() fails to open the pipe to ENV{'GIT_ASKPASS'}, it
>   will return 'undef' to us. Don't we want to fall back to SSH_ASKPASS in
>   such a case?
>
>  - The last "return undef" makes all callers of this method to implement a
>   fall-back way somehow. I find it very likely that they will want to use

Not only that, "return undef" will have nasty side-effects if this
subroutine is called in list-context -- it's usually discouraged to
have explicit returns of "undef", where in scalar context that might
be OK, but in list context, the caller will see:

(undef)

and not:

()

i.e., the empty list.

-- Thomas Adam

Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS

[Junio C Hamano]

Thomas Adam <thomas@xteddy.org> writes:

> On 27 December 2011 20:47, Junio C Hamano <gitster@pobox.com> wrote:
>> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>>> +sub askpass_prompt {
>>> +     my ($self, $prompt) = _maybe_self(@_);
>>> +     if (exists $ENV{'GIT_ASKPASS'}) {
>>> +             return _askpass_prompt($ENV{'GIT_ASKPASS'}, $prompt);
>>> +     } elsif (exists $ENV{'SSH_ASKPASS'}) {
>>> +             return _askpass_prompt($ENV{'SSH_ASKPASS'}, $prompt);
>>> +     } else {
>>> +             return undef;
>>
>> Two problems with this if/elsif/else cascade.
>>
>>  - If _askpass_prompt() fails to open the pipe to ENV{'GIT_ASKPASS'}, it
>>   will return 'undef' to us. Don't we want to fall back to SSH_ASKPASS in
>>   such a case?
>>
>>  - The last "return undef" makes all callers of this method to implement a
>>   fall-back way somehow. I find it very likely that they will want to use
>
> Not only that, "return undef" will have nasty side-effects if this
> subroutine is called in list-context -- it's usually discouraged to
> have explicit returns of "undef", where in scalar context that might
> be OK, but in list context, the caller will see:
>
> (undef)
>
> and not:
>
> ()
>
> i.e., the empty list.

Well, for this particular function whose interface is "I'll give you a
prompt, use it to interact with the user and give me what the user gave us
in response", a scalar caller would do

	my $response = askpass_prompt("What is your password?");

while a list context caller would instead do

	my ($response) = askpass_prompt("What is your password?");

or

	my @answer = askpass_prompt("What is your password?");
        my $response = $answer[0];

and all three callers would get "undef" in $response. I suspect returning
(undef) is a better thing to do, than relying that

        my @answer = ();
        my $response = $answer[0];

happes to give undef to $response because the access goes beyond the end
of the array, no?

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

[Sven Strickroth]

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>
---
 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 eeb83d3..bcee8aa 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..b1c7c50 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.
+
+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.
+
+=cut
+
+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;
+		}
+		Term::ReadKey::ReadMode('restore');
+		print STDERR "\n";
+		STDERR->flush;
+	}
+	return $ret;
+}
+
+sub _prompt {
+	my ($askpass, $prompt) = @_;
+	unless ($askpass) {
+		return undef;
+	}
+	my $ret;
+	open my $fh, "-|", $askpass, $prompt || return undef;
+	$ret = <$fh>;
+	$ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
+	close ($fh);
+	return $ret;
+}
+
+
 =item repo_path ()

 Return path to the git repository. Must be called on a repository instance.
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

[PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

git-svn reads usernames and other user queries from an interactive
terminal. 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).

This change extends the Git->prompt method, so that it can also be used
for non password queries, and makes use of it instead of using
hand-rolled prompt-response code that only works with the interactive
terminal.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   16 +++++-----------
 perl/Git.pm  |   25 +++++++++++++++----------
 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index bcee8aa..1f30dc2 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4357,11 +4357,10 @@ sub ssl_server_trust {
 	                               issuer_dname fingerprint);
 	my $choice;
 prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	$choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);
 	if ($choice =~ /^t$/i) {
 		$cred->may_save(undef);
 	} elsif ($choice =~ /^r$/i) {
@@ -4378,10 +4377,7 @@ prompt:
 sub ssl_client_cert {
 	my ($cred, $realm, $may_save, $pool) = @_;
 	$may_save = undef if $_no_auth_cache;
-	print STDERR "Client certificate filename: ";
-	STDERR->flush;
-	chomp(my $filename = <STDIN>);
-	$cred->cert_file($filename);
+	$cred->cert_file(Git->prompt("Client certificate filename: "));
 	$cred->may_save($may_save);
 	$SVN::_Core::SVN_NO_ERROR;
 }
@@ -4404,9 +4400,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git->prompt("Username: ");
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -4415,7 +4409,7 @@ sub username {

 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git->prompt($prompt);
+	my $password = Git->prompt($prompt, 1);
 	$password;
 }

diff --git a/perl/Git.pm b/perl/Git.pm
index b1c7c50..62b824c 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -512,18 +512,19 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


-=item prompt ( PROMPT )
+=item prompt ( PROMPT , ISPASSWORD )

 Query user C<PROMPT> and return answer from user.

 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.
+If C<ISPASSWORD> is set and true, the terminal disables echo.

 =cut

 sub prompt {
-	my ($self, $prompt) = _maybe_self(@_);
+	my ($self, $prompt, $isPassword) = _maybe_self(@_);
 	my $ret;
 	if (exists $ENV{'GIT_ASKPASS'}) {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
@@ -534,15 +535,19 @@ sub 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;
+		if (defined $isPassword && $isPassword) {
+			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;
+		} else {
+			chomp($ret = <STDIN>);
 		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
 	}
 	return $ret;
 }
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Junio C Hamano]

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?

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

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

> @@ -4357,11 +4357,10 @@ sub ssl_server_trust {
>  	                               issuer_dname fingerprint);
>  	my $choice;
>  prompt:
> -	print STDERR $may_save ?
> +	my $options = $may_save ?
>  	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
>  	      "(R)eject or accept (t)emporarily? ";
> -	STDERR->flush;
> -	$choice = lc(substr(<STDIN> || 'R', 0, 1));
> +	$choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);

I am afraid the extra "Certificate unknown. " prefix may make the prompt
way too long to fit on a line on the terminal or in the GUI. Would it be
Ok to perhaps add LF to make it a multi-line prompt? Do GUI based helpers
make that into a dialog box with multi-line prompt, or do they just barf?

>  	if ($choice =~ /^t$/i) {
>  		$cred->may_save(undef);

We seem to have lost lc() there, but it probably is deliberate and
harmless, as the value is checked with /^x$/i later.

As we are making sure $choice has a single character anyway, I think that
checking with "=~ /^x$/i" is unnecessarily ugly and wrong, even though it
is obviously not the fault of this patch.

> @@ -4378,10 +4377,7 @@ prompt:
>  sub ssl_client_cert {
>  	my ($cred, $realm, $may_save, $pool) = @_;
>  	$may_save = undef if $_no_auth_cache;
> -	print STDERR "Client certificate filename: ";
> -	STDERR->flush;
> -	chomp(my $filename = <STDIN>);
> -	$cred->cert_file($filename);
> +	$cred->cert_file(Git->prompt("Client certificate filename: "));
>  	$cred->may_save($may_save);
>  	$SVN::_Core::SVN_NO_ERROR;
>  }

We may later add an option to "prompt" method to allow the caller to say
that we are asking for a filename, and let GUI prompt helper to run a file
picker, but I think that is outside the immediate scope of this patch.
Just a thing for the future to keep in mind.

This patch itself looks almost perfect to me (modulo the above minor
nits), and except that it textually depends on 1/2 that may need to be
updated.

Thanks.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 28.12.2011 03:41 schrieb Junio C Hamano:
> I am afraid the extra "Certificate unknown. " prefix may make the prompt
> way too long to fit on a line on the terminal or in the GUI. Would it be
> Ok to perhaps add LF to make it a multi-line prompt? Do GUI based helpers
> make that into a dialog box with multi-line prompt, or do they just barf?

LF is problematic. But we could do $prompt =~ s/\n/ /g; in _prompt()-method.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Sven Strickroth]

Am 28.12.2011 03:34 schrieb Junio C Hamano:
>> +	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?

close() can return a number in case of an error, but we already got our
response/line, so why care?

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Jakub Narebski]

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

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

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

> Am 28.12.2011 03:41 schrieb Junio C Hamano:
>> I am afraid the extra "Certificate unknown. " prefix may make the prompt
>> way too long to fit on a line on the terminal or in the GUI. Would it be
>> Ok to perhaps add LF to make it a multi-line prompt? Do GUI based helpers
>> make that into a dialog box with multi-line prompt, or do they just barf?
>
> LF is problematic. But we could do $prompt =~ s/\n/ /g; in _prompt()-method.

I actually was hoping that the answer is "it depends on the helper
specified by *_ASKPASS".

In any case, let's not add that extra "Certificate unknown. " prefix at
all to avoid regressions and queue this patch series for real.

After somebody comes up with a way to deal with overlong prompt, building
on top of this series, we can work on making this particular prompt longer
and more descriptive.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I actually was hoping that the answer is "it depends on the helper
> specified by *_ASKPASS".
>
> In any case, let's not add that extra "Certificate unknown. " prefix at
> all to avoid regressions and queue this patch series for real.
>
> After somebody comes up with a way to deal with overlong prompt, building
> on top of this series, we can work on making this particular prompt longer
> and more descriptive.

I've queued the two patches with minor tweaks.

I think the first patch is a definite improvement for both GUI users and
terminal users who use the *_ASKPASS environment variable. Other parts of
git already asks the latter their password using *_ASKPASS anyway, so I do
not foresee complaints from them saying that git-svn suddenly stopped
reading the password from the terminal.

I am however not sure if the second patch in this series is a good thing
in the current shape. For GUI users who do not have a terminal, earlier
they couldn't respond to these questions but now they can, so in that
narrow sense we are not going backwards.

But for people who use *_ASKPASS and are working from the terminal, it is
a regression to ask these non-password questions using *_ASKPASS. Most
likely, these helpers that are designed for password entry will hide what
is typed, and I also wouldn't be surprised if some of them have fairly low
input-length restriction that may be shorter than a long-ish pathname that
users might want to give as an answer, which they could do in the terminal
based interaction but will become impossible with this patch.

I suspect that we would need to enhance *_ASKPASS interface first, so that
we can ask things other than passwords. Until that happens, I do not think
we should apply the second patch to use *_ASKPASS for non-passwords.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 28.12.2011 22:38 schrieb Junio C Hamano:
> I am however not sure if the second patch in this series is a good thing
> in the current shape. For GUI users who do not have a terminal, earlier
> they couldn't respond to these questions but now they can, so in that
> narrow sense we are not going backwards.
> 
> But for people who use *_ASKPASS and are working from the terminal, it is
> a regression to ask these non-password questions using *_ASKPASS. Most
> likely, these helpers that are designed for password entry will hide what
> is typed, and I also wouldn't be surprised if some of them have fairly low
> input-length restriction that may be shorter than a long-ish pathname that
> users might want to give as an answer, which they could do in the terminal
> based interaction but will become impossible with this patch.
> 
> I suspect that we would need to enhance *_ASKPASS interface first, so that
> we can ask things other than passwords. Until that happens, I do not think
> we should apply the second patch to use *_ASKPASS for non-passwords.

git-core also asks for username using *_ASKPASS, this is the reason why
I implemented it this way. I noticed it when I tried to push to google
code (using https).

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

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

>> I suspect that we would need to enhance *_ASKPASS interface first, so that
>> we can ask things other than passwords. Until that happens, I do not think
>> we should apply the second patch to use *_ASKPASS for non-passwords.
>
> git-core also asks for username using *_ASKPASS, this is the reason why
> I implemented it this way. I noticed it when I tried to push to google
> code (using https).

I thought that was updated with Peff's series recently?

In any case, your username has a lot minor annoyance factor if we force
you to type in blind, but the second patch in your series ask things other
than that using the same mechanism, so it is not a good excuse for this
usability regression in git-svn, I would think.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 28.12.2011 23:29 schrieb Junio C Hamano:
>>> I suspect that we would need to enhance *_ASKPASS interface first, so that
>>> we can ask things other than passwords. Until that happens, I do not think
>>> we should apply the second patch to use *_ASKPASS for non-passwords.
>>
>> git-core also asks for username using *_ASKPASS, this is the reason why
>> I implemented it this way. I noticed it when I tried to push to google
>> code (using https).
> 
> I thought that was updated with Peff's series recently?

So, was this changed? git-core doesn't ask for a username using
*_ASKPASS helpers anymore?

> In any case, your username has a lot minor annoyance factor if we force
> you to type in blind, but the second patch in your series ask things other
> than that using the same mechanism, so it is not a good excuse for this
> usability regression in git-svn, I would think.

Yeah, typing a whole path is more annoying. But not being able to clone
(push, ...) w/o being able to type in a username or accept an unknown
certificate is also problematic.

I talked off-list with Junio and he proposed to use another environment
variable (e.g. GIT_DIALOG for a different tool) to solve these issues.

A good way could be to define the GIT_DIALOG-tools to have two
parameters. First (pass|text|filename|...) with fallback to text, this
way one can implement a password field, a text field, a file chooser (on
type filename) and it is still extendable for e.g. directory choosers
(if we might need that)...

To make it even more complicated one could also say that we propose more
parameters (e.g. two additional parameters for working copy directory
and repository), so that answers can be stored by the dialog-helper in
the .git-directory.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Jeff King]

On Fri, Dec 30, 2011 at 05:40:47AM +0100, Sven Strickroth wrote:

> >> git-core also asks for username using *_ASKPASS, this is the reason why
> >> I implemented it this way. I noticed it when I tried to push to google
> >> code (using https).
> > 
> > I thought that was updated with Peff's series recently?
> 
> So, was this changed? git-core doesn't ask for a username using
> *_ASKPASS helpers anymore?

No, it will. It's only that we will echo characters when using the
terminal prompt. In theory we could have an ASKPASS-style interface that
would would echo characters, but there's no such interface in common
use (i.e., we would have to invent it).

> I talked off-list with Junio and he proposed to use another environment
> variable (e.g. GIT_DIALOG for a different tool) to solve these issues.
> 
> A good way could be to define the GIT_DIALOG-tools to have two
> parameters. First (pass|text|filename|...) with fallback to text, this
> way one can implement a password field, a text field, a file chooser (on
> type filename) and it is still extendable for e.g. directory choosers
> (if we might need that)...

Yes, like that. I think some windowing toolkits already have programs to
provide dialogs from shell scripts. You might look at them for
inspiration on the interface.

For credentials, it would be nice to be able to create a multi-field
dialog, like:

  Username: <text input>
  Password: <text input>
  Remember password? [checkbox]

I was planning to do something custom for credentials as an extension to
the credential helper protocol, but this could also fall under the
heading of a general prompt helper.

-Peff

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 30.12.2011 14:54 schrieb Jeff King:
>>> I thought that was updated with Peff's series recently?
>>
>> So, was this changed? git-core doesn't ask for a username using
>> *_ASKPASS helpers anymore?
> 
> No, it will. It's only that we will echo characters when using the
> terminal prompt. In theory we could have an ASKPASS-style interface that
> would would echo characters, but there's no such interface in common
> use (i.e., we would have to invent it).

I also updated the _ASKPASS helper of TortoiseGit so that it only shows
asterisks if "pass" is not contained in the prompt.

> For credentials, it would be nice to be able to create a multi-field
> dialog, like:
> 
>   Username: <text input>
>   Password: <text input>
>   Remember password? [checkbox]
> 
> I was planning to do something custom for credentials as an extension to
> the credential helper protocol, but this could also fall under the
> heading of a general prompt helper.

This might be problematic, because (for git-svn) username and password
are not requested together.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

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

> Am 30.12.2011 14:54 schrieb Jeff King:
> ...
>>   Username: <text input>
>>   Password: <text input>
>>   Remember password? [checkbox]
>> 
>> I was planning to do something custom for credentials as an extension to
>> the credential helper protocol, but this could also fall under the
>> heading of a general prompt helper.
>
> This might be problematic, because (for git-svn) username and password
> are not requested together.

I do not think Peff means the dialog must ask these three items at the
same time. The point is that other codepaths know they need to ask them
and would benefit if they can instruct the dialog external helper to ask
them in a single interaction. So if your callsite does not ask them
together, it is OK. You can keep asking them separately in two dialog
interactions.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 28.12.2011 22:38 schrieb Junio C Hamano:
> I am however not sure if the second patch in this series is a good thing
> in the current shape. For GUI users who do not have a terminal, earlier
> they couldn't respond to these questions but now they can, so in that
> narrow sense we are not going backwards.

> But for people who use *_ASKPASS and are working from the terminal, it is
> a regression to ask these non-password questions using *_ASKPASS. Most
> likely, these helpers that are designed for password entry will hide what
> is typed, and I also wouldn't be surprised if some of them have fairly low
> input-length restriction that may be shorter than a long-ish pathname that
> users might want to give as an answer, which they could do in the terminal
> based interaction but will become impossible with this patch.

I'm still for the second patch to be applied (maybe w/o the certificate
filename prompt), too, because this makes git-svn behave the save way as
git-core does (especially asking for username).

Do you think that ppl. mainly using the terminal have *_ASKPASS set?
Most GUIs I know do set it automatically.

I agree that a new interface is needed (working on a patch), but before
we hurry, we should make git-core and git-svn behave the same way.

Btw. git-svn also does not honour git-credentials.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 01.01.2012 10:11 schrieb Junio C Hamano:
> I do not think Peff means the dialog must ask these three items at the
> same time. The point is that other codepaths know they need to ask them
> and would benefit if they can instruct the dialog external helper to ask
> them in a single interaction. So if your callsite does not ask them
> together, it is OK. You can keep asking them separately in two dialog
> interactions.

Sure. This is possible with my proposed interface.

Two parameters should be sufficient, since we get the path to the
repository from the CWD.

TYPE: 'text' (default), 'pass', 'userpass' (username + password in one
dialog), 'filename'
PROMPT

Am I missing something?

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Following you can find my first proposal, based upon patch 1. It's only the
perl part, because I haven't checked out Peffs git_prompt patch(es) and to
avoid double work.

ATM I'm unsure about the 'username' type, but I think it's quite necessary
to make git-svn behave like git-core in case of asking for a username. A type
'userpass' (username and password in one dialog) isn't mentioned here, because
it's not necessary for the git-svn part, but we should also specify/document it
if we want to use it in the future.

Btw. happy new year! ;)

---
 git-svn.perl |   24 +++++++------------
 perl/Git.pm  |   70 ++++++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 58 insertions(+), 36 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 26d3559..54cf77f 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4356,17 +4356,16 @@ sub ssl_server_trust {
 	        map $cert_info->$_, qw(hostname valid_from valid_until
 	                               issuer_dname fingerprint);
 	my $choice;
-prompt:
-	print STDERR $may_save ?
+	my $prompt = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
-	if ($choice =~ /^t$/i) {
+prompt:
+	$choice = lc(substr(Git->prompt($prompt) || 'R', 0, 1));
+	if ($choice eq "t") {
 		$cred->may_save(undef);
-	} elsif ($choice =~ /^r$/i) {
+	} elsif ($choice eq "r") {
 		return -1;
-	} elsif ($may_save && $choice =~ /^p$/i) {
+	} elsif ($may_save && $choice eq "p") {
 		$cred->may_save($may_save);
 	} else {
 		goto prompt;
@@ -4378,10 +4377,7 @@ prompt:
 sub ssl_client_cert {
 	my ($cred, $realm, $may_save, $pool) = @_;
 	$may_save = undef if $_no_auth_cache;
-	print STDERR "Client certificate filename: ";
-	STDERR->flush;
-	chomp(my $filename = <STDIN>);
-	$cred->cert_file($filename);
+	$cred->cert_file(Git->prompt("Client certificate filename: ", 'filename'));
 	$cred->may_save($may_save);
 	$SVN::_Core::SVN_NO_ERROR;
 }
@@ -4404,9 +4400,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git->prompt("Username: ", 'username');
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -4415,7 +4409,7 @@ sub username {

 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git->prompt($prompt);
+	my $password = Git->prompt($prompt, 'pass');
 	$password;
 }

diff --git a/perl/Git.pm b/perl/Git.pm
index ba9a5f2..17ddf40 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -512,52 +512,80 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


-=item prompt ( PROMPT )
+=item prompt ( PROMPT , TYPE )

 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.
+If an external helper is specified via GIT_DIALOG, 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.
+
+Possible values for C<TYPE>:
+- '' or 'text': prompt for normal text (only GIT_DIALOG)
+- 'username': prompt for username (behaves exactly as 'text', but also
+              uses *_ASKPASS)
+- 'pass': prompt for password, echoing of what is typed is disabled on
+          the terminal, GUI tool shows asterisks.
+- 'filename': prompt for a filename, GUI tool migh provide file chooser
+              (only GIT_DIALOG)

 =cut

 sub prompt {
-	my ($self, $prompt) = _maybe_self(@_);
+	my ($self, $prompt, $type) = _maybe_self(@_);
+	$type = 'text' unless ($type);
+	my $useAskPass = ($type eq 'pass' || $type eq 'username');
 	my $ret;
 	if (!defined $ret) {
-		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
+		$ret = _promptgitdialog($ENV{'GIT_DIALOG'}, $prompt, $type);
 	}
-	if (!defined $ret) {
-		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	if ($useAskPass && !defined $ret) {
+		$ret = _promptaskpass($ENV{'GIT_ASKPASS'}, $prompt);
+	}
+	if ($useAskPass && !defined $ret) {
+		$ret = _promptaskpass($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;
+		if ($type eq 'pass') {
+			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;
+		} else {
+			chomp($ret = <STDIN>);
 		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
 	}
 	return $ret;
 }

-sub _prompt {
+sub _promptgitdialog {
+	my ($gitdialog, $prompt, $type) = @_;
+	return undef unless ($askpass);
+	my $ret;
+	open my $fh, "-|", $gitdialog, $type, $prompt
+		or return undef;
+	$ret = <$fh>;
+	$ret =~ s/\r?\n$//; # strip \r\n, chomp does not work on all systems (i.e. windows) as expected
+	close ($fh);
+	return $ret;
+}
+
+sub _promptaskpass {
 	my ($askpass, $prompt) = @_;
-	unless ($askpass) {
-		return undef;
-	}
+	return undef unless ($askpass);
 	my $ret;
 	open my $fh, "-|", $askpass, $prompt
 		or return undef;
 	$ret = <$fh>;
-	$ret =~ s/[\012\015]//g; # \n\r
+	$ret =~ s/\r?\n$//; # strip \r\n, chomp does not work on all systems (i.e. windows) as expected
 	close ($fh);
 	return $ret;
 }
-- 

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Ævar Arnfjörð Bjarmason]

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.

> diff --git a/git-svn.perl b/git-svn.perl
> index eeb83d3..bcee8aa 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..b1c7c50 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.
> +
> +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.
> +
> +=cut
> +
> +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;
> +               }
> +               Term::ReadKey::ReadMode('restore');
> +               print STDERR "\n";
> +               STDERR->flush;
> +       }
> +       return $ret;
> +}

Personally I prefer not to write code I don't have to write. Here
you're doing:

    my ($self, $prompt) = _maybe_self(@_);

And then never using $self, so there's actually no reason for this to
be an object/class function. It could just be called as:

    my $password = Git::prompt($prompt);

Instead of:

    my $password = Git->prompt($prompt);

Which means you could change the first line to just:

    my ($prompt) = @_;

Which wouldn't leave the reader wondering why this needs to maybe
construct an object just to throw it away.

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

The empty list is false in Perl, so explicitly returning undef is
usually the wrong thing. It means that in list context you'll return a
true list (consisting of one undef element), instead of an empty false
one.

    $ perl -MData::Dump=dump -wle 'sub a { return } sub b { return
undef }; my @lists = ([a()], [b()]); for (@lists) { print @$_ ? "true"
: "false" }'
    false
    true

You're not running into that error here since you always use scalar
context. But it's better just to do:

    if (xyz) {
        return;
    }

Unless you want this behavior.

But aside from that I find this code a bit bizarre. The caller is
doing:

    if (exists $ENV{'GIT_ASKPASS'}) {
            $ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
    }
    if (!defined $ret && exists $ENV{'SSH_ASKPASS'}) {
            $ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
    }

Which means we check whether *_ASKPASS *exists* in the env hash, and
then the function checks whether that value is true or not.

Which means the code implicitly depends on Perl's idea of true/false
values for the names of those commands. E.g. you can't create a
command called "0" and put it in your $PATH.

I suppose the case you actually care about is someone being able to do:

    export GIT_ASKPASS=

Instead of the better:

    unset GIT_ASKPASS

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

As pointed out already this is a logic error due to the ||.

But even if that worked I think this behavior is a bit
questionable. Why don't we just throw an error at this point? The way
I'd write this would be something like:

    my $pass = _prompt('GIT_ASKPASS', $prompt);

And then:

    sub _prompt {
        my ($env_var, $prompt) = @_;

        # or exists(), depending on whether we insist on "unset"
        return unless length $ENV{$env_var};

        my $command = $ENV{$env_var};
        open my $fh, "-|", $command or die "We can't get your password
from `$command' given in $env_var: $!";
        chomp(my $password = <$fh>);
        close $fh or die "We can't close() `$command' given in $env_var: $!";

        return $password;
    }

Which would give the user an error if his GIT_ASKPASS command
failed. Check the return value of close() too, and re-structure the
passing around of the env variable so we could give a sensible error
message.

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

Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
does in some languages. It means "The Platform's Native
Newline".

Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
until support for it was removed. This is covered in the second
section of "perldoc perlport".

Can you show me a case where it fails, and under what environment
exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
case we could check for that platform specifically.

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

[Sven Strickroth]

Am 03.01.2012 11:17 schrieb Ævar Arnfjörð Bjarmason:
>> +       $ret = <$fh>;
>> +       $ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
> 
> Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
> does in some languages. It means "The Platform's Native
> Newline".
> 
> Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
> until support for it was removed. This is covered in the second
> section of "perldoc perlport".
> 
> Can you show me a case where it fails, and under what environment
> exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
> case we could check for that platform specifically.

I'm using msys perl (shipped with msysgit) and there just using chomp()
did not work.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 3, 2012 at 11:25, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> Am 03.01.2012 11:17 schrieb Ævar Arnfjörð Bjarmason:
>>> +       $ret = <$fh>;
>>> +       $ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
>>
>> Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
>> does in some languages. It means "The Platform's Native
>> Newline".
>>
>> Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
>> until support for it was removed. This is covered in the second
>> section of "perldoc perlport".
>>
>> Can you show me a case where it fails, and under what environment
>> exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
>> case we could check for that platform specifically.
>
> I'm using msys perl (shipped with msysgit) and there just using chomp()
> did not work.

That's odd, what does this print:

    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
"moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
$fh; open my $in, "<", $name or die $!; my $in_str = <$in>; chomp(my
$cin_str = $in_str); print "in_str:<$in_str> cin_str:<$cin_str>
END\n"'

And how about this:

    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
"moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
$fh; open my $in, "<:crlf", $name or die $!; my $in_str = <$in>;
chomp(my $cin_str = $in_str); print "in_str:<$in_str>
cin_str:<$cin_str> END\n"'

It could be that there's some bug in either perl or mingw's build of
perl where it won't turn on the :crlf IO layer by default.

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

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 3, 2012 at 13:03, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Tue, Jan 3, 2012 at 11:25, Sven Strickroth
> <sven.strickroth@tu-clausthal.de> wrote:
>> Am 03.01.2012 11:17 schrieb Ævar Arnfjörð Bjarmason:
>>>> +       $ret = <$fh>;
>>>> +       $ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
>>>
>>> Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
>>> does in some languages. It means "The Platform's Native
>>> Newline".
>>>
>>> Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
>>> until support for it was removed. This is covered in the second
>>> section of "perldoc perlport".
>>>
>>> Can you show me a case where it fails, and under what environment
>>> exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
>>> case we could check for that platform specifically.
>>
>> I'm using msys perl (shipped with msysgit) and there just using chomp()
>> did not work.
>
> That's odd, what does this print:
>
>    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<", $name or die $!; my $in_str = <$in>; chomp(my
> $cin_str = $in_str); print "in_str:<$in_str> cin_str:<$cin_str>
> END\n"'
>
> And how about this:
>
>    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<:crlf", $name or die $!; my $in_str = <$in>;
> chomp(my $cin_str = $in_str); print "in_str:<$in_str>
> cin_str:<$cin_str> END\n"'
>
> It could be that there's some bug in either perl or mingw's build of
> perl where it won't turn on the :crlf IO layer by default.

Or actually you could do this before running your patch, except you
have to change the code to use chomp() instead of your regex hack:

  export PERLIO=crlf

If that makes it work we should just do that somewhere else (e.g. at
the top of Git.pm) if we detect Windows, which'll make chomp() work as
intended everywhere.

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

[Sven Strickroth]

Am 03.01.2012 13:03 schrieb Ævar Arnfjörð Bjarmason:
>> I'm using msys perl (shipped with msysgit) and there just using chomp()
>> did not work.

But why not drop all \n and \r, since we only accept and wait for a
single line?

chomp($str = <STDIN>) works as expected, but for some reason the line I
got from the ASKPASS tool isn't. Btw. the askpass tool provides a string
followed with \n (c++).

> That's odd, what does this print:
> 
>     perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<", $name or die $!; my $in_str = <$in>; chomp(my
> $cin_str = $in_str); print "in_str:<$in_str> cin_str:<$cin_str>
> END\n"'
> 
> And how about this:
> 
>     perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<:crlf", $name or die $!; my $in_str = <$in>;
> chomp(my $cin_str = $in_str); print "in_str:<$in_str>
> cin_str:<$cin_str> END\n"'
> 
> It could be that there's some bug in either perl or mingw's build of
> perl where it won't turn on the :crlf IO layer by default.

I get an error in both cases "Das System kann die angegebene Datei nicht
finden".

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

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

> I'm still for the second patch to be applied (maybe w/o the certificate
> filename prompt), too, because this makes git-svn behave the save way as
> git-core does (especially asking for username).

I do not have much issues against the patch if the filename thing is
excluded as a short-term workaround to avoid regression.

For people who have been used to interact with git-svn from the terminal
but has *_ASKPASS for reasons other than their use of git-svn in their
environment, the change to the username codepath is technically a
regression, as they used to be able to see and correct typo while giving
their username but with the patch *_ASKPASS will kick in and they have to
type in bline, but I am not particularly worried about it. It is something
you type very often and committed to your muscle memory anyway.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Jeff King]

On Tue, Jan 03, 2012 at 10:19:42AM -0800, Junio C Hamano wrote:

> For people who have been used to interact with git-svn from the terminal
> but has *_ASKPASS for reasons other than their use of git-svn in their
> environment, the change to the username codepath is technically a
> regression, as they used to be able to see and correct typo while giving
> their username but with the patch *_ASKPASS will kick in and they have to
> type in bline, but I am not particularly worried about it. It is something
> you type very often and committed to your muscle memory anyway.

There is one difference between how git and ssh use the ASKPASS
variable. In git, we try it _first_, and fall back to asking on the
terminal.  For ssh, they first try the terminal, and fall back to
askpass only when the terminal cannot be opened.

If we tried the terminal first, then it wouldn't be a big deal to use
*_ASKPASS more frequently, since it's a fallback. Of course, that in
itself might be a regression for some people.

I wonder if we should make the order:

  1. GIT_ASKPASS

  2. terminal

  3. SSH_ASKPASS

to help make our use SSH_ASKPASS better match that of ssh. I dunno. I am
not an askpass user these days, so I don't know what people expect or
want.

-Peff

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

[Junio C Hamano]

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

> And how about this:
>
>     perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<:crlf", $name or die $!; my $in_str = <$in>;
> chomp(my $cin_str = $in_str); print "in_str:<$in_str>
> cin_str:<$cin_str> END\n"'
>
> It could be that there's some bug in either perl or mingw's build of
> perl where it won't turn on the :crlf IO layer by default.

I agree that PERLIO may be a solution that is supposed to work, but let's
not go there, at least not yet in this patch. The stripping of \015\012
(not \r\n) was copied from the original code and I would prefer to see the
patch focus on one primary thing it wants to enhance at a time.

Thanks.

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

[Junio C Hamano]

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

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

git-svn reads usernames and other user queries from an interactive
terminal. 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).
git-core already asks for username using *_ASKPASS tools, this commit
also enables git-svn to do so.

This change extends the Git::prompt method, so that it can also be used
for non password queries (e.g. usernames), and makes use of it instead
of using hand-rolled prompt-response code that only works with the
interactive terminal.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   19 ++++++++-----------
 perl/Git.pm  |   27 ++++++++++++++++-----------
 2 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index ade88ae..be713f5 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4356,17 +4356,16 @@ sub ssl_server_trust {
 	        map $cert_info->$_, qw(hostname valid_from valid_until
 	                               issuer_dname fingerprint);
 	my $choice;
-prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
-	if ($choice =~ /^t$/i) {
+prompt:
+	$choice = lc(substr(Git::prompt($options) || 'R', 0, 1));
+	if ($choice eq 't') {
 		$cred->may_save(undef);
-	} elsif ($choice =~ /^r$/i) {
+	} elsif ($choice eq 'r') {
 		return -1;
-	} elsif ($may_save && $choice =~ /^p$/i) {
+	} elsif ($may_save && $choice eq 'p') {
 		$cred->may_save($may_save);
 	} else {
 		goto prompt;
@@ -4404,9 +4403,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git::prompt("Username: ");
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -4415,7 +4412,7 @@ sub username {

 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git::prompt($prompt);
+	my $password = Git::prompt($prompt, 1);
 	$password;
 }

diff --git a/perl/Git.pm b/perl/Git.pm
index 46f11a8..33e68c4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -512,18 +512,19 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


-=item prompt ( PROMPT )
+=item prompt ( PROMPT , ISPASSWORD )

 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.
+If C<ISPASSWORD> is true, the terminal disables echo.

 =cut

 sub prompt {
-	my ($prompt) = @_;
+	my ($prompt, $isPassword) = @_;
 	my $ret;
 	if (!defined $ret) {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
@@ -532,18 +533,22 @@ sub prompt {
 		$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;
+		if ($isPassword) {
+			$ret = '';
+			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;
+		} else {
+			chomp($ret = <STDIN>);
 		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
 	}
 	return $ret;
 }
-- 
1.7.8.msysgit.0

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

[Sven Strickroth]

Am 03.01.2012 23:51 schrieb Junio C Hamano:
> Sven, does it look agreeable? And more importantly, does it still work? ;-)

Works for me :)

I also updated my second patch minutes ago to fit onto the new patch
(w/o the filename stuff).

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Junio C Hamano]

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

> Am 03.01.2012 23:51 schrieb Junio C Hamano:
>> Sven, does it look agreeable? And more importantly, does it still work? ;-)
>
> Works for me :)
>
> I also updated my second patch minutes ago to fit onto the new patch
> (w/o the filename stuff).

Thanks.

For the second patch, I have a feeling that Peff's earlier suggestion to
give precedence to the terminal interaction over SSH_ASKPASS iff we can
open terminal, but I think the first one is OK for 1.7.9.

I'll queue both of them in 'pu' for now just in case others spot silly
mistakes I made while rewriting the first one, though.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

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

> git-svn reads usernames and other user queries from an interactive
> terminal. 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).
> git-core already asks for username using *_ASKPASS tools, this commit
> also enables git-svn to do so.
>
> This change extends the Git::prompt method, so that it can also be used
> for non password queries (e.g. usernames), and makes use of it instead
> of using hand-rolled prompt-response code that only works with the
> interactive terminal.

Now "prompt" is no longer a method but is merely a helper function, so
I've queued this (and 1/2 rewrite we discussed in a separate thread) to
'pu' after rewording the commit log message.

Thanks.

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

[Sven Strickroth]

Am 04.01.2012 01:10 schrieb Junio C Hamano:
> I'll queue both of them in 'pu' for now just in case others spot silly
> mistakes I made while rewriting the first one, though.

I just hit another issue (I created another patch, but we might want to
integrate it into the first one). This is especially needed if we want
to apply my second patch in this mail.
From: Sven Strickroth <email@cs-ware.de>
Date: Wed, 4 Jan 2012 08:32:13 +0100
Subject: [PATCH] Git.pm: check if value is defined before accessing it

Some perl versions, like the one from msys, crash sometimes
if reading from STDIN wasn't successful and chomp is applied
to the variable into which was read.

Errormessage:
Username: Use of uninitialized value in chomp at C:\Program
Files\Git/libexec/git-core\git-svn line 4321.
0 [main] perl.exe" 1916 handle_exceptions: Exception:
STATUS_ACCESS_VIOLATION
1297 [main] perl.exe" 1916 open_stackdumpfile: Dumping stack trace to
perl.exe.stackdump

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 33e68c4..1c96a20 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -547,7 +547,12 @@ sub prompt {
 			print STDERR "\n";
 			STDERR->flush;
 		} else {
-			chomp($ret = <STDIN>);
+			$ret = <STDIN>;
+			if (defined $ret) {
+				chomp($ret);
+			} else {
+				$ret = '';
+			}
 		}
 	}
 	return $ret;

> For the second patch, I have a feeling that Peff's earlier suggestion to
> give precedence to the terminal interaction over SSH_ASKPASS iff we can
> open terminal, but I think the first one is OK for 1.7.9.

We also do the wrong order for querying the password. if we want to
adopt this, we should also update prompt.c, the make both prompt methods
behave the same way again.

The Git.pm part is easy, but I also tried to update prompt.c (untested).
From: Sven Strickroth <email@cs-ware.de>
Date: Wed, 4 Jan 2012 08:44:48 +0100
Subject: [PATCH] Git.pm, prompt: try reading from interactive terminal
before
 using SSH_ASKPASS

SVN tries to read reading from interactive terminal before using
SSH_ASKPASS helper. This change adjust git to behave the same way.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm |    6 +++---
 prompt.c    |   14 +++++++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 1c96a20..6ce193e 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -530,9 +530,6 @@ sub prompt {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
 	}
 	if (!defined $ret) {
-		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
-	}
-	if (!defined $ret) {
 		print STDERR $prompt;
 		STDERR->flush;
 		if ($isPassword) {
@@ -555,5 +552,8 @@ sub prompt {
 		}
 	}
+	if (!defined $ret) {
+		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	}
 	return $ret;
 }


diff --git a/prompt.c b/prompt.c
index 72ab9de..e791619 100644
--- a/prompt.c
+++ b/prompt.c
@@ -52,9 +52,17 @@ char *git_prompt(const char *prompt, int flags)
 	}

 	r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
-	if (!r)
-		die_errno("could not read '%s'", prompt);
-	return r;
+	if (r)
+		return r;
+
+	if (flags & PROMPT_ASKPASS) {
+		const char *askpass;
+		askpass = getenv("SSH_ASKPASS");
+		if (askpass && *askpass)
+			return do_askpass(askpass, prompt);
+	}
+
+	die_errno("could not read '%s'", prompt);
 }

 char *git_getpass(const char *prompt)
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Sven Strickroth]

Am 04.01.2012 08:55 schrieb Sven Strickroth:
> The Git.pm part is easy, but I also tried to update prompt.c (untested).

I said "easy" and then I mailed the wrong/outdated patch :(
I'm sorry for the noise.

From: Sven Strickroth <email@cs-ware.de>
Date: Wed, 4 Jan 2012 08:44:48 +0100
Subject: [PATCH] Git.pm, prompt: try reading from interactive terminal
 before using SSH_ASKPASS

SVN tries to read reading from interactive terminal before using
SSH_ASKPASS helper. This change adjust git to behave the same way.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm |    9 ++++-----
 prompt.c    |   14 +++++++++++---
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 1c96a20..721aef7 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -530,13 +530,9 @@ sub prompt {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
 	}
 	if (!defined $ret) {
-		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
-	}
-	if (!defined $ret) {
 		print STDERR $prompt;
 		STDERR->flush;
 		if ($isPassword) {
-			$ret = '';
 			require Term::ReadKey;
 			Term::ReadKey::ReadMode('noecho');
 			while (defined(my $key = Term::ReadKey::ReadKey(0))) {
@@ -551,10 +547,13 @@ sub prompt {
 			if (defined $ret) {
 				chomp($ret);
 			} else {
-				$ret = '';
+				undef $ret;
 			}
 		}
 	}
+	if (!defined $ret) {
+		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	}
 	return $ret;
 }

diff --git a/prompt.c b/prompt.c
index 72ab9de..e791619 100644
--- a/prompt.c
+++ b/prompt.c
@@ -52,9 +52,17 @@ char *git_prompt(const char *prompt, int flags)
 	}

 	r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
-	if (!r)
-		die_errno("could not read '%s'", prompt);
-	return r;
+	if (r)
+		return r;
+
+	if (flags & PROMPT_ASKPASS) {
+		const char *askpass;
+		askpass = getenv("SSH_ASKPASS");
+		if (askpass && *askpass)
+			return do_askpass(askpass, prompt);
+	}
+
+	die_errno("could not read '%s'", prompt);
 }

 char *git_getpass(const char *prompt)
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Jeff King]

On Wed, Jan 04, 2012 at 09:31:02AM +0100, Sven Strickroth wrote:

> diff --git a/prompt.c b/prompt.c
> index 72ab9de..e791619 100644
> --- a/prompt.c
> +++ b/prompt.c
> @@ -52,9 +52,17 @@ char *git_prompt(const char *prompt, int flags)
>  	}
> 
>  	r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
> -	if (!r)
> -		die_errno("could not read '%s'", prompt);
> -	return r;
> +	if (r)
> +		return r;
> +
> +	if (flags & PROMPT_ASKPASS) {
> +		const char *askpass;
> +		askpass = getenv("SSH_ASKPASS");
> +		if (askpass && *askpass)
> +			return do_askpass(askpass, prompt);
> +	}
> +
> +	die_errno("could not read '%s'", prompt);
>  }

Wouldn't you also have to drop checking of SSH_ASKPASS in the block
right before calling git_terminal_prompt (right before the context in
your patch)?

-Peff

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

[Sven Strickroth]

Am 04.01.2012 14:34 schrieb Jeff King:
> Wouldn't you also have to drop checking of SSH_ASKPASS in the block
> right before calling git_terminal_prompt (right before the context in
> your patch)?

of course :( Thanks for spotting this...

diff --git a/prompt.c b/prompt.c
index 72ab9de..230ac3c 100644
--- a/prompt.c
+++ b/prompt.c
@@ -45,16 +45,23 @@ char *git_prompt(const char *prompt, int flags)
 		askpass = getenv("GIT_ASKPASS");
 		if (!askpass)
 			askpass = askpass_program;
-		if (!askpass)
-			askpass = getenv("SSH_ASKPASS");
 		if (askpass && *askpass)
 			return do_askpass(askpass, prompt);
 	}

 	r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
-	if (!r)
-		die_errno("could not read '%s'", prompt);
-	return r;
+	if (r)
+		return r;
+
+	if (flags & PROMPT_ASKPASS) {
+		const char *askpass;
+
+		askpass = getenv("SSH_ASKPASS");
+		if (askpass && *askpass)
+			return do_askpass(askpass, prompt);
+	}
+
+	die_errno("could not read '%s'", prompt);
 }

 char *git_getpass(const char *prompt)
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Junio C Hamano]

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

> Some perl versions, like the one from msys, crash sometimes
> if reading from STDIN wasn't successful and chomp is applied
> to the variable into which was read.

Depending on how widespread such implementations of Perl are, a patch to
fix other uses of chomps might deserve to be on the maintenance track
independent from this patch series. I seem to find many hits to:

    $ git grep -e 'chomp *([^)@]*='

already in our codebase.

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

[Junio C Hamano]

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

> Am 04.01.2012 08:55 schrieb Sven Strickroth:
>> The Git.pm part is easy, but I also tried to update prompt.c (untested).
>
> I said "easy" and then I mailed the wrong/outdated patch :(
> I'm sorry for the noise.
>
> From: Sven Strickroth <email@cs-ware.de>
> Date: Wed, 4 Jan 2012 08:44:48 +0100
> Subject: [PATCH] Git.pm, prompt: try reading from interactive terminal
>  before using SSH_ASKPASS
>
> SVN tries to read reading from interactive terminal before using
> SSH_ASKPASS helper. This change adjust git to behave the same way.

It might be an accurate description of what Subversion does ("tries to" is
the key phrase).  I however do not know if it is equivalent to what your
patch does.

When GIT_ASKPASS is not set, the $prompt is given to the standard error
stream unconditionally and then the "require Term::ReadKey" codepath is
used. When the terminal is unavailable, you might get undef in $ret and be
able to fall back on SSH_ASKPASS part, but you cannot take back the noise
you have given to the standard error stream.

Is there a way to ask Term::ReadKey (or possibly some other module) if we
will be able to interact with the terminal _before_ we give that prompt?

The simplest would be to do this, I would think, but I didn't test it.

	if (!defined $ret && -t) {
		print STDERR $prompt;
		if ($isPassword) {
                	...
	}
        if (!defined $ret) {
		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
	}

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

[Sven Strickroth]

Am 04.01.2012 19:58 schrieb Junio C Hamano:
> Depending on how widespread such implementations of Perl are, a patch to
> fix other uses of chomps might deserve to be on the maintenance track
> independent from this patch series. I seem to find many hits to:
> 
>     $ git grep -e 'chomp *([^)@]*='
> 
> already in our codebase.

I'm not sure if this is a general chomp problem. I think that this is
more related to a variable which is accessed after a readfailure on STDIN.

Reported to msys team.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

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

[Sven Strickroth]

Hi,

Am 04.01.2012 20:08 schrieb Junio C Hamano:
> Is there a way to ask Term::ReadKey (or possibly some other module) if we
> will be able to interact with the terminal _before_ we give that prompt?
> 
> The simplest would be to do this, I would think, but I didn't test it.
> 
> 	if (!defined $ret && -t) {
> 		print STDERR $prompt;
> 		if ($isPassword) {
>                 	...
> 	}

-t does not help, but I think it's not a big deal if the prompt is printed on
the terminal and also on the ASKPASS-helper.

Using Term::ReadLine seems to help:
...
		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
	}
	if (!defined $ret) {
		use Term::Readline;
		my $term = Term::ReadLine->new("Git.pm");
		if ($isPassword) {
			require Term::ReadKey;
			Term::ReadKey::ReadMode('noecho');
		}
		$ret = $term->readline($prompt);
		if ($isPassword) {
			Term::ReadKey::ReadMode('restore');
			print STDERR "\n";
			
		}
	}
	if (!defined $ret) {
		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
...

But I'm not sure if this is what we want, because you can go with the cursor
over the whole terminal.

A better (working) alternative might be:
...
		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
	}
	use Term::Readline;
	my $term = Term::ReadLine->new("Git.pm");
	if (!defined $ret && fileno($term->IN)) {
 		print STDERR $prompt;
 		if ($isPassword) {
                 	...
 	}
...

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Hi,

Am 03.01.2012 19:40 schrieb Jeff King:
> There is one difference between how git and ssh use the ASKPASS
> variable. In git, we try it _first_, and fall back to asking on the
> terminal.  For ssh, they first try the terminal, and fall back to
> askpass only when the terminal cannot be opened.

I checked out subversion (svn co
http://svn.apache.org/repos/asf/subversion/trunk subversion) and
performed a "grep ASKPASS * -R": Only match in
"contrib\client-side\emacs\psvn.el". So I doubt if subversion really
supports SSH_ASKPASS.

-- 
Best regards,
 Sven Strickroth

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Jakub Narebski]

Sven Strickroth wrote:
> Am 03.01.2012 19:40 schrieb Jeff King:

> > There is one difference between how git and ssh use the ASKPASS
> > variable. In git, we try it _first_, and fall back to asking on the
> > terminal.  For ssh, they first try the terminal, and fall back to
> > askpass only when the terminal cannot be opened.
> 
> I checked out subversion (svn co
> http://svn.apache.org/repos/asf/subversion/trunk subversion) and
> performed a "grep ASKPASS * -R": Only match in
> "contrib\client-side\emacs\psvn.el". So I doubt if subversion really
> supports SSH_ASKPASS.

Doesn't Subversion use SSH directly?  If it is so, the question is
about how SSH itself supports SSH_ASKPASS.

-- 
Jakub Narebski
Poland

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 12.02.2012 17:11 schrieb Jakub Narebski:
> Doesn't Subversion use SSH directly?  If it is so, the question is
> about how SSH itself supports SSH_ASKPASS.

Oh sorry, I mixed up SSH and SVN_ASKPASS. :( Of couse SSH_ASKPASS is
provided by the ssh-client itself.

-- 
Best regards,
 Sven Strickroth

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Jeff King]

On Sun, Feb 12, 2012 at 05:26:43PM +0100, Sven Strickroth wrote:

> Am 12.02.2012 17:11 schrieb Jakub Narebski:
> > Doesn't Subversion use SSH directly?  If it is so, the question is
> > about how SSH itself supports SSH_ASKPASS.
> 
> Oh sorry, I mixed up SSH and SVN_ASKPASS. :( Of couse SSH_ASKPASS is
> provided by the ssh-client itself.

That raises an interesting point for git (I don't remember seeing this
in the previous discussion, so apologies if I'm repeating). We sometimes
use SSH_ASKPASS for internal prompting, and sometimes via calling out to
ssh. So forgetting about git being consistent with the rest of the
world for a moment, I think we are inconsistent with ourselves. E.g.:

  export SSH_ASKPASS=whatever

  # this will try the terminal first, then SSH_ASKPASS, because it is
  # ssh doing the asking
  git push ssh://example.com/repo.git

  # this will try SSH_ASKPASS first, then the terminal, because git is
  # doing the asking
  git push https://example.com/repo.git

So now I'm more convinced than ever that the order should be
GIT_ASKPASS, terminal, SSH_ASKPASS.

-Peff

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   export SSH_ASKPASS=whatever
>
>   # this will try the terminal first, then SSH_ASKPASS, because it is
>   # ssh doing the asking
>   git push ssh://example.com/repo.git

Sorry, you lost me here.  Does "ssh example.com" consult the terminal
first and then fall back to SSH_ASKPASS environment variable?

I was under the impression that SSH_ASKPASS was to either give hands-free
access to the keychain or give GUI experience so that people do not have
to type from their terminals...

>   # this will try SSH_ASKPASS first, then the terminal, because git is
>   # doing the asking
>   git push https://example.com/repo.git
>
> So now I'm more convinced than ever that the order should be
> GIT_ASKPASS, terminal, SSH_ASKPASS.

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Jeff King]

On Tue, Feb 14, 2012 at 02:35:23PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   export SSH_ASKPASS=whatever
> >
> >   # this will try the terminal first, then SSH_ASKPASS, because it is
> >   # ssh doing the asking
> >   git push ssh://example.com/repo.git
> 
> Sorry, you lost me here.  Does "ssh example.com" consult the terminal
> first and then fall back to SSH_ASKPASS environment variable?

Yes. Try:

  SSH_ASKPASS=cat ssh example.com

where cat is any program whose running you could detect (in this case,
because cat will complain to stderr about not being able to open its
argument). And "example.com" must be something that you can actually
make an ssh connection to.

Compare with:

  SSH_ASKPASS=cat setsid ssh example.com

which will realize it has no controlling tty, and fallback to
SSH_ASKPASS.

I actually find the behavior slightly annoying (because sometimes you do
have a controlling terminal, but it is not accessible or obvious to the
user). But I think it's important to be consistent, and provide
GIT_ASKPASS for people who really want to say "no, don't even bother
with the terminal".

> I was under the impression that SSH_ASKPASS was to either give hands-free
> access to the keychain or give GUI experience so that people do not have
> to type from their terminals...

Not exactly. It's useful in two situations (in my experience):

  1. A GUI program spawns an ssh tunnel, and there is no tty on which to
     prompt the user.

  2. Populating an ssh-agent via ssh-add during the user's login
     sequence.

It doesn't work to give a GUI experience to creating a remote terminal
session, since if you have a terminal, it will always prefer to prompt
on the terminal.

-Peff

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

Am 04.01.2012 01:12 schrieb Junio C Hamano:
> Now "prompt" is no longer a method but is merely a helper function, so
> I've queued this (and 1/2 rewrite we discussed in a separate thread) to
> 'pu' after rewording the commit log message.
> 
> Thanks.

Is there a reason why these changes did not get merged? The issues are
still there.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Junio C Hamano]

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

> Am 04.01.2012 01:12 schrieb Junio C Hamano:
>> Now "prompt" is no longer a method but is merely a helper function, so
>> I've queued this (and 1/2 rewrite we discussed in a separate thread) to
>> 'pu' after rewording the commit log message.
>> 
>> Thanks.
>
> Is there a reason why these changes did not get merged? The issues are
> still there.

It is either that it was simply forgotten, or after I wrote the part
you quoted early in January there were discussions later that showed
the patch was not desirable for some reason. I do not recall which.

[PATCH 0/2] second try

[Sven Strickroth]

Hi,

Am 06.10.2012 20:28 schrieb Junio C Hamano:
> It is either that it was simply forgotten, or after I wrote the part
> you quoted early in January there were discussions later that showed
> the patch was not desirable for some reason. I do not recall which.

I noticed no threads about possible problems, so I try again.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

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

[Sven Strickroth]

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 also 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>
---
 perl/Git.pm            | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 perl/Git/SVN/Prompt.pm | 20 +-------------------
 2 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..0a0fe91 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);
 
 
@@ -511,6 +511,52 @@ 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.
+
+Honours GIT_ASKPASS, SSH_ASKPASS environment variables for querying
+the user. If no *_ASKPASS variable is set or an error occoured,
+the terminal is tried as a fallback.
+
+=cut
+
+sub prompt {
+	my ($prompt) = @_;
+	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');
+		$ret = '';
+		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 length $askpass;
+	my $ret;
+	open my $fh, "-|", $askpass, $prompt or return;
+	$ret = <$fh>;
+	$ret =~ s/[\015\012]//g; # strip \r\n, chomp does not work on all systems (i.e. windows) as expected
+	close ($fh);
+	return $ret;
+}
 
 =item repo_path ()
 
diff --git a/perl/Git/SVN/Prompt.pm b/perl/Git/SVN/Prompt.pm
index 3a6f8af..a2cbcc8 100644
--- a/perl/Git/SVN/Prompt.pm
+++ b/perl/Git/SVN/Prompt.pm
@@ -120,25 +120,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;
 }
 
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

[PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

git-svn reads usernames and other user queries from an interactive
terminal. 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).

This change extends the Git::prompt helper, so that it can also be used
for non password queries, and makes use of it instead of using
hand-rolled prompt-response code that only works with the interactive
terminal.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm            | 28 +++++++++++++++++-----------
 perl/Git/SVN/Prompt.pm | 16 +++++++---------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 0a0fe91..3200f4d 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -511,18 +511,19 @@ C<git --html-path>). Useful mostly only internally.
 
 sub html_path { command_oneline('--html-path') }
 
-=item prompt ( PROMPT )
+=item prompt ( PROMPT , ISPASSWORD  )
 
 Query user C<PROMPT> and return answer from user.
 
 Honours GIT_ASKPASS, SSH_ASKPASS environment variables for querying
 the user. If no *_ASKPASS variable is set or an error occoured,
 the terminal is tried as a fallback.
+If C<ISPASSWORD> is set and true, the terminal disables echo.
 
 =cut
 
 sub prompt {
-	my ($prompt) = @_;
+	my ($prompt, $isPassword) = @_;
 	my $ret;
 	if (exists $ENV{'GIT_ASKPASS'}) {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
@@ -533,16 +534,20 @@ sub prompt {
 	if (!defined $ret) {
 		print STDERR $prompt;
 		STDERR->flush;
-		require Term::ReadKey;
-		Term::ReadKey::ReadMode('noecho');
-		$ret = '';
-		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
-			last if $key =~ /[\012\015]/; # \n\r
-			$ret .= $key;
+		if (defined $isPassword && $isPassword) {
+			require Term::ReadKey;
+			Term::ReadKey::ReadMode('noecho');
+			$ret = '';
+			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;
+		} else {
+			chomp($ret = <STDIN>);
 		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
 	}
 	return $ret;
 }
@@ -550,6 +555,7 @@ sub prompt {
 sub _prompt {
 	my ($askpass, $prompt) = @_;
 	return unless length $askpass;
+	$prompt =~ s/\n/ /g;
 	my $ret;
 	open my $fh, "-|", $askpass, $prompt or return;
 	$ret = <$fh>;
diff --git a/perl/Git/SVN/Prompt.pm b/perl/Git/SVN/Prompt.pm
index a2cbcc8..74daa7a 100644
--- a/perl/Git/SVN/Prompt.pm
+++ b/perl/Git/SVN/Prompt.pm
@@ -62,16 +62,16 @@ sub ssl_server_trust {
 	                               issuer_dname fingerprint);
 	my $choice;
 prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
 	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
-	if ($choice =~ /^t$/i) {
+	$choice = lc(substr(Git::prompt("Certificate problem.\n" . $options) || 'R', 0, 1));
+	if ($choice eq 't') {
 		$cred->may_save(undef);
-	} elsif ($choice =~ /^r$/i) {
+	} elsif ($choice eq 'r') {
 		return -1;
-	} elsif ($may_save && $choice =~ /^p$/i) {
+	} elsif ($may_save && $choice eq 'p') {
 		$cred->may_save($may_save);
 	} else {
 		goto prompt;
@@ -109,9 +109,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git::prompt("Username: ");
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -120,7 +118,7 @@ sub username {
 
 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git::prompt($prompt);
+	my $password = Git::prompt($prompt, 1);
 	$password;
 }
 
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 0/2] second try

[Sven Strickroth]

Hi,

Am 11.11.2012 17:40 schrieb Sven Strickroth:
> Am 06.10.2012 20:28 schrieb Junio C Hamano:
>> It is either that it was simply forgotten, or after I wrote the part
>> you quoted early in January there were discussions later that showed
>> the patch was not desirable for some reason. I do not recall which.
> 
> I noticed no threads about possible problems, so I try again.

On November 11th I submitted the updated patches again, however, without
any reaction or comments.

git pull git://github.com/csware/git.git gitsvn-askpass

Maybe the reason that this was forgotten was, that the git-svn code was
rearranged and splitted into different files in the past and the code
did not apply cleanly any more.

So, what's the status of this issue? It's now open for nearly one year.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 0/2] second try

[Junio C Hamano]

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

> Am 11.11.2012 17:40 schrieb Sven Strickroth:
>> Am 06.10.2012 20:28 schrieb Junio C Hamano:
>>> It is either that it was simply forgotten, or after I wrote the part
>>> you quoted early in January there were discussions later that showed
>>> the patch was not desirable for some reason. I do not recall which.
>> 
>> I noticed no threads about possible problems, so I try again.
>
> On November 11th I submitted the updated patches again, however, without
> any reaction or comments.

I think between Peff and me it fell in the cracks during the
hand-off; I do not know about the others, probably people did not
find it interesting perhaps?

I'll add Eric Wong (git-svn submaintainer) to Cc.

Re: [PATCH 0/2] second try

[Sven Strickroth]

Hi,

Am 26.11.2012 05:50 schrieb Junio C Hamano:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> 
>> Am 11.11.2012 17:40 schrieb Sven Strickroth:
>>> Am 06.10.2012 20:28 schrieb Junio C Hamano:
>>>> It is either that it was simply forgotten, or after I wrote the part
>>>> you quoted early in January there were discussions later that showed
>>>> the patch was not desirable for some reason. I do not recall which.
>>>
>>> I noticed no threads about possible problems, so I try again.
>>
>> On November 11th I submitted the updated patches again, however, without
>> any reaction or comments.
> 
> I think between Peff and me it fell in the cracks during the
> hand-off; I do not know about the others, probably people did not
> find it interesting perhaps?
> 
> I'll add Eric Wong (git-svn submaintainer) to Cc.

I received no feedback, so is there any progress on this issue?

I'd really appreciate if we could fix it soon.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 0/2] second try

[Junio C Hamano]

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

> Am 26.11.2012 05:50 schrieb Junio C Hamano:
>> I think between Peff and me it fell in the cracks during the
>> hand-off; I do not know about the others, probably people did not
>> find it interesting perhaps?
>> 
>> I'll add Eric Wong (git-svn submaintainer) to Cc.
>
> I received no feedback, so is there any progress on this issue?

I took a look at it, and from the code-cleanness point of view, I
think it loos more or less right, even though I'd prefer to see the
"fall back on SSH_ASKPASS" bit as a separate patch, either before or
after [1/2] that moves the logic to a separate helper function.

[PATCH 1/3] git-svn, perl/Git.pm: add central method for prompting passwords

[Sven Strickroth]

git-svn reads passwords from an interactive terminal or by using
GIT_ASKPASS helper tool. 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 also 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.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm            | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 perl/Git/SVN/Prompt.pm | 20 +-------------------
 2 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..72e93c7 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);
 
 
@@ -511,6 +511,49 @@ 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.
+
+Honours GIT_ASKPASS environment variable for querying
+the user. If no GIT_ASKPASS variable is set or an error occoured,
+the terminal is tried as a fallback.
+
+=cut
+
+sub prompt {
+	my ($prompt) = @_;
+	my $ret;
+	if (exists $ENV{'GIT_ASKPASS'}) {
+		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
+	}
+	if (!defined $ret) {
+		print STDERR $prompt;
+		STDERR->flush;
+		require Term::ReadKey;
+		Term::ReadKey::ReadMode('noecho');
+		$ret = '';
+		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 length $askpass;
+	my $ret;
+	open my $fh, "-|", $askpass, $prompt or return;
+	$ret = <$fh>;
+	$ret =~ s/[\015\012]//g; # strip \r\n, chomp does not work on all systems (i.e. windows) as expected
+	close ($fh);
+	return $ret;
+}
 
 =item repo_path ()
 
diff --git a/perl/Git/SVN/Prompt.pm b/perl/Git/SVN/Prompt.pm
index 3a6f8af..a2cbcc8 100644
--- a/perl/Git/SVN/Prompt.pm
+++ b/perl/Git/SVN/Prompt.pm
@@ -120,25 +120,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;
 }
 
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

[PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set

[Sven Strickroth]

If GIT_ASKPASS environment variable is not set, git-svn does not try to use
SSH_ASKPASS as git-core does. This change adds a fallback to SSH_ASKPASS.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 72e93c7..8dfca65 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -515,8 +515,8 @@ sub html_path { command_oneline('--html-path') }

 Query user C<PROMPT> and return answer from user.

-Honours GIT_ASKPASS environment variable for querying
-the user. If no GIT_ASKPASS variable is set or an error occoured,
+Honours GIT_ASKPASS and SSH_ASKPASS environment variables for querying
+the user. If no *_ASKPASS variable is set or an error occoured,
 the terminal is tried as a fallback.

 =cut
@@ -527,6 +527,9 @@ sub prompt {
 	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;
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

[PATCH 3/3] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users

[Sven Strickroth]

git-svn reads usernames and other user queries from an interactive
terminal. 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).

This change extends the Git::prompt helper, so that it can also be used
for non password queries, and makes use of it instead of using
hand-rolled prompt-response code that only works with the interactive
terminal.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 perl/Git.pm            | 28 +++++++++++++++++-----------
 perl/Git/SVN/Prompt.pm | 16 +++++++---------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 8dfca65..931047c 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -511,18 +511,19 @@ C<git --html-path>). Useful mostly only internally.
 
 sub html_path { command_oneline('--html-path') }
 
-=item prompt ( PROMPT )
+=item prompt ( PROMPT , ISPASSWORD  )
 
 Query user C<PROMPT> and return answer from user.
 
 Honours GIT_ASKPASS and SSH_ASKPASS environment variables for querying
 the user. If no *_ASKPASS variable is set or an error occoured,
 the terminal is tried as a fallback.
+If C<ISPASSWORD> is set and true, the terminal disables echo.
 
 =cut
 
 sub prompt {
-	my ($prompt) = @_;
+	my ($prompt, $isPassword) = @_;
 	my $ret;
 	if (exists $ENV{'GIT_ASKPASS'}) {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
@@ -533,16 +534,20 @@ sub prompt {
 	if (!defined $ret) {
 		print STDERR $prompt;
 		STDERR->flush;
-		require Term::ReadKey;
-		Term::ReadKey::ReadMode('noecho');
-		$ret = '';
-		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
-			last if $key =~ /[\012\015]/; # \n\r
-			$ret .= $key;
+		if (defined $isPassword && $isPassword) {
+			require Term::ReadKey;
+			Term::ReadKey::ReadMode('noecho');
+			$ret = '';
+			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;
+		} else {
+			chomp($ret = <STDIN>);
 		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
 	}
 	return $ret;
 }
@@ -550,6 +555,7 @@ sub prompt {
 sub _prompt {
 	my ($askpass, $prompt) = @_;
 	return unless length $askpass;
+	$prompt =~ s/\n/ /g;
 	my $ret;
 	open my $fh, "-|", $askpass, $prompt or return;
 	$ret = <$fh>;
diff --git a/perl/Git/SVN/Prompt.pm b/perl/Git/SVN/Prompt.pm
index a2cbcc8..74daa7a 100644
--- a/perl/Git/SVN/Prompt.pm
+++ b/perl/Git/SVN/Prompt.pm
@@ -62,16 +62,16 @@ sub ssl_server_trust {
 	                               issuer_dname fingerprint);
 	my $choice;
 prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
 	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
-	if ($choice =~ /^t$/i) {
+	$choice = lc(substr(Git::prompt("Certificate problem.\n" . $options) || 'R', 0, 1));
+	if ($choice eq 't') {
 		$cred->may_save(undef);
-	} elsif ($choice =~ /^r$/i) {
+	} elsif ($choice eq 'r') {
 		return -1;
-	} elsif ($may_save && $choice =~ /^p$/i) {
+	} elsif ($may_save && $choice eq 'p') {
 		$cred->may_save($may_save);
 	} else {
 		goto prompt;
@@ -109,9 +109,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git::prompt("Username: ");
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -120,7 +118,7 @@ sub username {
 
 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git::prompt($prompt);
+	my $password = Git::prompt($prompt, 1);
 	$password;
 }
 
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set

[Jeff King]

On Tue, Dec 18, 2012 at 01:28:47AM +0100, Sven Strickroth wrote:

> If GIT_ASKPASS environment variable is not set, git-svn does not try to use
> SSH_ASKPASS as git-core does. This change adds a fallback to SSH_ASKPASS.
> 
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

Thanks, this series looks fine to me.

I skimmed through the original thread. It looks like we got bogged down
in discussion of whether GIT_ASKPASS should fall back on error, and
whether SSH_ASKPASS should come _after_ the terminal has been tried and
failed. This version of the series brings the behavior of git-svn in
line with the rest of git, which I think is a good first step.

I don't know whether we want to take a second step and tweak the order
in both perl and C code. Since nobody has mentioned it in the interim
months, I'm assuming it's not a big deal, and people are happy enough
with the current ordering. So unless somebody feels strongly about it,
it's not worth bothering.

-Peff