Disabling credential helper?

Disabling credential helper?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

At $DAYJOB, we have a Git server[0] that supports the smart HTTP
protocol.  That server can return a 401 if the repository is private or
doesn't exist.

We have several scripts, some of which run interactively, some not, that
we simply want to fail if git fetch gets a non-2xx code.  Unfortunately,
git is very insistent about trying to use the default credential helper
to prompt for a username and password in this case, even opening
/dev/tty.

We've used GIT_ASKPASS=/bin/echo, which seems to solve the problem,
although it's ugly and I'm concerned it might break in the future.  Is
there a better way to do this?  I didn't see one in the documentation or
code when I looked.

[0] An Atlassian Stash instance.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Disabling credential helper?

[Jonathan Nieder]

(+peff)
Hi,

brian m. carlson wrote:

> We've used GIT_ASKPASS=/bin/echo, which seems to solve the problem,
> although it's ugly and I'm concerned it might break in the future.  Is
> there a better way to do this?

That's a good question.  Before falling back to the askpass based
prompt, Git tries each credential helper matching the URL in turn, and
there doesn't seem to be an option to override that behavior and disable
credential helpers.

As long as you have no credential helpers configured, your GIT_ASKPASS
based approach should work fine.

But once you have helpers configured, you're potentially in trouble.
I'm wondering if we ought to provide an --no-credential-helpers option
to help with this.  (Or to go further and provide a way to unset
configuration items --- e.g., '-c "credential.*=unset"'.)

Thoughts?
Jonathan

Re: Disabling credential helper?

[Jeff King]

On Tue, Dec 02, 2014 at 04:59:53PM -0800, Jonathan Nieder wrote:

> brian m. carlson wrote:
> 
> > We've used GIT_ASKPASS=/bin/echo, which seems to solve the problem,
> > although it's ugly and I'm concerned it might break in the future.  Is
> > there a better way to do this?
> 
> That's a good question.  Before falling back to the askpass based
> prompt, Git tries each credential helper matching the URL in turn, and
> there doesn't seem to be an option to override that behavior and disable
> credential helpers.

I think this has nothing at all to do with credential helpers. Git tries
the helpers, of which there are none, and falls back to askpass and
prompting on the terminal. There is no way to design a helper to say "I
tried and failed; do not bother prompting on the terminal". Git only
sees that no helper provided an answer and uses its internal methods.

I did at one point consider making the terminal prompt a credential
helper (since it is, after all, no different from git's perspective;
it's just a mechanism for "somehow" coming up with a username/password
pair).  But people generally thought that was unnecessarily complicated
(and it certainly is for the common cases).

> As long as you have no credential helpers configured, your GIT_ASKPASS
> based approach should work fine.

Yeah, it's fine (as is GIT_ASKPASS=true). You could also provide a
credential helper that gives you an empty username and password. But in
both cases, I think that git will then feed the empty password to the
server again, resulting in an extra useless round-trip. You probably
instead want to say "stop now, git, there is nothing else to be done".

We could teach the credential-helper code to do that (e.g., a helper
returns "stop=true" and we respect that). But I think you can do it
reasonably well today by making the input process fail. Sadly setting
GIT_ASKPASS to "false" just makes git complain and then try harder[1].
But you can dissociate git from the terminal, like:

  $ setsid -w git ls-remote https://github.com/private/repo
  fatal: could not read Username for 'https://github.com': No such device or address

That might have other fallouts if you use process groups for anything. I
have no problem with either an option to disable the terminal prompting,
or teaching the credential-helper interface to allow helpers to stop
lookup, either of which would be cleaner.

-Peff

[1] Courtesy of 84d7273 (prompt: fall back to terminal if askpass fails,
2012-02-03).

Re: Disabling credential helper?

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Dec 02, 2014 at 04:59:53PM -0800, Jonathan Nieder wrote:

>> As long as you have no credential helpers configured, your GIT_ASKPASS
>> based approach should work fine.
>
> Yeah, it's fine (as is GIT_ASKPASS=true). You could also provide a
> credential helper that gives you an empty username and password. But in
> both cases, I think that git will then feed the empty password to the
> server again, resulting in an extra useless round-trip. You probably
> instead want to say "stop now, git, there is nothing else to be done".
>
> We could teach the credential-helper code to do that (e.g., a helper
> returns "stop=true" and we respect that). But I think you can do it
> reasonably well today by making the input process fail.

