From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Fri, 27 Aug 2010 12:00:16 GMT
Subject: Re: [PATCH 2/2] Allow core.askpass to override SSH_ASKPASS.
Message-ID: <AANLkTik5eMLCKK7muBW9k8-0ZAxraATCs5hHVcX-Zutn@mail.gmail.com>
In-Reply-To: <201008271251.23136.k.franke@science-computing.de>

On Fri, Aug 27, 2010 at 12:51 PM, Knut Franke
<k.franke@science-computing.de> wrote:
> From: Knut Franke <k.franke@science-computing.de>
>

You don't need this line unless you're forwarding a patch for someone
else (or using a different e-mail account).

> Modify handling of the 'core.askpass' option so that it has the same effect as
> GIT_ASKPASS also if SSH_ASKPASS is set.
>

Why are you sending a two-patch series where the second patch tries to
correct the first one? Wouldn't it make more sense to squash the two?

> Signed-off-by: Knut Franke <k.franke@science-computing.de>
> ---
> I'd say this precedence order is more intuitive; and more useful.
>
>  connect.c |    4 ++++
>  git.c     |    3 ---
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index e296bfc..f97b84c 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -627,9 +627,13 @@ char *git_getpass(const char *prompt)
>        static struct strbuf buffer = STRBUF_INIT;
>
>        askpass = getenv("GIT_ASKPASS");
> +

Why?

Apart from these nits, I think the patch does the right thing. IMO it
doesn't make sense for SSH_ASKPASS to trump core.askpass.