Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS

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

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.