On Tue, Nov 12, 2013 at 09:57:39AM -0800, Junio C Hamano wrote:
> > With this patch "git-send-mail" ask a configurable number of questions to
> > input the smtp password. Without this patch we have only one trial.
>
> I wonder if Git::credential (or even the underlying lower level
> credential_fill() in the helper API) should give hints to the caller
> if calling it again may yield a different result. An interactive
> prompt may allow the user to mistype the password and then a later
> call may return a correct one, but the .netrc helper will read from
> the file and will return a fixed result, so there is no use calling
> credential_fill() again. And in the latter case, you do not want to
> loop $askpasswordcount times.
It would be pretty easy to add an "interactive=true" flag to the
credential response (patch below). Credential readers are supposed to
ignore elements that they don't understand. So storage helpers which are
told "we got a password interactively" can choose to use that
information if they want, but current implementations will fall back to
ignoring it. Similarly, users of "git credential fill" can use it, but
it won't hurt existing readers.
> I also have to wonder if this logic belongs to git-send-email.
> Specifically, I wonder if we can place the looping logic in
> Git::credential, so that other users of the library can take
> advantage of it?
A very early draft of the credential code added looping, but I cut it
out (I think before it even made it to the list). I don't recall the
exact reason, but it was probably a combination of:
1. It's awkward to do at the credential layer in C, because you need
input from the calling code on whether the credential worked or
not. The perl Git::credential can take a callback, though, which
means the credential code owns the outer loop, and it would be
pretty easy to just loop on trying.
2. We already have a retry mechanism; it is called "shell history". :)
The second one is only somewhat tongue in cheek. If we were an
interactive program, retrying would be essential. But fetch and push
tend to be very easy to simply re-run, as they perform only a single
action that either works or does not. So I have not really seen anyone
make a request for the feature.
I guess "send-email" does not (always) fall under the same category
because the user may have put work into a cover letter, or filling
interactive fields. So I have no objection to adding it there, but I do
agree we might as well put it in Git::credential.
---
Here's the "interactive" patch. It needs documentation and tests, and
it would probably make sense to simplify the text bool to "0|1", just to
make life easier for other reader implementations.
diff --git a/credential.c b/credential.c
index e54753c..fccb944 100644
--- a/credential.c
+++ b/credential.c
@@ -126,6 +126,7 @@ static char *credential_ask_one(const char *what, struct credential *c,
strbuf_release(&desc);
strbuf_release(&prompt);
+ c->interactive = 1;
return xstrdup(r);
}
@@ -174,6 +175,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, "interactive")) {
+ c->interactive = git_config_bool("interactive", value);
}
/*
* Ignore other lines; we don't know what they mean, but
@@ -200,6 +203,8 @@ void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path);
credential_write_item(fp, "username", c->username);
credential_write_item(fp, "password", c->password);
+ if (c->interactive)
+ credential_write_item(fp, "interactive", "true");
}
static int run_credential_helper(struct credential *c,
diff --git a/credential.h b/credential.h
index 0c3e85e..c0e5cbc 100644
--- a/credential.h
+++ b/credential.h
@@ -7,6 +7,7 @@ struct credential {
struct string_list helpers;
unsigned approved:1,
configured:1,
+ interactive:1,
use_http_path:1;
char *username;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html