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

Johannes Sixt [off-list ref] writes:

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.

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.

Junio C Hamano schrieb:

Johannes Sixt [off-list ref] writes:

quoted

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.

openssh treats SSH_ASKPASS as a command name and uses execlp, i.e., does a
PATH search; no shell tricks are possible. Hence, we should *not* set
use_shell.

http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readpass.c?rev=1.47

Of course, we could define that GIT_ASKPASS is different from SSH_ASKPASS
in this regard, but I haven't followed the discussion to know whether this
is necessary.

-- Hannes