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

From: Jakub Narebski <[email-masked]>
Date: Tue, 27 Dec 2011 06:33:51 -0800 (PST)

Sven Strickroth <[email-masked]> 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-masked]>
> Date: Tue, 27 Dec 2011 00:34:09 +0100
> Subject: [PATCH 2/4] switch to central prompt method
> 
> Signed-off-by: Sven Strickroth <[email-masked]>

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

See Documentation/SubmittingPatches

[...]
-- 
Jakub Narebski