Skip to contentSkip to conversation

Add git-credential support to git-send-email

PATCH Superseded Michal Nazarewicz 3 participants 16 messages Jun 15, 2016 Documentation

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 cover Add git-credential support to git-send-email

From: Michal Nazarewicz <[email protected]>

The third version of the patch with changes suggested by Jeff in the 4/5 patch. Also credential_read and credential_write are now public functions in case someone wants to write a helper in perl.

Michal Nazarewicz (5):
Git.pm: allow command_close_bidi_pipe to be called as method Git.pm: fix example in command_close_bidi_pipe documentation Git.pm: allow pipes to be closed prior to calling
command_close_bidi_pipe
Git.pm: add interface for git credential command
git-send-email: use git credential to obtain password

 Documentation/git-send-email.txt |   4 +-
 git-send-email.perl              |  59 ++++++++------
 perl/Git.pm                      | 166 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 198 insertions(+), 31 deletions(-)
Signature · 1 line
1.8.1.3.571.g3f8bed7
#1 lore raw
Quoted 5 lines · Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

The third version of the patch with changes suggested by Jeff in the 4/5 patch. Also credential_read and credential_write are now public functions in case someone wants to write a helper in perl.

Thanks, the changes you made look good. And I think it's a good idea to make the read/write functions public.

I have two minor comments, which I'll reply inline with. But even with those comments, I think this would be OK to merge.

-Peff

#2 lore raw
Quoted 2 lines · Jeff King

I have two minor comments, which I'll reply inline with. But even with those comments, I think this would be OK to merge.

I'll send a new patchset tomorrow with.

Signature · 4 lines
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: [email protected]>--------------ooO--(_)--Ooo--
#3 lore raw
Quoted 5 lines · Michal Nazarewicz
Quoted 2 lines · Jeff King

I have two minor comments, which I'll reply inline with. But even with those comments, I think this would be OK to merge.

I'll send a new patchset tomorrow with.

Based on our discussion, I think it would just need the patch below squashed into your 4/5 (this handles the "undef" thing, and I also fixed a few typos in the API documentation):

---

perl/Git.pm +13 -8
index 0e6fcf9..35893e6 100644
@@ -1038,7 +1038,7 @@ sub credential_read {
 	return %credential;
 }
 
-=item credential_read( FILE_HANDLE, CREDENTIAL_HASH )
+=item credential_write( FILE_HANDLE, CREDENTIAL_HASH )
 
 Writes credential key-value pairs from hash referenced by C<CREDENTIAL_HASH>
 to C<FILE_HANDLE>.  Keys and values cannot contain new-line or NUL byte
