Re: [PATCH v2 2/3] git-core: Support retrieving passwords with GIT_ASKPASS

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2010, 07:50 UTC
Message-ID: <7vr5o84erv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4B87797D.7030905@viscovery.net>

Johannes Sixt <j.sixt@viscovery.net> writes:

Show 7 quoted lines

> BTW, to save a level of indentation, you could handle the "trivial" case
> early like this:
>
> 	if (!askpass || !*askpass)
> 		return get_pass(prompt);
>
> and continue without an 'else' branch.

That is a good advice in general.

Also, when you have a way unbalanced if ... else ... where else clause is very small, it usually is much easier to read if you invert the logic to make if part smaller.

Show 6 quoted lines

> OTOH, it may be worthwhile to set
>
> 		pass.use_shell = 1;
>
> to allow commands that are not just a single plain word. But perhaps this
> has security implications - I don't know.

How does SSH_ASKPASS gets interpreted by other programs? I think we should follow that example.

Other than that, I agree with everything you said in your review.  Thanks.