Re: [[PATCH v2]] git-send-email: Added the ability to query the number of smtp password questions
sil...@port1024.net writes:
From: Silvio F silvio.fri...@gmail.com
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.
Signed-off-by: Silvio F silvio.fri...@gmail.com
---
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.
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?
Thanks.
[jc: cc'ed peff@ for thoughts on credential helper API]
Documentation/git-send-email.txt | 4
git-send-email.perl | 32 +---
2 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-send-email.txt
b/Documentation/git-send-email.txt
index f0e57a5..ac993d6 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -364,6 +364,10 @@ sendemail.confirm::
one of 'always', 'never', 'cc', 'compose', or 'auto'. See '--confirm'
in the previous section for the meaning of these values.
+sendmail.askpasswordcount::
+ Number of times the smtp password can be entered before sending mail is
+ aborted. Default is 1.
+
EXAMPLE
---
Use gmail as the smtp server
diff --git a/git-send-email.perl b/git-send-email.perl
index 3782c3b..aeb2e6d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -203,6 +203,7 @@ my ($validate, $confirm);
my (@suppress_cc);
my ($auto_8bit_encoding);
my ($compose_encoding);
+my ($askpasswordcount) = 1;
my ($debug_net_smtp) = 0;# Net::SMTP, see send_message()
@@ -237,6 +238,7 @@ my %config_settings = (
from = \$sender,
assume8bitencoding = \$auto_8bit_encoding,
composeencoding = \$compose_encoding,
+askpasswordcount = \$askpasswordcount
);
my %config_path_settings = (
@@ -360,6 +362,10 @@ sub read_config {
}
}
+ if ($askpasswordcount 1) {
+ $askpasswordcount = 1;
+ }
+
if (!defined $smtp_encryption) {
my $enc = Git::config(@repo, $prefix.smtpencryption);
if (defined $enc) {
@@ -1069,17 +1075,21 @@ sub smtp_auth_maybe {
# TODO: Authentication may fail not because credentials were
# invalid but due to other reasons, in which we should not
# reject credentials.
- $auth = Git::credential({
- 'protocol' = 'smtp',
- 'host' = smtp_host_string(),
- 'username' = $smtp_authuser,
- # if there's no password, git credential fill will
- # give us one, otherwise it'll just pass this one.
- 'password' = $smtp_authpass
- }, sub {
- my $cred = shift;
- return !!$smtp-auth($cred-{'username'}, $cred-{'password'});
- });
+ for my $i (1 .. $askpasswordcount) {
+ $auth = Git::credential({
+ 'protocol' = 'smtp',
+ 'host' = smtp_host_string(),
+ 'username' = $smtp_authuser,
+ # if there's no password, git credential fill will
+ # give us one, otherwise it'll just pass this one.
+ 'password' = $smtp_authpass
+ }, sub {
+ my $cred = shift;
+ return !!$smtp-auth($cred-{'username'},
$cred-{'password'});
+ });
+
+ last if ($auth);
+ }
return $auth;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [[PATCH v2]] git-send-email: Added the ability to query the number of smtp password questions
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [[PATCH v2]] git-send-email: Added the ability to query the number of smtp password questions
Jeff King p...@peff.net writes: 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. Yeah, it may be a sensible way forward. 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. I was wondering if it should go to Git::credential; I did not say it might go to credential_fill() API, exactly for that fill does not know if that was accepted reason. We are in full agreement, I think. 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