How can my scripts defend against a credential helper that I didn't
set up that e.g. pops up a GUI window to ask for a password?

Thanks,
Jonathan

Re: Disabling credential helper?

[Jeff King]

On Tue, Dec 02, 2014 at 05:29:50PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Tue, Dec 02, 2014 at 04:59:53PM -0800, Jonathan Nieder wrote:
> 
> >> As long as you have no credential helpers configured, your GIT_ASKPASS
> >> based approach should work fine.
> >
> > Yeah, it's fine (as is GIT_ASKPASS=true). You could also provide a
> > credential helper that gives you an empty username and password. But in
> > both cases, I think that git will then feed the empty password to the
> > server again, resulting in an extra useless round-trip. You probably
> > instead want to say "stop now, git, there is nothing else to be done".
> >
> > We could teach the credential-helper code to do that (e.g., a helper
> > returns "stop=true" and we respect that). But I think you can do it
> > reasonably well today by making the input process fail.
> 
> How can my scripts defend against a credential helper that I didn't
> set up that e.g. pops up a GUI window to ask for a password?

Maybe I am misunderstanding the original situation, but I did not think
that was the problem. I thought the situation was one where the
environment was controlled, but Git still would not do what was wanted
(if you did have such a renegade helper, setting GIT_ASKPASS certainly
would not help, as it is the fallback).

But to answer your question: you can't currently. I would be happy to
have a config syntax that means "reset this multi-value config option
list to nothing", but it does not yet exist. It would be useful for more
than just credential-helper config.

-Peff

Re: Disabling credential helper?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, it's fine (as is GIT_ASKPASS=true). You could also provide a
> credential helper that gives you an empty username and password. But in
> both cases, I think that git will then feed the empty password to the
> server again, resulting in an extra useless round-trip.
> ...
> You probably
> instead want to say "stop now, git, there is nothing else to be done".
> ...
> We could teach the credential-helper code to do that (e.g., a helper
> returns "stop=true" and we respect that). But I think you can do it
> reasonably well today by making the input process fail.

Yeah, it is roughly equivalent to the 'ASKPASS=true' approach, and
probably is a good enough solution, I would think.

Re: Disabling credential helper?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

On Tue, Dec 02, 2014 at 08:21:48PM -0500, Jeff King wrote:
> We could teach the credential-helper code to do that (e.g., a helper
> returns "stop=true" and we respect that). But I think you can do it
> reasonably well today by making the input process fail. Sadly setting
> GIT_ASKPASS to "false" just makes git complain and then try harder[1].

Yes, I did notice that.  I tried /bin/false at first, and was a bit
surprised it wasn't effective.

> But you can dissociate git from the terminal, like:
> 
>   $ setsid -w git ls-remote https://github.com/private/repo
>   fatal: could not read Username for 'https://github.com': No such device or address

I think this is a bit heavy-handed for my needs.  At work, we develop on
headless VMs, so we use SSH for pushing since we can forward the agent.
At home, I use Kerberos, so the prompt generally indicates I need to run
kinit.

In neither case do I actually want to enter a password, so the
environment variable will work fine, I think, since it sounds like it's
at least semi-supported and it works well in scripts and in
configuration files.

Also, having to patch the Perl git wrappers to use setsid would be more
inconvenience than it's worth.

> That might have other fallouts if you use process groups for anything. I
> have no problem with either an option to disable the terminal prompting,
> or teaching the credential-helper interface to allow helpers to stop
> lookup, either of which would be cleaner.

I'll probably submit a patch to disable the terminal prompting this
weekend.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Disabling credential helper?

[Jeff King]

On Tue, Dec 02, 2014 at 08:36:07PM -0500, Jeff King wrote:

> But to answer your question: you can't currently. I would be happy to
> have a config syntax that means "reset this multi-value config option
> list to nothing", but it does not yet exist. It would be useful for more
> than just credential-helper config.

This is something I've wanted for a while, so I took another peek at it.
It turns out to be rather complicated.

This is the initial patch I came up with (don't bother reading it too
closely):

