* Re: Push from an SSH Terminal
2012-02-03 21:36 ` Jeff King
@ 2012-02-03 22:13 ` Jeff King
2012-02-03 22:14 ` [PATCH 1/2] prompt: clean up strbuf usage Jeff King
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2012-02-03 22:13 UTC (permalink / raw)
To: Neal Groothuis; +Cc: Feanil Patel, git
On Fri, Feb 03, 2012 at 04:36:54PM -0500, Jeff King wrote:
> > Check to see if the GIT_ASKPASS and/or SSH_ASKPASS environment variables
> > are set, and if the core.askpass config variable is set. If any of these
> > are set, unset them. Git should fall back to a simple password prompt.
>
> Hmm, yeah that is likely the problem. I was thinking git would fall back
> to asking on the terminal, but it does not. We probably should.
We should probably do this:
[1/2]: prompt: clean up strbuf usage
[2/2]: prompt: fall back to terminal if askpass fails
-Peff
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] prompt: clean up strbuf usage
2012-02-03 21:36 ` Jeff King
2012-02-03 22:13 ` Jeff King
@ 2012-02-03 22:14 ` Jeff King
2012-02-03 22:16 ` [PATCH 2/2] prompt: fall back to terminal if askpass fails Jeff King
2012-02-04 7:47 ` Push from an SSH Terminal Junio C Hamano
3 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2012-02-03 22:14 UTC (permalink / raw)
To: Neal Groothuis; +Cc: Feanil Patel, git
The do_askpass function inherited a few bad habits from the
original git_getpass. One, there's no need to strbuf_reset a
buffer which was just initialized. And two, it's a good
habit to use strbuf_detach to claim ownership of a buffer's
string (even though in this case the owning buffer goes out
of scope, so it's effectively the same thing).
Signed-off-by: Jeff King <peff@peff.net>
---
Neither is a big deal, but just some style cleanups while I was in the
area.
prompt.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/prompt.c b/prompt.c
index 72ab9de..64f817b 100644
--- a/prompt.c
+++ b/prompt.c
@@ -21,7 +21,6 @@ static char *do_askpass(const char *cmd, const char *prompt)
if (start_command(&pass))
exit(1);
- strbuf_reset(&buffer);
if (strbuf_read(&buffer, pass.out, 20) < 0)
die("failed to get '%s' from %s\n", prompt, cmd);
@@ -32,7 +31,7 @@ static char *do_askpass(const char *cmd, const char *prompt)
strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
- return buffer.buf;
+ return strbuf_detach(&buffer, NULL);
}
char *git_prompt(const char *prompt, int flags)
--
1.7.9.rc1.28.gf4be5
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/2] prompt: fall back to terminal if askpass fails
2012-02-03 21:36 ` Jeff King
2012-02-03 22:13 ` Jeff King
2012-02-03 22:14 ` [PATCH 1/2] prompt: clean up strbuf usage Jeff King
@ 2012-02-03 22:16 ` Jeff King
2012-02-04 7:47 ` Push from an SSH Terminal Junio C Hamano
3 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2012-02-03 22:16 UTC (permalink / raw)
To: Neal Groothuis; +Cc: Feanil Patel, git
The current askpass code simply dies if calling an askpass
helper fails. Worse, in some failure modes it doesn't even
print an error (if start_command fails, then it prints its
own error; if reading fails, we print an error; but if the
command exits non-zero, finish_command fails and we print
nothing!).
Let's be more kind to the user by printing an error message
when askpass doesn't work out, and then falling back to the
terminal (which also may fail, of course, but we die already
there with a nice message).
While we're at it, let's clean up the existing error
messages a bit. Now that our prompts are very long and
contain quotes and colons themselves, our error messages are
hard to read.
So the new failure modes look like:
[before, with a terminal]
$ GIT_ASKPASS=false git push
$ echo $?
128
[before, with no terminal, and we must give up]
$ setsid git push
fatal: could not read 'Password for 'https://peff@github.com': ': No such device or address
[after, with a terminal]
$ GIT_ASKPASS=false git push
error: unable to read askpass response from 'false'
Password for 'https://peff@github.com':
[after, with no terminal, and we must give up]
$ GIT_ASKPASS=false setsid git push
error: unable to read askpass response from 'false'
fatal: could not read Password for 'https://peff@github.com': No such device or address
Signed-off-by: Jeff King <peff@peff.net>
---
Arguably, the terminal-failure error message shouldn't even bother
reporting errno. I can't imagine it failing for any reason other than
ENODEV, and the message would probably be less confusing as:
fatal: could not prompt on terminal for Password for...
prompt.c | 24 +++++++++++++++++-------
1 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/prompt.c b/prompt.c
index 64f817b..d851807 100644
--- a/prompt.c
+++ b/prompt.c
@@ -9,6 +9,7 @@ static char *do_askpass(const char *cmd, const char *prompt)
struct child_process pass;
const char *args[3];
static struct strbuf buffer = STRBUF_INIT;
+ int err = 0;
args[0] = cmd;
args[1] = prompt;
@@ -19,15 +20,21 @@ static char *do_askpass(const char *cmd, const char *prompt)
pass.out = -1;
if (start_command(&pass))
- exit(1);
+ return NULL;
if (strbuf_read(&buffer, pass.out, 20) < 0)
- die("failed to get '%s' from %s\n", prompt, cmd);
+ err = 1;
close(pass.out);
if (finish_command(&pass))
- exit(1);
+ err = 1;
+
+ if (err) {
+ error("unable to read askpass response from '%s'", cmd);
+ strbuf_release(&buffer);
+ return NULL;
+ }
strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
@@ -36,7 +43,7 @@ static char *do_askpass(const char *cmd, const char *prompt)
char *git_prompt(const char *prompt, int flags)
{
- char *r;
+ char *r = NULL;
if (flags & PROMPT_ASKPASS) {
const char *askpass;
@@ -47,12 +54,15 @@ char *git_prompt(const char *prompt, int flags)
if (!askpass)
askpass = getenv("SSH_ASKPASS");
if (askpass && *askpass)
- return do_askpass(askpass, prompt);
+ r = do_askpass(askpass, prompt);
}
- r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
if (!r)
- die_errno("could not read '%s'", prompt);
+ r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
+ if (!r) {
+ /* prompts already contain ": " at the end */
+ die("could not read %s%s", prompt, strerror(errno));
+ }
return r;
}
--
1.7.9.rc1.28.gf4be5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: Push from an SSH Terminal
2012-02-03 21:36 ` Jeff King
` (2 preceding siblings ...)
2012-02-03 22:16 ` [PATCH 2/2] prompt: fall back to terminal if askpass fails Jeff King
@ 2012-02-04 7:47 ` Junio C Hamano
2012-02-04 8:09 ` Jeff King
3 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2012-02-04 7:47 UTC (permalink / raw)
To: Jeff King; +Cc: Sven Strickroth, Neal Groothuis, Feanil Patel, git
Jeff King <peff@peff.net> writes:
> On Fri, Feb 03, 2012 at 12:10:27PM -0500, Neal Groothuis wrote:
> ...
>> Check to see if the GIT_ASKPASS and/or SSH_ASKPASS environment variables
>> are set, and if the core.askpass config variable is set. If any of these
>> are set, unset them. Git should fall back to a simple password prompt.
>
> Hmm, yeah that is likely the problem. I was thinking git would fall back
> to asking on the terminal, but it does not. We probably should.
How well would it mesh with the goal of the ss/git-svn-prompt-sans-terminal
topic, which is now stalled [*1*]? I do not mean this change and the other
topic textually conflict with each other---but the philosophies of this
topic and the other one seem to conflict. Not falling back to the terminal
that is not available and failing the command outright might make more
sense.
I dunno.
[Footnote]
*1* Will the topic see any action soon? I am inclined to throw the topic
into "not even the original author is not interested" category otherwise.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Push from an SSH Terminal
2012-02-04 7:47 ` Push from an SSH Terminal Junio C Hamano
@ 2012-02-04 8:09 ` Jeff King
2012-02-04 8:16 ` Junio C Hamano
0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2012-02-04 8:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sven Strickroth, Neal Groothuis, Feanil Patel, git
On Fri, Feb 03, 2012 at 11:47:11PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Fri, Feb 03, 2012 at 12:10:27PM -0500, Neal Groothuis wrote:
> > ...
> >> Check to see if the GIT_ASKPASS and/or SSH_ASKPASS environment variables
> >> are set, and if the core.askpass config variable is set. If any of these
> >> are set, unset them. Git should fall back to a simple password prompt.
> >
> > Hmm, yeah that is likely the problem. I was thinking git would fall back
> > to asking on the terminal, but it does not. We probably should.
>
> How well would it mesh with the goal of the ss/git-svn-prompt-sans-terminal
> topic, which is now stalled [*1*]? I do not mean this change and the other
> topic textually conflict with each other---but the philosophies of this
> topic and the other one seem to conflict. Not falling back to the terminal
> that is not available and failing the command outright might make more
> sense.
I don't see a conflict in the two series. That one seems to do two
things for perl programs:
1. respect SSH_ASKPASS along with GIT_ASKPASS
2. prefer askpass over asking on the terminal
But both of those are already the case in the C code.
If you look into the original complaint mentioned in the commit
messages, though, you will see that the some GUIs will appear to hang
when the terminal is prompted (because the prompt is reading from some
location invisible to the user). So in that sense, my patches could be a
regression for those users, as outright failing is better for them.
But I would argue that the bug is not prompting on the terminal, but
rather that the terminal-prompting code does not recognize when there is
no terminal connection to the user (and AFAICT, this is a Windows
problem). Any solution that doesn't fix that is really just papering
over the problem, and hurting people[1] on sane systems.
So I'd rather see the version of getpass() in compat/mingw.c better
learn to realize when we aren't actually connected to a console.
-Peff
[1] The amount of hurt is relatively small, though. It only hurts people
who set GIT_ASKPASS but can't use it (e.g., you set it in your
.bashrc because you connect via "ssh -X", but this time you happen
to be ssh-ing from a Windows box). And you can generally fix that
outside of git (e.g., by checking $DISPLAY before setting the
variable).
So one one hand, I don't want to make a decision on behavior for
Unix users because we have to cater to Windows shortcomings. On the
other hand, while fixing the root problem is preferable, if
for whatever reason we can't reliably find out whether the user is
actually going to see and respond to the prompt on Windows, it may
be practical to just paper over the issue. On the gripping hand,
after the Sven's series, TortoiseGit users would see the hang
(instead of a failure) _only_ if their askpass command failed. Which
is also perhaps not that big a deal.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Push from an SSH Terminal
2012-02-04 8:09 ` Jeff King
@ 2012-02-04 8:16 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2012-02-04 8:16 UTC (permalink / raw)
To: Jeff King; +Cc: Sven Strickroth, Neal Groothuis, Feanil Patel, git
Jeff King <peff@peff.net> writes:
> On Fri, Feb 03, 2012 at 11:47:11PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> ...
>> How well would it mesh with the goal of the ss/git-svn-prompt-sans-terminal
>> topic, which is now stalled [*1*]? I do not mean this change and the other
>> topic textually conflict with each other---but the philosophies of this
>> topic and the other one seem to conflict.
>
> I don't see a conflict in the two series. That one seems to do two
> things for perl programs ...
That is the "[not] textually conflict" part of my message.
> If you look into the original complaint mentioned in the commit
> messages, though, you will see that the some GUIs will appear to hang
> when the terminal is prompted (because the prompt is reading from some
> location invisible to the user). So in that sense, my patches could be a
> regression for those users, as outright failing is better for them.
Yes, that is what I meant by "philosophies conflict".
> But I would argue that the bug is not prompting on the terminal, but
> rather that the terminal-prompting code does not recognize when there is
> no terminal connection to the user (and AFAICT, this is a Windows
> problem). Any solution that doesn't fix that is really just papering
> over the problem, and hurting people[1] on sane systems.
>
> So I'd rather see the version of getpass() in compat/mingw.c better
> learn to realize when we aren't actually connected to a console.
That is a sane diagnosis, I'd have to agree.
Thanks for a dose of sanity.
> [1] The amount of hurt is relatively small, though. It only hurts people
> who set GIT_ASKPASS but can't use it (e.g., you set it in your
> .bashrc because you connect via "ssh -X", but this time you happen
> to be ssh-ing from a Windows box). And you can generally fix that
> outside of git (e.g., by checking $DISPLAY before setting the
> variable).
>
> So one one hand, I don't want to make a decision on behavior for
> Unix users because we have to cater to Windows shortcomings. On the
> other hand, while fixing the root problem is preferable, if
> for whatever reason we can't reliably find out whether the user is
> actually going to see and respond to the prompt on Windows, it may
> be practical to just paper over the issue. On the gripping hand,
> after the Sven's series, TortoiseGit users would see the hang
> (instead of a failure) _only_ if their askpass command failed. Which
> is also perhaps not that big a deal.
Wow, you do have many hands ;-).
^ permalink raw reply [flat|nested] 12+ messages in thread