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

[Junio C Hamano <gitster@pobox.com>]

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.