Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 28, 2011, 02:41 UTC
- Message-ID
- <7vpqf91kqo.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4EFA5F08.2060705@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
Show 11 quoted lines
> @@ -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.
Show 12 quoted lines
> @@ -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.