@@ -1102,7 +1102,7 @@ sub _credential_run {
 =item credential( CREDENTIAL_HASH, CODE )
 
 Executes C<git credential> for a given set of credentials and
-specified operation.  In both form C<CREDENTIAL_HASH> needs to be
+specified operation.  In both forms C<CREDENTIAL_HASH> needs to be
 a reference to a hash which stores credentials.  Under certain
 conditions the hash can change.
 
@@ -1126,11 +1126,14 @@ sub _credential_run {
 	}
 
 In the second form, C<CODE> needs to be a reference to a subroutine.
-The function will execute C<git credential fill> to fill provided
-credential hash, than call C<CODE> with C<CREDENTIAL> as the sole
-argument, and finally depending on C<CODE>'s return value execute
-C<git credential approve> (if return value yields true) or C<git
-credential reject> (otherwise).  The return value is the same as what
+The function will execute C<git credential fill> to fill the provided
+credential hash, then call C<CODE> with C<CREDENTIAL> as the sole
+argument. If C<CODE>'s return value is defined, the function will
+execute C<git credential approve> (if return value yields true) or
+C<git credential reject> (if return value is false). If the return
+value is undef, nothing at all is executed; this is useful, for
+example, if the credential could neither be verified nor rejected due
+to an unrelated network error. The return value is the same as what
 C<CODE> returned.  With this form, the usage might look as follows:
 
 	if (Git::credential {
@@ -1152,7 +1155,9 @@ sub credential {
 	if ('CODE' eq ref $op_or_code) {
 		_credential_run $credential, 'fill';
 		my $ret = $op_or_code->($credential);
-		_credential_run $credential, $ret ? 'approve' : 'reject';
+		if (defined $ret) {
+			_credential_run $credential, $ret ? 'approve' : 'reject';
+		}
 		return $ret;
 	} else {
 		_credential_run $credential, $op_or_code;
#4 lore raw
Quoted 5 lines · Jeff King

Based on our discussion, I think it would just need the patch below squashed into your 4/5 (this handles the "undef" thing, and I also fixed a few typos in the API documentation):

perl/Git.pm sub credential { · line 1163

Quoted 10 lines
perl/Git.pm +3 -1
@@ -1152,7 +1155,9 @@ sub credential {
 	if ('CODE' eq ref $op_or_code) {
 		_credential_run $credential, 'fill';
 		my $ret = $op_or_code->($credential);
-		_credential_run $credential, $ret ? 'approve' : 'reject';
+		if (defined $ret) {
+			_credential_run $credential, $ret ? 'approve' : 'reject';
+		}
 		return $ret;
 	} else {
 		_credential_run $credential, $op_or_code;

Yep, that's what I did as well. Thanks for spotting the typos, I actually changed some other wording as well (most notably CREDENTIAL_HASH -> CREDENTIAL_HASHREF), and also added some unrelated patch in the middle: <https://github.com/mina86/git/commits/master>.

Signature · 4 lines
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: [email protected]>--------------ooO--(_)--Ooo--
#5 lore raw

PATCH v3 1/5 Git.pm: allow command_close_bidi_pipe to be called as method

In reply to Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

The documentation of command_close_bidi_pipe() claims that it can be called as a method, but it does not check whether the first argument is $self or not assuming the latter. Using _maybe_self() fixes this.

---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
perl/Git.pm +1 -1
index 931047c..bbb753a 100644
@@ -430,7 +430,7 @@ have more complicated structure.
 
 sub command_close_bidi_pipe {
 	local $?;
-	my ($pid, $in, $out, $ctx) = @_;
+	my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
 	foreach my $fh ($in, $out) {
 		unless (close $fh) {
 			if ($!) {
Signature · 1 line
1.8.1.3.571.g3f8bed7.dirty
#6 lore raw

PATCH v3 2/5 Git.pm: fix example in command_close_bidi_pipe documentation

In reply to Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
perl/Git.pm +1 -1
index bbb753a..11f310a 100644
@@ -418,7 +418,7 @@ and it is the fourth value returned by C<command_bidi_pipe()>.  The call idiom
 is:
 
 	my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');
-	print "000000000\n" $out;
+	print $out "000000000\n";
 	while (<$in>) { ... }
 	$r->command_close_bidi_pipe($pid, $in, $out, $ctx);
 
Signature · 1 line
1.8.1.3.571.g3f8bed7.dirty
#7 lore raw

PATCH v3 3/5 Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe

In reply to Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

The command_close_bidi_pipe() function will insist on closing both input and output pipes returned by command_bidi_pipe(). With this change it is possible to close one of the pipes in advance and pass undef as an argument.

---
 perl/Git.pm | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
perl/Git.pm +14 -1
index 11f310a..9dded54 100644
@@ -426,13 +426,26 @@ Note that you should not rely on whatever actually is in C<CTX>;
 currently it is simply the command name but in future the context might
 have more complicated structure.
 
+C<PIPE_IN> and C<PIPE_OUT> may be C<undef> if they have been closed prior to
+calling this function.  This may be useful in a query-response type of
+commands where caller first writes a query and later reads response, eg:
+
+	my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe('cat-file --batch-check');
+	print $out "000000000\n";
+	close $out;
+	while (<$in>) { ... }
+	$r->command_close_bidi_pipe($pid, $in, undef, $ctx);
+
+This idiom may prevent potential dead locks caused by data sent to the output
+pipe not being flushed and thus not reaching the executed command.
+
 =cut
 
 sub command_close_bidi_pipe {
 	local $?;
 	my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
 	foreach my $fh ($in, $out) {
-		unless (close $fh) {
+		if (defined $fh && !close $fh) {
 			if ($!) {
 				carp "error closing pipe: $!";
 			} elsif ($? >> 8) {
Signature · 1 line
1.8.1.3.571.g3f8bed7.dirty
#8 lore raw

PATCH v3 4/5 Git.pm: add interface for git credential command

In reply to Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

Add a credential() function which is an interface to the git credential command. The code is heavily based on credential_* functions in <contrib/mw-to-git/git-remote-mediawiki>.

---
 perl/Git.pm | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 1 deletion(-)
perl/Git.pm +147 -1
index 9dded54..0e6fcf9 100644
@@ -59,7 +59,8 @@ require Exporter;
                 command_bidi_pipe command_close_bidi_pipe
                 version exec_path html_path hash_object git_cmd_try
                 remote_refs prompt
-                temp_acquire temp_release temp_reset temp_path);
+                temp_acquire temp_release temp_reset temp_path
+                credential credential_read credential_write);
 
 
 =head1 DESCRIPTION
@@ -1013,6 +1014,151 @@ sub _close_cat_blob {
 }
 
 
+=item credential_read( FILE_HANDLE )
+
+Reads credential key-value pairs from C<FILE_HANDLE>.  Reading stops at EOF or
+when an empty line is encountered.  Each line must be of the form C<key=value>
+with a non-empty key.  Function returns a hash with all read values.  Any
+white space (other then new-line character) is preserved.
+
+=cut
+
+sub credential_read {
+	my ($self, $reader) = _maybe_self(@_);
+	my %credential;
+	while (<$reader>) {
+		chomp;
+		if ($_ eq '') {
+			last;
+		} elsif (!/^([^=]+)=(.*)$/) {
+			throw Error::Simple("unable to parse git credential data:\n$_");
+		}
+		$credential{$1} = $2;
+	}
+	return %credential;
+}
+
+=item credential_read( FILE_HANDLE, CREDENTIAL_HASH )
+
+Writes credential key-value pairs from hash referenced by C<CREDENTIAL_HASH>
+to C<FILE_HANDLE>.  Keys and values cannot contain new-line or NUL byte
+characters, and key cannot contain equal sign nor be empty (if they do
+Error::Simple is thrown).  Any white space is preserved.  If value for a key
+is C<undef>, it will be skipped.
+
+If C<'url'> key exists it will be written first.  (All the other key-value
+pairs are written in sorted order but you should not depend on that).  Once
+all lines are written, an empty line is printed.
+
+=cut
+
+sub credential_write {
+	my ($self, $writer, $credential) = _maybe_self(@_);
+	my ($key, $value);
+
+	# Check if $credential is valid prior to writing anything
+	while (($key, $value) = each %$credential) {
+		if (!defined $key || !length $key) {
+			throw Error::Simple("credential key empty or undefined");
+		} elsif ($key =~ /[=\n\0]/) {
+			throw Error::Simple("credential key contains invalid characters: $key");
+		} elsif (defined $value && $value =~ /[\n\0]/) {
+			throw Error::Simple("credential value for key=$key contains invalid characters: $value");
+		}
+	}
+
+	for $key (sort {
+		# url overwrites other fields, so it must come first
+		return -1 if $a eq 'url';
+		return  1 if $b eq 'url';
+		return $a cmp $b;
+	} keys %$credential) {
+		if (defined $credential->{$key}) {
+			print $writer $key, '=', $credential->{$key}, "\n";
+		}
+	}
+	print $writer "\n";
+}
+
+sub _credential_run {
+	my ($self, $credential, $op) = _maybe_self(@_);
+
+	my ($pid, $reader, $writer, $ctx) = command_bidi_pipe('credential', $op);
+
+	credential_write $writer, $credential;
+	close $writer;
+
+	if ($op eq "fill") {
+		%$credential = credential_read $reader;
+	} elsif (<$reader>) {
+		throw Error::Simple("unexpected output from git credential $op response:\n$_\n");
+	}
+
+	command_close_bidi_pipe($pid, $reader, undef, $ctx);
+}
+
+=item credential( CREDENTIAL_HASH [, OPERATION ] )
+
+=item credential( CREDENTIAL_HASH, CODE )
+
+Executes C<git credential> for a given set of credentials and
+specified operation.  In both form C<CREDENTIAL_HASH> needs to be
+a reference to a hash which stores credentials.  Under certain
+conditions the hash can change.
+
+In the first form, C<OPERATION> can be C<'fill'> (or omitted),
+C<'approve'> or C<'reject'>, and function will execute corresponding
+C<git credential> sub-command.  In case of C<'fill'> the values stored
+in C<CREDENTIAL_HASH> will be changed to the ones returned by the
+C<git credential> command.  The usual usage would look something like:
+
+	my %cred = (
+		'protocol' => 'https',
+		'host' => 'example.com',
+		'username' => 'bob'
+	);
+	Git::credential \%cred;
+	if (try_to_authenticate($cred{'username'}, $cred{'password'})) {
+		Git::credential \%cred, 'approve';
+		... do more stuff ...
+	} else {
+		Git::credential \%cred, 'reject';
+	}
+
+In the second form, C<CODE> needs to be a reference to a subroutine.
+The function will execute C<git credential fill> to fill provided
+credential hash, than call C<CODE> with C<CREDENTIAL> as the sole
+argument, and finally depending on C<CODE>'s return value execute
+C<git credential approve> (if return value yields true) or C<git
+credential reject> (otherwise).  The return value is the same as what
+C<CODE> returned.  With this form, the usage might look as follows:
+
+	if (Git::credential {
+		'protocol' => 'https',
+		'host' => 'example.com',
+		'username' => 'bob'
+	}, sub {
+		my $cred = shift;
+		return try_to_authenticate($cred->{'username'}, $cred->{'password'});
+	}) {
+		... do more stuff ...
+	}
+
+=cut
+
+sub credential {
+	my ($self, $credential, $op_or_code) = (_maybe_self(@_), 'fill');
+
+	if ('CODE' eq ref $op_or_code) {
+		_credential_run $credential, 'fill';
+		my $ret = $op_or_code->($credential);
+		_credential_run $credential, $ret ? 'approve' : 'reject';
+		return $ret;
+	} else {
+		_credential_run $credential, $op_or_code;
+	}
+}
+
 { # %TEMP_* Lexical Context
 
 my (%TEMP_FILEMAP, %TEMP_FILES);
Signature · 1 line
1.8.1.3.571.g3f8bed7.dirty
#9 lore raw

Re: [PATCHv3 4/5] Git.pm: add interface for git credential command

Quoted 13 lines · Michal Nazarewicz

+=item credential_read( FILE_HANDLE )

perl/Git.pm +12 -0
+
+Reads credential key-value pairs from C<FILE_HANDLE>.  Reading stops at EOF or
+when an empty line is encountered.  Each line must be of the form C<key=value>
+with a non-empty key.  Function returns a hash with all read values.  Any
+white space (other then new-line character) is preserved.
+
+=cut
+
+sub credential_read {
+	my ($self, $reader) = _maybe_self(@_);
+	my %credential;
+	while (<$reader>) {

perl/Git.pm sub _close_cat_blob { · line 1039

Quoted 10 lines
perl/Git.pm +10 -0
+		chomp;
+		if ($_ eq '') {
+			last;
+		} elsif (!/^([^=]+)=(.*)$/) {
+			throw Error::Simple("unable to parse git credential data:\n$_");
+		}
+		$credential{$1} = $2;
+	}
+	return %credential;
+}

Should this return a hash reference? It seems like that is how we end up using and passing it elsewhere (since we have to anyway when passing it as a parameter).

I don't have a strong preference, and it's somewhat a matter of taste. And maybe returning the actual hash matches the rest of the module better. I admit I don't really use Git.pm much.

-Peff

#10 lore raw

Re: [PATCHv3 4/5] Git.pm: add interface for git credential command

Quoted 21 lines · Jeff King
Quoted 19 lines · Michal Nazarewicz

+=item credential_read( FILE_HANDLE )

perl/Git.pm +18 -0
+
+Reads credential key-value pairs from C<FILE_HANDLE>.  Reading stops at EOF or
+when an empty line is encountered.  Each line must be of the form C<key=value>
+with a non-empty key.  Function returns a hash with all read values.  Any
+white space (other then new-line character) is preserved.
+
+=cut
+
+sub credential_read {
+	my ($self, $reader) = _maybe_self(@_);
+	my %credential;
+	while (<$reader>) {
+		chomp;
+		if ($_ eq '') {
+			last;
+		} elsif (!/^([^=]+)=(.*)$/) {
+			throw Error::Simple("unable to parse git credential data:\n$_");
+		}
Quoted 8 lines

perl/Git.pm sub _close_cat_blob { · line 1039

Quoted 4 lines
perl/Git.pm +4 -0
+		$credential{$1} = $2;
+	}
+	return %credential;
+}

Should this return a hash reference? It seems like that is how we end up using and passing it elsewhere (since we have to anyway when passing it as a parameter).

Admittedly I mostly just copied what git-remote-mediawiki did here and don't really have any preference either way, even though with this function returning a reference the call site would have to become:

                %$credential = %{ credential_read $reader };

Another alternative would be for it to take a reference as an argument, possibly an optional one:

+sub credential_read {
+ my ($self, $reader, $ret) = (_maybe_self(@_), {});
+ my %credential;
+ while (<$reader>) {
+ # ...
+ }
+ %$ret = %credential;
+ $ret;
+}

I'd avoid modifying the hash while reading though since I think it's best if it's left intact in case of an error.

And of course, if we want to get even more crazy, credential_write could accept either reference or a hash, like so:

+sub credential_write {
+ my ($self, $writer, @rest) = _maybe_self(@_);
+ my $credential = @rest == 1 ? $rest[0] : { @rest };
+ my ($key, $value);
+ # ...
+}

Bottom line is, anything can be coded, but a question is whether it makes sense to do so. ;)

Signature · 4 lines
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: [email protected]>--------------ooO--(_)--Ooo--
#11 lore raw

Re: [PATCHv3 4/5] Git.pm: add interface for git credential command

Quoted 9 lines · Michal Nazarewicz
Quoted 3 lines

Should this return a hash reference? It seems like that is how we end up using and passing it elsewhere (since we have to anyway when passing it as a parameter).

Admittedly I mostly just copied what git-remote-mediawiki did here and don't really have any preference either way, even though with this function returning a reference the call site would have to become:

                %$credential = %{ credential_read $reader };

Oh, right, because Git::credential takes the credential as an in-out parameter rather than just returning it. Which is a bit unusual in perl, but keeps the interface reasonably simple. The alternative would be:

  $cred = Git::credential $cred, sub {
     ...
  }

which is a little less nice.

Quoted 2 lines

Another alternative would be for it to take a reference as an argument, possibly an optional one:

I think that is making things more ugly.

Quoted 2 lines

I'd avoid modifying the hash while reading though since I think it's best if it's left intact in case of an error.

Agreed.

perl/Git.pm sub _close_cat_blob { · line 1067

Quoted 9 lines

And of course, if we want to get even more crazy, credential_write could accept either reference or a hash, like so:

perl/Git.pm +6 -0
+sub credential_write {
+	my ($self, $writer, @rest) = _maybe_self(@_);
+	my $credential = @rest == 1 ? $rest[0] : { @rest };
+	my ($key, $value);
+	# ...
+}

Ugh.

Quoted 2 lines

Bottom line is, anything can be coded, but a question is whether it makes sense to do so. ;)

Yes, it is probably OK to leave it as-is, then. It is largely a matter of taste, and I will defer to your judgement on that. :)

-Peff

#12 lore raw

PATCH v3 5/5 git-send-email: use git credential to obtain password

In reply to Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

If smtp_user is provided but smtp_pass is not, instead of prompting for password, make git-send-email use git credential command instead.

---
 Documentation/git-send-email.txt |  4 +--
 git-send-email.perl              | 59 +++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 27 deletions(-)
Documentation/git-send-email.txt +2 -2
index 44a1f7c..0cffef8 100644
@@ -164,8 +164,8 @@ Sending
 Furthermore, passwords need not be specified in configuration files
 or on the command line. If a username has been specified (with
 '--smtp-user' or a 'sendemail.smtpuser'), but no password has been
-specified (with '--smtp-pass' or 'sendemail.smtppass'), then the
-user is prompted for a password while the input is masked for privacy.
+specified (with '--smtp-pass' or 'sendemail.smtppass'), then
+a password is obtained using 'git-credential'.
 
 --smtp-server=<host>::
 	If set, specifies the outgoing SMTP server to use (e.g.
git-send-email.perl +34 -25

71 lines not on this page · raw message

Signature · 1 line
1.8.1.3.571.g3f8bed7.dirty
#13 lore raw

Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password

Quoted 4 lines · Michal Nazarewicz
git-send-email.perl +4 -0
+	# TODO: Authentication may fail not because credentials were
+	# invalid but due to other reasons, in which we should not
+	# reject credentials.
+	$auth = Git::credential({

git-send-email.perl sub maildomain { · line 1076

Quoted 10 lines
git-send-email.perl +10 -0
+		'protocol' => 'smtp',
+		'host' => join(':', $smtp_server, $smtp_server_port),
+		'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'});
+	});

What do we want to do about this TODO?

I am happy to put it off until it becomes a problem, but I wonder if the Git::credential() interface is sufficient to express what we would want. It only allows two return values: true for approve, false for reject. But we would want a tri-state: approve, reject, indeterminate.

Reading the Net::SMTP code, it doesn't look like the information is even available to us (it really just passes out success or failure), so I don't think we can even make it work now. But it may be better to prepare the public Git::credential interface for it now, so we do not have to deal with breaking compatibility later.

-Peff

#14 lore raw

Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password

Quoted 15 lines · Michal Nazarewicz
Quoted 14 lines
git-send-email.perl +14 -0
+	# 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' => join(':', $smtp_server, $smtp_server_port),
+		'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'});
+	});
Quoted 6 lines · Jeff King

What do we want to do about this TODO?

I am happy to put it off until it becomes a problem, but I wonder if the Git::credential() interface is sufficient to express what we would want. It only allows two return values: true for approve, false for reject. But we would want a tri-state: approve, reject, indeterminate.

Being it tri-state is not a problem. The last can be easily represented by undef.

Quoted 5 lines

Reading the Net::SMTP code, it doesn't look like the information is even available to us (it really just passes out success or failure), so I don't think we can even make it work now. But it may be better to prepare the public Git::credential interface for it now, so we do not have to deal with breaking compatibility later.

I guess. I left it as is since git-send-email won't make use of the indeterminate values, but I can add it in this patchset as well.

Signature · 4 lines
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: [email protected]>--------------ooO--(_)--Ooo--
#15 lore raw

Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password

Quoted 7 lines · Michal Nazarewicz
Quoted 4 lines

I am happy to put it off until it becomes a problem, but I wonder if the Git::credential() interface is sufficient to express what we would want. It only allows two return values: true for approve, false for reject. But we would want a tri-state: approve, reject, indeterminate.

Being it tri-state is not a problem. The last can be easily represented by undef.

Yeah, I think undef makes sense there.

Quoted 8 lines
Quoted 5 lines

Reading the Net::SMTP code, it doesn't look like the information is even available to us (it really just passes out success or failure), so I don't think we can even make it work now. But it may be better to prepare the public Git::credential interface for it now, so we do not have to deal with breaking compatibility later.

I guess. I left it as is since git-send-email won't make use of the indeterminate values, but I can add it in this patchset as well.

Yeah, I am more worried about third-party uses outside of the Git tree, which we may then break if we change the meaning of "undef" later. Thanks.

-Peff