threads / patch / 22793

patch, 3 partsfallback SSH_ASKPASS when GIT_ASKPASS not set

Subject: [PATCH 3/3] fallback SSH_ASKPASS when GIT_ASKPASS not set

## tl;dr

5 messages between Feb 24, 2010 and Feb 25, 2010. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Frank Li· Feb 24, 2010, 15:15 UTC · lore

If GIT_ASKPASS is not set and SSH_ASKPASS set, GIT_ASKPASS will use SSH_ASKPASS. If GIT_ASKPASS set and SSH_ASKPASS is not set, SSH_ASKPASS will use GIT_ASKPASS.

Signed-off-by: Frank Li <lznuaa@gmail.com>
---
 git.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

Show changes to git.c +5 −0

diff --git a/git.c b/git.c
index 90c6daf..39e1eba 100644
--- a/git.c
+++ b/git.c
@@ -54,6 +54,11 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
 	int handled = 0;
 
+	if (getenv("GIT_ASKPASS") && !getenv("SSH_ASKPASS"))
+		setenv("SSH_ASKPASS", getenv("GIT_ASKPASS"), 1);
+	if (!getenv("GIT_ASKPASS") && getenv("SSH_ASKPASS"))
+		setenv("GIT_ASKPASS", getenv("SSH_ASKPASS"), 1);
+
 	while (*argc > 0) {
 		const char *cmd = (*argv)[0];
 		if (cmd[0] != '-')

-- 
1.7.0.85.g37fda.dirty

Erik Faye-Lund· Feb 24, 2010, 15:34 UTC · re: Frank Li · lore

Re: [PATCH 3/3] fallback SSH_ASKPASS when GIT_ASKPASS not set

On Wed, Feb 24, 2010 at 4:15 PM, Frank Li <lznuaa@gmail.com> wrote:

> If GIT_ASKPASS is not set and SSH_ASKPASS set, GIT_ASKPASS will
> use SSH_ASKPASS. If GIT_ASKPASS set and SSH_ASKPASS is not set,
> SSH_ASKPASS will use GIT_ASKPASS.
>

Since there's a maximum total size of the environment on Windows (64kib), perhaps it's better to not increase the environment-size more than necessary and instead check both variables when used? I must admit doing so gives me a kind of yuck-feeling, but at least it should also make non-builtins (like git-svn, which AFAICT is what you're fixing) work consistently even if it's not called through the git-wrapper...

Show 29 quoted lines

> Signed-off-by: Frank Li <lznuaa@gmail.com>
> ---
>  git.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/git.c b/git.c
> index 90c6daf..39e1eba 100644
> --- a/git.c
> +++ b/git.c
> @@ -54,6 +54,11 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  {
>        int handled = 0;
>
> +       if (getenv("GIT_ASKPASS") && !getenv("SSH_ASKPASS"))
> +               setenv("SSH_ASKPASS", getenv("GIT_ASKPASS"), 1);
> +       if (!getenv("GIT_ASKPASS") && getenv("SSH_ASKPASS"))
> +               setenv("GIT_ASKPASS", getenv("SSH_ASKPASS"), 1);
> +
>        while (*argc > 0) {
>                const char *cmd = (*argv)[0];
>                if (cmd[0] != '-')
> --
> 1.7.0.85.g37fda.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Erik "kusma" Faye-Lund

Frank Li· Feb 25, 2010, 04:17 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 3/3] fallback SSH_ASKPASS when GIT_ASKPASS not set

Show 9 quoted lines

>
> Since there's a maximum total size of the environment on Windows
> (64kib), perhaps it's better to not increase the environment-size more
> than necessary and instead check both variables when used? I must
> admit doing so gives me a kind of yuck-feeling, but at least it should
> also make non-builtins (like git-svn, which AFAICT is what you're
> fixing) work consistently even if it's not called through the
> git-wrapper...
>

Git-svn use the same way, please read first patch.

Junio C Hamano· Feb 24, 2010, 16:24 UTC · re: Frank Li · lore

Re: [PATCH 3/3] fallback SSH_ASKPASS when GIT_ASKPASS not set

Frank Li <lznuaa@gmail.com> writes:

> If GIT_ASKPASS is not set and SSH_ASKPASS set, GIT_ASKPASS will
> use SSH_ASKPASS. If GIT_ASKPASS set and SSH_ASKPASS is not set,
> SSH_ASKPASS will use GIT_ASKPASS.

If both are set and to different values, what should happen?

I think the basic idea is sound, but I am not sure if we should fallback on both ways. If both GIT_ASKPASS and SSH_ASKPASS are set, the user is probably telling us to use GIT_ASKPASS when git does something, similar to how GIT_EDITOR and EDITOR interacts.

So probably the patch to implement the fallback should be more like

	if (SSH_ASKPASS is set but not GIT_ASKPASS)
        	set GIT_ASKPASS from SSH_ASKPASS

and then the password prompter should use GIT_ASKPASS exclusively.

Of course, when we _do_ spawn ssh ourselves internally in connect.c, we may have to export the value of GIT_ASKPASS as SSH_ASKPASS at the call site to honor the user's request to use GIT_ASKPASS while in git (that is part of "use GIT_ASKPASS exclusively").

But it feels wrong if we exported SSH_ASKPASS set from GIT_ASKPASS when spawning processes in other codepaths, especially if SSH_ASKPASS is set (or unset) differently from GIT_ASKPASS.

Shawn is CC'ed as git-gui currently honors only SSH_ASKPASS and this patch changes the behaviour slightly.

Frank Li· Feb 25, 2010, 01:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] fallback SSH_ASKPASS when GIT_ASKPASS not set

Show 5 quoted lines

> So probably the patch to implement the fallback should be more like
>
>        if (SSH_ASKPASS is set but not GIT_ASKPASS)
>                set GIT_ASKPASS from SSH_ASKPASS
>

do you means just keep these?

+       if (!getenv("GIT_ASKPASS") && getenv("SSH_ASKPASS"))
+               setenv("GIT_ASKPASS", getenv("SSH_ASKPASS"), 1);

best regards Frank Li