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

[Jakub Narebski <jnareb@gmail.com>]

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