[PATCH] git-send-email: Added the ability to query the number of smtp password questions

[PATCH] git-send-email: Added the ability to query the number of smtp password questions

[silvio]

From: Silvio F <silvio.fricke@gmail.com>

Signed-off-by: Silvio F <silvio.fricke@gmail.com>
---
 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..9b2eda3 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;
 }
-- 
1.8.4.2

[PATCH v2] git-send-mail: ask smtp password several times

[silvio]

Hi,

Here the second version of my patch.

With this patch we are able to enter more than one time the password for the
smtp server. Before this patch we had only one trial.

I'm a newbie in perl. Please have attention to newbie failures for perl.

To me it was not possible to implement a test for my changes. Has someone
suggestion to this?

v2:
	* rebase to master-branch (0ecd94d7d728)
	* fix some perl-styles

v1:
	* first version. patch was based on next branch

Cheers,
	Silvio

[[PATCH v2]] git-send-email: Added the ability to query the number of smtp password questions

[silvio]

From: Silvio F <silvio.fricke@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.fricke@gmail.com>
---
 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;
 }
-- 
1.8.4.2

Re: [[PATCH v2]] git-send-email: Added the ability to query the number of smtp password questions

[Junio C Hamano]

silvio@port1024.net writes:

> From: Silvio F <silvio.fricke@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.fricke@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;
>  }

Re: [[PATCH v2]] git-send-email: Added the ability to query the number of smtp password questions

[Jeff King]

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;

Re: [[PATCH v2]] git-send-email: Added the ability to query the number of smtp password questions

[Junio C Hamano]

Jeff King <peff@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.