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

From: Thomas Adam <thomas@xteddy.org>
Date: Dec 27, 2011, 23:12 UTC
Message-ID: <CA+39Oz5J82GVyLfzWbWz20VS=Gp=8q9WsHQY33GuOKT1PyFCbQ@mail.gmail.com>
In-Reply-To: <7vwr9h68t9.fsf@alter.siamese.dyndns.org>

On 27 December 2011 20:47, Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

> 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