diff --git a/credential.c b/credential.c
index 1886ea5..9e84575 100644
--- a/credential.c
+++ b/credential.c
@@ -43,9 +43,6 @@ static int credential_config_callback(const char *var, const char *value,
 	if (!skip_prefix(var, "credential.", &key))
 		return 0;
 
-	if (!value)
-		return config_error_nonbool(var);
-
 	dot = strrchr(key, '.');
 	if (dot) {
 		struct credential want = CREDENTIAL_INIT;
@@ -63,8 +60,13 @@ static int credential_config_callback(const char *var, const char *value,
 		key = dot + 1;
 	}
 
-	if (!strcmp(key, "helper"))
-		string_list_append(&c->helpers, value);
+	if (!strcmp(key, "helper")) {
+		if (value)
+			string_list_append(&c->helpers, value);
+		else
+			string_list_clear(&c->helpers, 0);
+	} else if (!value)
+		return config_error_nonbool(var);
 	else if (!strcmp(key, "username")) {
 		if (!c->username)
 			c->username = xstrdup(value);


It allows you to do:

  git -c credential.helper [-c credential.helper=foo] fetch ...

The first one "resets" the list to empty, and then you can further
append to the list after that. There are a bunch of shortcomings,
though:

  1. I chose the value-less boolean as a token to reset the list (since
     it is otherwise an unmeaningful error). The example above shows its
     use with "-c", but you could also do:

       [credential]
       helper
       helper = foo

     in a config file itself.  This is probably rather unintuitive.

     But whatever syntax is chosen would need to have some mechanism for
     specifying it in the config file itself, as well as in the
     command-line (and therefore in the semi-public
     GIT_CONFIG_PARAMETERS environment variable which is passed around).
     So this is at least backwards-compatible, "just works" with
     existing config code, and won't stomp on any existing working
     values.

     If we can accept stomping on an unlikely-used token, something
     like:

       git -c credential.helper=RESET fetch ...

     is more sensible (and we can argue about the exact token used).
     If we can accept new syntax and new config code, something like:

       git -c '!credential.helper' fetch ...

     is probably workable.

  2. Notice how we didn't touch the config code at all here. That's
     because it doesn't know anything about whether a config item is a
     multi-valued list, or that the string list exists. It is up to each
     individual consumer of the config callbacks to implement this
     list-clearing mechanism (and obviously we should make them all
     agree on the token used). So we'd probably want to make similar
     changes everywhere that multi-values are used (which, to be fair,
     really isn't that many, I don't think).

  3. Running `git config credential.helper` doesn't know about this
     mechanism either. You can either get the "last one wins" behavior,
     or you can use --get-all to get all instances. We'd probably have
     to teach it a new `--get-list` that recognized the magic reset
     token.

None of those problems is insurmountable. It just takes a little more
code than what I wrote above. But for credential helpers specifically,
it's a bit more challenging. Because we're _not_ constructing just a
list of just credential.helper here. We're constructing a list of all
matching credential.*.helper config keys. So in something like:

  [credential "http://example.com"]
  helper = foo
  [credential]
  helper = bar

if you run "git fetch http://example.com", you'll get both helpers, in
sequence.

What should adding:

  [credential]
  helper = RESET

do on top of that?  Intuitively, I'd say that it would only touch the
credential.helper variables. But that's not what the user probably
wants. They probably want to reset _any_ helpers that have matched. And
that's actually what the code I posted above would do. So that's good, I
guess. But I wonder if this approach is introducing more confusion than
it is worth.

Having a separate --no-respect-credential-helpers is conceptually much
simpler. But it also does not allow you to reset the list and add in a
new helper.

-Peff

[PATCH 0/2] disabling terminal prompts

[Jeff King]

On Thu, Dec 04, 2014 at 12:42:31AM +0000, brian m. carlson wrote:

> I'll probably submit a patch to disable the terminal prompting this
> weekend.

Too late. You got me thinking about it, so I wrote the following series.

  [1/2]: credential: let helpers tell us to quit
  [2/2]: prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

Technically the second one is redundant with the first. You could just
define a credential helper which bails before we get to the prompts. But
for simple cases, I think that:

  GIT_TERMINAL_PROMPT=0 git ...

is a lot more intuitive than:

  git -c credential.helper='!f() { echo quit=1; }; f' ...

and is also easier to set at the top of a non-interactive script. The
first is not redundant with the second, because it is a lot more
flexible. E.g., you can abort only after seeing that we match a specific
URL, or after running any arbitrary code you like (e.g., checking out
some characteristics of the tty and deciding it is not a good idea to
access it).

There is one thing this _doesn't_ handle, which is that of preventing
existing configured credential-helpers from running (even a helper you
define with "git -c" runs after the others). But I think that is a
separate issue.

-Peff

[PATCH 1/2] credential: let helpers tell us to quit

[Jeff King]

When we are trying to fill a credential, we loop over the
set of defined credential-helpers, then fall back to running
askpass, and then finally prompt on the terminal. Helpers
which cannot find a credential are free to tell us nothing,
but they cannot currently ask us to stop prompting.

This patch lets them provide a "quit" attribute, which asks
us to stop the process entirely (avoiding running more
helpers, as well as the askpass/terminal prompt).

This has a few possible uses:

  1. A helper which prompts the user itself (e.g., in a
     dialog) can provide a "cancel" button to the user to
     stop further prompts.

  2. Some helpers may know that prompting cannot possibly
     work. For example, if their role is to broker a ticket
     from an external auth system and that auth system
     cannot be contacted, there is no point in continuing
     (we need a ticket to authenticate, and the user cannot
     provide one by typing it in).

Signed-off-by: Jeff King <peff@peff.net>
---
+cc spearce, as I recall that you may do something similar to the second
scenario (and this would prevent annoying useless prompts when the
helper fails).

 Documentation/technical/api-credentials.txt | 5 ++++-
 credential.c                                | 5 +++++
 credential.h                                | 1 +
 t/t0300-credentials.sh                      | 9 +++++++++
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index c1b42a4..e44426d 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -248,7 +248,10 @@ FORMAT` in linkgit:git-credential[7] for a detailed specification).
 For a `get` operation, the helper should produce a list of attributes
 on stdout in the same format. A helper is free to produce a subset, or
 even no values at all if it has nothing useful to provide. Any provided
-attributes will overwrite those already known about by Git.
+attributes will overwrite those already known about by Git.  If a helper
+outputs a `quit` attribute with a value of `true` or `1`, no further
+helpers will be consulted, nor will the user be prompted (if no
+credential has been provided, the operation will then fail).
 
 For a `store` or `erase` operation, the helper's output is ignored.
 If it fails to perform the requested operation, it may complain to
diff --git a/credential.c b/credential.c
index 1886ea5..b146ad8 100644
--- a/credential.c
+++ b/credential.c
@@ -173,6 +173,8 @@ int credential_read(struct credential *c, FILE *fp)
 			c->path = xstrdup(value);
 		} else if (!strcmp(key, "url")) {
 			credential_from_url(c, value);
+		} else if (!strcmp(key, "quit")) {
+			c->quit = !!git_config_bool("quit", value);
 		}
 		/*
 		 * Ignore other lines; we don't know what they mean, but
@@ -274,6 +276,9 @@ void credential_fill(struct credential *c)
 		credential_do(c, c->helpers.items[i].string, "get");
 		if (c->username && c->password)
 			return;
+		if (c->quit)
+			die("credential helper '%s' told us to quit",
+			    c->helpers.items[i].string);
 	}
 
 	credential_getpass(c);
diff --git a/credential.h b/credential.h
index 0c3e85e..6b0cd16 100644
--- a/credential.h
+++ b/credential.h
@@ -7,6 +7,7 @@ struct credential {
 	struct string_list helpers;
 	unsigned approved:1,
 		 configured:1,
+		 quit:1,
 		 use_http_path:1;
 
 	char *username;
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 57ea5a1..d7ef44b 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -289,4 +289,13 @@ test_expect_success 'http paths can be part of context' '
 	EOF
 '
 
+test_expect_success 'helpers can abort the process' '
+	test_must_fail git \
+		-c credential.helper="!f() { echo quit=1; }; f" \
+		-c credential.helper="verbatim foo bar" \
+		credential fill >stdout &&
+	>expect &&
+	test_cmp expect stdout
+'
+
 test_done
-- 
2.2.0.390.gf60752d

[PATCH 2/2] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Jeff King]

If you run git as part of an automated system, you might
prefer git to die rather than try to issue a prompt on the
terminal (because there would be nobody to see it and
respond, and the process would hang forever).

This usually works out of the box because getpass() (and our
more featureful replacements) will fail when there is no
tty, but this does not cover all cases. For example, a batch
system run via ssh might have a tty, even when the user does
not expect it.

Let's provide an environment variable the user can set to
avoid even trying to touch the tty at all.

Signed-off-by: Jeff King <peff@peff.net>
---
I also considered adding a "git --[no-]terminal-prompt"
option, but it seemed like overkill to me. I imagine the
common use would be to just set this at the top of a script.
But it would be trivial to add if we want to.

No tests, because our lib-terminal.sh infrastructure is not
up to the challenge of fooling getpass(). Besides not
handling input at all, git will actually open /dev/tty
itself. So we would have to open a pty and then set it as
the controlling terminal for the sub-process.  It's probably
possible, but I'm not sure the portability headaches are
worth testing such a trivial feature.

 Documentation/git.txt |  4 ++++
 prompt.c              | 16 ++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index afb48d3..54f7a1c 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -906,6 +906,10 @@ for further details.
 	and read the password from its STDOUT. See also the 'core.askpass'
 	option in linkgit:git-config[1].
 
+'GIT_TERMINAL_PROMPT'::
+	If this environment variable is set to `0`, git will not prompt
+	on the terminal (e.g., when asking for HTTP authentication).
+
 'GIT_CONFIG_NOSYSTEM'::
 	Whether to skip reading settings from the system-wide
 	`$(prefix)/etc/gitconfig` file.  This environment variable can
diff --git a/prompt.c b/prompt.c
index e5b4938..8181eeb 100644
--- a/prompt.c
+++ b/prompt.c
@@ -57,11 +57,19 @@ char *git_prompt(const char *prompt, int flags)
 			r = do_askpass(askpass, prompt);
 	}
 
-	if (!r)
-		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));
+		const char *err;
+
+		if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
+			r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
+			err = strerror(errno);
+		} else {
+			err = "terminal prompts disabled";
+		}
+		if (!r) {
+			/* prompts already contain ": " at the end */
+			die("could not read %s%s", prompt, err);
+		}
 	}
 	return r;
 }
-- 
2.2.0.390.gf60752d

Re: Disabling credential helper?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   1. I chose the value-less boolean as a token to reset the list (since
>      it is otherwise an unmeaningful error). The example above shows its
>      use with "-c", but you could also do:
>
>        [credential]
>        helper
>        helper = foo
>
>      in a config file itself.  This is probably rather unintuitive.

For this one, and I suspect all the "multi-valued" ones, I think it
actually is the most sensible syntax (another possiblity is to give
an empty string, assuming that all multi-valued variables we care
about take non-empty string or numeric values), as I do not see a
useful/valid use case for wanting to define boolean multi-valued
variable.

>      If we can accept stomping on an unlikely-used token, something
>      like:
>
>        git -c credential.helper=RESET fetch ...
>
>      is more sensible (and we can argue about the exact token used).

This unfortunately is unlikely to fly well if we are shooting for a
generic mechanism that is applicable for multi-valued ones in
general (your comment 2. below is very much relevant and true).

>      If we can accept new syntax and new config code, something like:
>
>        git -c '!credential.helper' fetch ...
>
>      is probably workable.

I think I suggested exactly this syntax (and "[credential] !helper"
in the config file) when this was brought up the last time, but it
was shot down because that would make the resulting configuration
file unparsable (not just ignored) by existing versions of Git.

But perhaps it is a good thing to break existing parser when "clear
the variable settings seen so far" is used.  It would not do us very
good to allow existing implementations to ignore it and continue as
if all other entries (and special token like RESET) matter will
silently give users incorrect result.

Re: [PATCH 2/2] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/prompt.c b/prompt.c
> index e5b4938..8181eeb 100644
> --- a/prompt.c
> +++ b/prompt.c
> @@ -57,11 +57,19 @@ char *git_prompt(const char *prompt, int flags)
>  			r = do_askpass(askpass, prompt);
>  	}
>  
> -	if (!r)
> -		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));
> +		const char *err;
> +
> +		if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
> +			r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
> +			err = strerror(errno);
> +		} else {
> +			err = "terminal prompts disabled";
> +		}
> +		if (!r) {
> +			/* prompts already contain ": " at the end */
> +			die("could not read %s%s", prompt, err);
> +		}
>  	}
>  	return r;
>  }

I wish this covered a lot more than just this part from an
end-user's point of view, but this is definitely one of the most
important code paths the mechanism should cover.

Thanks, will queue.

Re: [PATCH 2/2] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Jeff King]

On Thu, Dec 04, 2014 at 10:24:09AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/prompt.c b/prompt.c
> > index e5b4938..8181eeb 100644
> > --- a/prompt.c
> > +++ b/prompt.c
> > @@ -57,11 +57,19 @@ char *git_prompt(const char *prompt, int flags)
> >  			r = do_askpass(askpass, prompt);
> >  	}
> >  
> > -	if (!r)
> > -		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));
> > +		const char *err;
> > +
> > +		if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
> > +			r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
> > +			err = strerror(errno);
> > +		} else {
> > +			err = "terminal prompts disabled";
> > +		}
> > +		if (!r) {
> > +			/* prompts already contain ": " at the end */
> > +			die("could not read %s%s", prompt, err);
> > +		}
> >  	}
> >  	return r;
> >  }
> 
> I wish this covered a lot more than just this part from an
> end-user's point of view, but this is definitely one of the most
> important code paths the mechanism should cover.

Which parts do you mean? Stuff like "git add -i"?  I agree it might be
nice to turn that off, but it is a little bit of a different beast, in
that it reads from stdin. The git_prompt code is unique in accessing
/dev/tty directly, which makes it hard to shut off.

I don't know of any other code in git that  (and it is used in
many spots due to calls into the credential_fill code).

I suspect there's similar code in git-svn that comes from the svn
library, and it might be nice to cover that, too.

Anyway, I'm happy to give other prompts the same treatment, but I think
we can wait and add them as they are noticed.

-Peff

Re: [PATCH 2/2] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 04, 2014 at 10:24:09AM -0800, Junio C Hamano wrote:
>
>> I wish this covered a lot more than just this part from an
>> end-user's point of view, but this is definitely one of the most
>> important code paths the mechanism should cover.
>
> Which parts do you mean? Stuff like "git add -i"?

No, more like "tag -s" that eventually leads to somebody prompting
for the passphrase to unlock your GPG key---and from an end user's
point of view, that somebody is Git.

Of course, from _our_ point of view, that somebody is not us.  We do
not have direct control, certainly from this codepath.

Re: [PATCH 2/2] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Jeff King]

On Thu, Dec 04, 2014 at 01:33:53PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Dec 04, 2014 at 10:24:09AM -0800, Junio C Hamano wrote:
> >
> >> I wish this covered a lot more than just this part from an
> >> end-user's point of view, but this is definitely one of the most
> >> important code paths the mechanism should cover.
> >
> > Which parts do you mean? Stuff like "git add -i"?
> 
> No, more like "tag -s" that eventually leads to somebody prompting
> for the passphrase to unlock your GPG key---and from an end user's
> point of view, that somebody is Git.

Ah, yeah, I definitely agree that GIT_TERMINAL_PROMPT should work there,
too.

> Of course, from _our_ point of view, that somebody is not us.  We do
> not have direct control, certainly from this codepath.

Right, but in theory we can provoke gpg to do what we want when we spawn
it. However, having had zero luck in convincing it to stop asking me for
a passphrase recently in another thread, I do not know what magic
command line option is required. :(

I think it would be OK to merge the git handling of GIT_TERMINAL_PROMPT
(i.e., the patch I sent), and somebody who runs into the issue with gpg
and can figure out how to tame it can scratch their own itch later. I
hate leaving things half-implemented or inconsistent, but I also don't
know how to make gpg do what we want. And doing a partial solution
seems better to me than holding the credential.c half hostage.

-Peff

Re: [PATCH 2/2] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 04, 2014 at 01:33:53PM -0800, Junio C Hamano wrote:
>
>> Of course, from _our_ point of view, that somebody is not us.  We do
>> not have direct control, certainly from this codepath.

Belated typofix - s/from /not &/.

>
> Right, but in theory we can provoke gpg to do what we want when we spawn
> it. However, having had zero luck in convincing it to stop asking me for
> a passphrase recently in another thread, I do not know what magic
> command line option is required. :(
>
> I think it would be OK to merge the git handling of GIT_TERMINAL_PROMPT
> (i.e., the patch I sent), and somebody who runs into the issue with gpg
> and can figure out how to tame it can scratch their own itch later. I
> hate leaving things half-implemented or inconsistent, but I also don't
> know how to make gpg do what we want. And doing a partial solution
> seems better to me than holding the credential.c half hostage.

Oh, no question about that.  You are making things better by
advancing one step at a time.

Queued and will be moving to 'next' shortly.