* [PATCH] honour GIT_ASKPASS for querying username in git-svn @ 2011-11-17 15:15 Sven Strickroth 2011-11-18 11:36 ` Erik Faye-Lund 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-11-17 15:15 UTC (permalink / raw) To: git, gitster >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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-11-17 15:15 [PATCH] honour GIT_ASKPASS for querying username in git-svn Sven Strickroth @ 2011-11-18 11:36 ` Erik Faye-Lund 2011-11-18 13:30 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Erik Faye-Lund @ 2011-11-18 11:36 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, gitster 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? ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-11-18 11:36 ` Erik Faye-Lund @ 2011-11-18 13:30 ` Sven Strickroth 2011-11-18 14:19 ` Erik Faye-Lund 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-11-18 13:30 UTC (permalink / raw) To: kusmabite; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-11-18 13:30 ` Sven Strickroth @ 2011-11-18 14:19 ` Erik Faye-Lund 2011-11-26 11:33 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Erik Faye-Lund @ 2011-11-18 14:19 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, gitster 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 ;) ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-11-18 14:19 ` Erik Faye-Lund @ 2011-11-26 11:33 ` Sven Strickroth 2011-11-30 6:44 ` Jeff King 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-11-26 11:33 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-11-26 11:33 ` Sven Strickroth @ 2011-11-30 6:44 ` Jeff King 2011-12-26 23:49 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Jeff King @ 2011-11-30 6:44 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-11-30 6:44 ` Jeff King @ 2011-12-26 23:49 ` Sven Strickroth 2011-12-27 14:33 ` Jakub Narebski 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-26 23:49 UTC (permalink / raw) To: git; +Cc: Jeff King, gitster 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-12-26 23:49 ` Sven Strickroth @ 2011-12-27 14:33 ` Jakub Narebski 2011-12-27 14:39 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Jakub Narebski @ 2011-12-27 14:33 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-12-27 14:33 ` Jakub Narebski @ 2011-12-27 14:39 ` Sven Strickroth 2011-12-27 16:00 ` Jakub Narebski 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth 0 siblings, 2 replies; 82+ messages in thread From: Sven Strickroth @ 2011-12-27 14:39 UTC (permalink / raw) To: Jakub Narebski; +Cc: git, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn 2011-12-27 14:39 ` Sven Strickroth @ 2011-12-27 16:00 ` Jakub Narebski 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth 1 sibling, 0 replies; 82+ messages in thread From: Jakub Narebski @ 2011-12-27 16:00 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 0/5] honour *_ASKPASS for querying user in git-svn 2011-12-27 14:39 ` Sven Strickroth 2011-12-27 16:00 ` Jakub Narebski @ 2011-12-27 16:01 ` Sven Strickroth 2011-12-27 16:05 ` [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS Sven Strickroth ` (4 more replies) 1 sibling, 5 replies; 82+ messages in thread From: Sven Strickroth @ 2011-12-27 16:01 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Jeff King, gitster 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth @ 2011-12-27 16:05 ` Sven Strickroth 2011-12-27 20:47 ` Junio C Hamano 2011-12-27 16:06 ` [PATCH 2/5] switch to central prompt method Sven Strickroth ` (3 subsequent siblings) 4 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-27 16:05 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Jeff King, gitster 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS 2011-12-27 16:05 ` [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS Sven Strickroth @ 2011-12-27 20:47 ` Junio C Hamano 2011-12-27 23:12 ` Thomas Adam 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2011-12-27 20:47 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS 2011-12-27 20:47 ` Junio C Hamano @ 2011-12-27 23:12 ` Thomas Adam 2011-12-27 23:35 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Thomas Adam @ 2011-12-27 23:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sven Strickroth, git, Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS 2011-12-27 23:12 ` Thomas Adam @ 2011-12-27 23:35 ` Junio C Hamano 0 siblings, 0 replies; 82+ messages in thread From: Junio C Hamano @ 2011-12-27 23:35 UTC (permalink / raw) To: Thomas Adam; +Cc: Sven Strickroth, git, Jakub Narebski, Jeff King 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? ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 2/5] switch to central prompt method 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth 2011-12-27 16:05 ` [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS Sven Strickroth @ 2011-12-27 16:06 ` Sven Strickroth 2011-12-27 20:47 ` Junio C Hamano 2011-12-27 16:07 ` [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates Sven Strickroth ` (2 subsequent siblings) 4 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-27 16:06 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Jeff King, gitster 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 2/5] switch to central prompt method 2011-12-27 16:06 ` [PATCH 2/5] switch to central prompt method Sven Strickroth @ 2011-12-27 20:47 ` Junio C Hamano 0 siblings, 0 replies; 82+ messages in thread From: Junio C Hamano @ 2011-12-27 20:47 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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; ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth 2011-12-27 16:05 ` [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS Sven Strickroth 2011-12-27 16:06 ` [PATCH 2/5] switch to central prompt method Sven Strickroth @ 2011-12-27 16:07 ` Sven Strickroth 2011-12-27 20:56 ` Junio C Hamano 2011-12-27 16:07 ` [PATCH 4/5] ignore empty *_ASKPASS variables Sven Strickroth 2011-12-27 16:07 ` [PATCH 5/5] make askpass_prompt a global prompt method for asking users Sven Strickroth 4 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-27 16:07 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Jeff King, gitster 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates 2011-12-27 16:07 ` [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates Sven Strickroth @ 2011-12-27 20:56 ` Junio C Hamano 0 siblings, 0 replies; 82+ messages in thread From: Junio C Hamano @ 2011-12-27 20:56 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 4/5] ignore empty *_ASKPASS variables 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth ` (2 preceding siblings ...) 2011-12-27 16:07 ` [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates Sven Strickroth @ 2011-12-27 16:07 ` Sven Strickroth 2011-12-27 21:00 ` Junio C Hamano 2011-12-27 16:07 ` [PATCH 5/5] make askpass_prompt a global prompt method for asking users Sven Strickroth 4 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-27 16:07 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Jeff King, gitster 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 4/5] ignore empty *_ASKPASS variables 2011-12-27 16:07 ` [PATCH 4/5] ignore empty *_ASKPASS variables Sven Strickroth @ 2011-12-27 21:00 ` Junio C Hamano 0 siblings, 0 replies; 82+ messages in thread From: Junio C Hamano @ 2011-12-27 21:00 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King, gitster 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>; ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 5/5] make askpass_prompt a global prompt method for asking users 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth ` (3 preceding siblings ...) 2011-12-27 16:07 ` [PATCH 4/5] ignore empty *_ASKPASS variables Sven Strickroth @ 2011-12-27 16:07 ` Sven Strickroth 2011-12-27 21:10 ` Junio C Hamano 4 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-27 16:07 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Jeff King, gitster 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 5/5] make askpass_prompt a global prompt method for asking users 2011-12-27 16:07 ` [PATCH 5/5] make askpass_prompt a global prompt method for asking users Sven Strickroth @ 2011-12-27 21:10 ` Junio C Hamano 2011-12-27 21:41 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2011-12-27 21:10 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 5/5] make askpass_prompt a global prompt method for asking users 2011-12-27 21:10 ` Junio C Hamano @ 2011-12-27 21:41 ` Junio C Hamano 2011-12-28 0:11 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth 2011-12-28 0:12 ` [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 0 siblings, 2 replies; 82+ messages in thread From: Junio C Hamano @ 2011-12-27 21:41 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2011-12-27 21:41 ` Junio C Hamano @ 2011-12-28 0:11 ` Sven Strickroth 2011-12-28 2:34 ` Junio C Hamano 2012-01-03 10:17 ` Ævar Arnfjörð Bjarmason 2011-12-28 0:12 ` [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 1 sibling, 2 replies; 82+ messages in thread From: Sven Strickroth @ 2011-12-28 0:11 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2011-12-28 0:11 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth @ 2011-12-28 2:34 ` Junio C Hamano 2011-12-28 16:17 ` Sven Strickroth 2011-12-28 18:56 ` Jakub Narebski 2012-01-03 10:17 ` Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 82+ messages in thread From: Junio C Hamano @ 2011-12-28 2:34 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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? ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2011-12-28 2:34 ` Junio C Hamano @ 2011-12-28 16:17 ` Sven Strickroth 2011-12-28 18:56 ` Jakub Narebski 1 sibling, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2011-12-28 16:17 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2011-12-28 2:34 ` Junio C Hamano 2011-12-28 16:17 ` Sven Strickroth @ 2011-12-28 18:56 ` Jakub Narebski 1 sibling, 0 replies; 82+ messages in thread From: Jakub Narebski @ 2011-12-28 18:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sven Strickroth, git, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2011-12-28 0:11 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth 2011-12-28 2:34 ` Junio C Hamano @ 2012-01-03 10:17 ` Ævar Arnfjörð Bjarmason 2012-01-03 10:25 ` Sven Strickroth 2012-01-03 22:51 ` Junio C Hamano 1 sibling, 2 replies; 82+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2012-01-03 10:17 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 10:17 ` Ævar Arnfjörð Bjarmason @ 2012-01-03 10:25 ` Sven Strickroth 2012-01-03 12:03 ` Ævar Arnfjörð Bjarmason 2012-01-03 22:51 ` Junio C Hamano 1 sibling, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-01-03 10:25 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 10:25 ` Sven Strickroth @ 2012-01-03 12:03 ` Ævar Arnfjörð Bjarmason 2012-01-03 12:06 ` Ævar Arnfjörð Bjarmason ` (2 more replies) 0 siblings, 3 replies; 82+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2012-01-03 12:03 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 12:03 ` Ævar Arnfjörð Bjarmason @ 2012-01-03 12:06 ` Ævar Arnfjörð Bjarmason 2012-01-03 13:18 ` Sven Strickroth 2012-01-03 19:42 ` Junio C Hamano 2 siblings, 0 replies; 82+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2012-01-03 12:06 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 12:03 ` Ævar Arnfjörð Bjarmason 2012-01-03 12:06 ` Ævar Arnfjörð Bjarmason @ 2012-01-03 13:18 ` Sven Strickroth 2012-01-03 19:42 ` Junio C Hamano 2 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-01-03 13:18 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 12:03 ` Ævar Arnfjörð Bjarmason 2012-01-03 12:06 ` Ævar Arnfjörð Bjarmason 2012-01-03 13:18 ` Sven Strickroth @ 2012-01-03 19:42 ` Junio C Hamano 2 siblings, 0 replies; 82+ messages in thread From: Junio C Hamano @ 2012-01-03 19:42 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Sven Strickroth, git, Jakub Narebski, Jeff King Æ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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 10:17 ` Ævar Arnfjörð Bjarmason 2012-01-03 10:25 ` Sven Strickroth @ 2012-01-03 22:51 ` Junio C Hamano 2012-01-03 23:27 ` Sven Strickroth 1 sibling, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-01-03 22:51 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Sven Strickroth, git, Jakub Narebski, Jeff King Æ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. ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 22:51 ` Junio C Hamano @ 2012-01-03 23:27 ` Sven Strickroth 2012-01-04 0:10 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-01-03 23:27 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, git, Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-03 23:27 ` Sven Strickroth @ 2012-01-04 0:10 ` Junio C Hamano 2012-01-04 7:55 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-01-04 0:10 UTC (permalink / raw) To: Sven Strickroth Cc: Ævar Arnfjörð Bjarmason, git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 0:10 ` Junio C Hamano @ 2012-01-04 7:55 ` Sven Strickroth 2012-01-04 8:31 ` Sven Strickroth 2012-01-04 18:58 ` Junio C Hamano 0 siblings, 2 replies; 82+ messages in thread From: Sven Strickroth @ 2012-01-04 7:55 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 7:55 ` Sven Strickroth @ 2012-01-04 8:31 ` Sven Strickroth 2012-01-04 13:34 ` Jeff King 2012-01-04 19:08 ` Junio C Hamano 2012-01-04 18:58 ` Junio C Hamano 1 sibling, 2 replies; 82+ messages in thread From: Sven Strickroth @ 2012-01-04 8:31 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 8:31 ` Sven Strickroth @ 2012-01-04 13:34 ` Jeff King 2012-01-04 14:13 ` Sven Strickroth 2012-01-04 19:08 ` Junio C Hamano 1 sibling, 1 reply; 82+ messages in thread From: Jeff King @ 2012-01-04 13:34 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 13:34 ` Jeff King @ 2012-01-04 14:13 ` Sven Strickroth 0 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-01-04 14:13 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 8:31 ` Sven Strickroth 2012-01-04 13:34 ` Jeff King @ 2012-01-04 19:08 ` Junio C Hamano 2012-01-07 4:27 ` Sven Strickroth 1 sibling, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-01-04 19:08 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski 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); } ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 19:08 ` Junio C Hamano @ 2012-01-07 4:27 ` Sven Strickroth 0 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-01-07 4:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 7:55 ` Sven Strickroth 2012-01-04 8:31 ` Sven Strickroth @ 2012-01-04 18:58 ` Junio C Hamano 2012-01-04 19:20 ` Sven Strickroth 1 sibling, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-01-04 18:58 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-01-04 18:58 ` Junio C Hamano @ 2012-01-04 19:20 ` Sven Strickroth 0 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-01-04 19:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-27 21:41 ` Junio C Hamano 2011-12-28 0:11 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth @ 2011-12-28 0:12 ` Sven Strickroth 2011-12-28 2:41 ` Junio C Hamano 1 sibling, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-28 0:12 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 0:12 ` [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth @ 2011-12-28 2:41 ` Junio C Hamano 2011-12-28 10:41 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2011-12-28 2:41 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 2:41 ` Junio C Hamano @ 2011-12-28 10:41 ` Sven Strickroth 2011-12-28 21:00 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-28 10:41 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 10:41 ` Sven Strickroth @ 2011-12-28 21:00 ` Junio C Hamano 2011-12-28 21:38 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2011-12-28 21:00 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 21:00 ` Junio C Hamano @ 2011-12-28 21:38 ` Junio C Hamano 2011-12-28 21:47 ` Sven Strickroth 2012-01-01 19:45 ` Sven Strickroth 0 siblings, 2 replies; 82+ messages in thread From: Junio C Hamano @ 2011-12-28 21:38 UTC (permalink / raw) To: git; +Cc: Sven Strickroth, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 21:38 ` Junio C Hamano @ 2011-12-28 21:47 ` Sven Strickroth 2011-12-28 22:29 ` Junio C Hamano 2012-01-01 19:45 ` Sven Strickroth 1 sibling, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-28 21:47 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 21:47 ` Sven Strickroth @ 2011-12-28 22:29 ` Junio C Hamano 2011-12-30 4:40 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2011-12-28 22:29 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 22:29 ` Junio C Hamano @ 2011-12-30 4:40 ` Sven Strickroth 2011-12-30 13:54 ` Jeff King 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-30 4:40 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-30 4:40 ` Sven Strickroth @ 2011-12-30 13:54 ` Jeff King 2011-12-30 14:53 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Jeff King @ 2011-12-30 13:54 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-30 13:54 ` Jeff King @ 2011-12-30 14:53 ` Sven Strickroth 2012-01-01 9:11 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2011-12-30 14:53 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-30 14:53 ` Sven Strickroth @ 2012-01-01 9:11 ` Junio C Hamano 2012-01-01 19:57 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-01-01 9:11 UTC (permalink / raw) To: Sven Strickroth; +Cc: Jeff King, git, Jakub Narebski 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-01 9:11 ` Junio C Hamano @ 2012-01-01 19:57 ` Sven Strickroth 2012-01-01 20:55 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-01-01 19:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-01 19:57 ` Sven Strickroth @ 2012-01-01 20:55 ` Sven Strickroth 0 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-01-01 20:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2011-12-28 21:38 ` Junio C Hamano 2011-12-28 21:47 ` Sven Strickroth @ 2012-01-01 19:45 ` Sven Strickroth 2012-01-03 18:19 ` Junio C Hamano 1 sibling, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-01-01 19:45 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-01 19:45 ` Sven Strickroth @ 2012-01-03 18:19 ` Junio C Hamano 2012-01-03 18:40 ` Jeff King 2012-01-03 23:24 ` Sven Strickroth 0 siblings, 2 replies; 82+ messages in thread From: Junio C Hamano @ 2012-01-03 18:19 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-03 18:19 ` Junio C Hamano @ 2012-01-03 18:40 ` Jeff King 2012-02-12 16:02 ` Sven Strickroth 2012-01-03 23:24 ` Sven Strickroth 1 sibling, 1 reply; 82+ messages in thread From: Jeff King @ 2012-01-03 18:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sven Strickroth, git, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-03 18:40 ` Jeff King @ 2012-02-12 16:02 ` Sven Strickroth 2012-02-12 16:11 ` Jakub Narebski 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-02-12 16:02 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-02-12 16:02 ` Sven Strickroth @ 2012-02-12 16:11 ` Jakub Narebski 2012-02-12 16:26 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Jakub Narebski @ 2012-02-12 16:11 UTC (permalink / raw) To: Sven Strickroth; +Cc: Jeff King, Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-02-12 16:11 ` Jakub Narebski @ 2012-02-12 16:26 ` Sven Strickroth 2012-02-14 22:20 ` Jeff King 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-02-12 16:26 UTC (permalink / raw) To: Jakub Narebski; +Cc: Jeff King, Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-02-12 16:26 ` Sven Strickroth @ 2012-02-14 22:20 ` Jeff King 2012-02-14 22:35 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Jeff King @ 2012-02-14 22:20 UTC (permalink / raw) To: Sven Strickroth; +Cc: Jakub Narebski, Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-02-14 22:20 ` Jeff King @ 2012-02-14 22:35 ` Junio C Hamano 2012-02-14 22:47 ` Jeff King 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-02-14 22:35 UTC (permalink / raw) To: Jeff King; +Cc: Sven Strickroth, Jakub Narebski, git 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-02-14 22:35 ` Junio C Hamano @ 2012-02-14 22:47 ` Jeff King 0 siblings, 0 replies; 82+ messages in thread From: Jeff King @ 2012-02-14 22:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sven Strickroth, Jakub Narebski, git 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-03 18:19 ` Junio C Hamano 2012-01-03 18:40 ` Jeff King @ 2012-01-03 23:24 ` Sven Strickroth 2012-01-04 0:12 ` Junio C Hamano 1 sibling, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-01-03 23:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-03 23:24 ` Sven Strickroth @ 2012-01-04 0:12 ` Junio C Hamano 2012-10-06 15:18 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-01-04 0:12 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-01-04 0:12 ` Junio C Hamano @ 2012-10-06 15:18 ` Sven Strickroth 2012-10-06 18:28 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-10-06 15:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-10-06 15:18 ` Sven Strickroth @ 2012-10-06 18:28 ` Junio C Hamano 2012-11-11 16:40 ` [PATCH 0/2] second try Sven Strickroth ` (2 more replies) 0 siblings, 3 replies; 82+ messages in thread From: Junio C Hamano @ 2012-10-06 18:28 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 0/2] second try 2012-10-06 18:28 ` Junio C Hamano @ 2012-11-11 16:40 ` Sven Strickroth 2012-11-24 19:07 ` Sven Strickroth 2012-11-11 16:40 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth 2012-11-11 16:41 ` [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 2 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-11-11 16:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 0/2] second try 2012-11-11 16:40 ` [PATCH 0/2] second try Sven Strickroth @ 2012-11-24 19:07 ` Sven Strickroth 2012-11-26 4:50 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-11-24 19:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 0/2] second try 2012-11-24 19:07 ` Sven Strickroth @ 2012-11-26 4:50 ` Junio C Hamano 2012-12-17 15:54 ` Sven Strickroth 0 siblings, 1 reply; 82+ messages in thread From: Junio C Hamano @ 2012-11-26 4:50 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski, Eric Wong 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 0/2] second try 2012-11-26 4:50 ` Junio C Hamano @ 2012-12-17 15:54 ` Sven Strickroth 2012-12-17 20:08 ` Junio C Hamano 0 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-12-17 15:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski, Eric Wong 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 0/2] second try 2012-12-17 15:54 ` Sven Strickroth @ 2012-12-17 20:08 ` Junio C Hamano 2012-12-18 0:28 ` [PATCH 1/3] git-svn, perl/Git.pm: add central method for prompting passwords Sven Strickroth ` (2 more replies) 0 siblings, 3 replies; 82+ messages in thread From: Junio C Hamano @ 2012-12-17 20:08 UTC (permalink / raw) To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski, Eric Wong 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. ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 1/3] git-svn, perl/Git.pm: add central method for prompting passwords 2012-12-17 20:08 ` Junio C Hamano @ 2012-12-18 0:28 ` Sven Strickroth 2012-12-18 0:28 ` [PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set Sven Strickroth 2012-12-18 0:28 ` [PATCH 3/3] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 2 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-12-18 0:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski, Eric Wong 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set 2012-12-17 20:08 ` Junio C Hamano 2012-12-18 0:28 ` [PATCH 1/3] git-svn, perl/Git.pm: add central method for prompting passwords Sven Strickroth @ 2012-12-18 0:28 ` Sven Strickroth 2012-12-18 0:57 ` Jeff King 2012-12-18 0:28 ` [PATCH 3/3] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 2 siblings, 1 reply; 82+ messages in thread From: Sven Strickroth @ 2012-12-18 0:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski, Eric Wong 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set 2012-12-18 0:28 ` [PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set Sven Strickroth @ 2012-12-18 0:57 ` Jeff King 0 siblings, 0 replies; 82+ messages in thread From: Jeff King @ 2012-12-18 0:57 UTC (permalink / raw) To: Sven Strickroth; +Cc: Junio C Hamano, git, Jakub Narebski, Eric Wong 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 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 3/3] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-12-17 20:08 ` Junio C Hamano 2012-12-18 0:28 ` [PATCH 1/3] git-svn, perl/Git.pm: add central method for prompting passwords Sven Strickroth 2012-12-18 0:28 ` [PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set Sven Strickroth @ 2012-12-18 0:28 ` Sven Strickroth 2 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-12-18 0:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski, Eric Wong 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS 2012-10-06 18:28 ` Junio C Hamano 2012-11-11 16:40 ` [PATCH 0/2] second try Sven Strickroth @ 2012-11-11 16:40 ` Sven Strickroth 2012-11-11 16:41 ` [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 2 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-11-11 16:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users 2012-10-06 18:28 ` Junio C Hamano 2012-11-11 16:40 ` [PATCH 0/2] second try Sven Strickroth 2012-11-11 16:40 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth @ 2012-11-11 16:41 ` Sven Strickroth 2 siblings, 0 replies; 82+ messages in thread From: Sven Strickroth @ 2012-11-11 16:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 82+ messages in thread
end of thread, other threads:[~2012-12-18 0:59 UTC | newest] Thread overview: 82+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-11-17 15:15 [PATCH] honour GIT_ASKPASS for querying username in git-svn Sven Strickroth 2011-11-18 11:36 ` Erik Faye-Lund 2011-11-18 13:30 ` Sven Strickroth 2011-11-18 14:19 ` Erik Faye-Lund 2011-11-26 11:33 ` Sven Strickroth 2011-11-30 6:44 ` Jeff King 2011-12-26 23:49 ` Sven Strickroth 2011-12-27 14:33 ` Jakub Narebski 2011-12-27 14:39 ` Sven Strickroth 2011-12-27 16:00 ` Jakub Narebski 2011-12-27 16:01 ` [PATCH 0/5] honour *_ASKPASS for querying user " Sven Strickroth 2011-12-27 16:05 ` [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS Sven Strickroth 2011-12-27 20:47 ` Junio C Hamano 2011-12-27 23:12 ` Thomas Adam 2011-12-27 23:35 ` Junio C Hamano 2011-12-27 16:06 ` [PATCH 2/5] switch to central prompt method Sven Strickroth 2011-12-27 20:47 ` Junio C Hamano 2011-12-27 16:07 ` [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates Sven Strickroth 2011-12-27 20:56 ` Junio C Hamano 2011-12-27 16:07 ` [PATCH 4/5] ignore empty *_ASKPASS variables Sven Strickroth 2011-12-27 21:00 ` Junio C Hamano 2011-12-27 16:07 ` [PATCH 5/5] make askpass_prompt a global prompt method for asking users Sven Strickroth 2011-12-27 21:10 ` Junio C Hamano 2011-12-27 21:41 ` Junio C Hamano 2011-12-28 0:11 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth 2011-12-28 2:34 ` Junio C Hamano 2011-12-28 16:17 ` Sven Strickroth 2011-12-28 18:56 ` Jakub Narebski 2012-01-03 10:17 ` Ævar Arnfjörð Bjarmason 2012-01-03 10:25 ` Sven Strickroth 2012-01-03 12:03 ` Ævar Arnfjörð Bjarmason 2012-01-03 12:06 ` Ævar Arnfjörð Bjarmason 2012-01-03 13:18 ` Sven Strickroth 2012-01-03 19:42 ` Junio C Hamano 2012-01-03 22:51 ` Junio C Hamano 2012-01-03 23:27 ` Sven Strickroth 2012-01-04 0:10 ` Junio C Hamano 2012-01-04 7:55 ` Sven Strickroth 2012-01-04 8:31 ` Sven Strickroth 2012-01-04 13:34 ` Jeff King 2012-01-04 14:13 ` Sven Strickroth 2012-01-04 19:08 ` Junio C Hamano 2012-01-07 4:27 ` Sven Strickroth 2012-01-04 18:58 ` Junio C Hamano 2012-01-04 19:20 ` Sven Strickroth 2011-12-28 0:12 ` [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 2011-12-28 2:41 ` Junio C Hamano 2011-12-28 10:41 ` Sven Strickroth 2011-12-28 21:00 ` Junio C Hamano 2011-12-28 21:38 ` Junio C Hamano 2011-12-28 21:47 ` Sven Strickroth 2011-12-28 22:29 ` Junio C Hamano 2011-12-30 4:40 ` Sven Strickroth 2011-12-30 13:54 ` Jeff King 2011-12-30 14:53 ` Sven Strickroth 2012-01-01 9:11 ` Junio C Hamano 2012-01-01 19:57 ` Sven Strickroth 2012-01-01 20:55 ` Sven Strickroth 2012-01-01 19:45 ` Sven Strickroth 2012-01-03 18:19 ` Junio C Hamano 2012-01-03 18:40 ` Jeff King 2012-02-12 16:02 ` Sven Strickroth 2012-02-12 16:11 ` Jakub Narebski 2012-02-12 16:26 ` Sven Strickroth 2012-02-14 22:20 ` Jeff King 2012-02-14 22:35 ` Junio C Hamano 2012-02-14 22:47 ` Jeff King 2012-01-03 23:24 ` Sven Strickroth 2012-01-04 0:12 ` Junio C Hamano 2012-10-06 15:18 ` Sven Strickroth 2012-10-06 18:28 ` Junio C Hamano 2012-11-11 16:40 ` [PATCH 0/2] second try Sven Strickroth 2012-11-24 19:07 ` Sven Strickroth 2012-11-26 4:50 ` Junio C Hamano 2012-12-17 15:54 ` Sven Strickroth 2012-12-17 20:08 ` Junio C Hamano 2012-12-18 0:28 ` [PATCH 1/3] git-svn, perl/Git.pm: add central method for prompting passwords Sven Strickroth 2012-12-18 0:28 ` [PATCH 2/3] perl/Git.pm: Honor SSH_ASKPASS as fallback if GIT_ASKPASS is not set Sven Strickroth 2012-12-18 0:57 ` Jeff King 2012-12-18 0:28 ` [PATCH 3/3] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth 2012-11-11 16:40 ` [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS Sven Strickroth 2012-11-11 16:41 ` [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users Sven Strickroth
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).