[PATCH] contrib: Honor symbolic port in git-credential-netrc.

[PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Maxim Cournoyer]

Symbolic ports were previously silently dropped, which made it
impossible to use them with git-credential-netrc. This is a supported
use case according to 'man git-send-email', for --smtp-server-port:

   [...] symbolic port names (e.g. "submission" instead of 587) are
   also accepted.
---
 contrib/credential/netrc/git-credential-netrc.perl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 9fb998ae09..ad06000b9f 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;
@@ -267,7 +267,9 @@ sub load_netrc {
 		if (!defined $nentry->{machine}) {
 			next;
 		}
-		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
+		if (defined $nentry->{port} && $nentry->{port} =~ m/^[[:alnum:]]+$/) {
+			# Port may be either an integer or a symbolic
+			# name, e.g. "smtps".
 			$num_port = $nentry->{port};
 			delete $nentry->{port};
 		}

base-commit: 9520f7d9985d8879bddd157309928fc0679c8e92
-- 
2.49.0

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Junio C Hamano]

Maxim Cournoyer <maxim@guixotic.coop> writes:

> Subject: Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

Please downcase "Honor" and drop the final full stop, per convention
(see "git shortlog --no-merges --since=2.months" for examples).

> Symbolic ports were previously silently dropped, which made it
> impossible to use them with git-credential-netrc.

Wouldn't it make sense to issue a warning message when a defined
$nentry->{port} is not unrecognized?  Wouldn't it make sense to
do so even before we add this new feature?

> This is a supported
> use case according to 'man git-send-email', for --smtp-server-port:
>
>    [...] symbolic port names (e.g. "submission" instead of 587) are
>    also accepted.
> ---

Missing sign-off?  See Documentation/SubmittingPatches

>  contrib/credential/netrc/git-credential-netrc.perl | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
> index 9fb998ae09..ad06000b9f 100755
> --- a/contrib/credential/netrc/git-credential-netrc.perl
> +++ b/contrib/credential/netrc/git-credential-netrc.perl
> @@ -1,4 +1,4 @@
> -#!/usr/bin/perl
> +#!/usr/bin/env perl

An unrelated change to introduce the use of /usr/bin/env in this
patch is unwelcome.  Besides, this is a source that is processed
by the nearby Makefile, which uses the toplevel genererate-perl.sh
to turn the "#!.../perl" line to name the correct $PERL_PATH before
the build product gets installed, so I suspect that this change is
totally unnecessary.

> @@ -267,7 +267,9 @@ sub load_netrc {
>  		if (!defined $nentry->{machine}) {
>  			next;
>  		}
> -		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
> +		if (defined $nentry->{port} && $nentry->{port} =~ m/^[[:alnum:]]+$/) {
> +			# Port may be either an integer or a symbolic
> +			# name, e.g. "smtps".

Do we know symbolic port names are always limited to alnums?  Or on
some systems some byte values in the fringe, like "_" or "-", are
also allowed?

Overall, I think it is a worthwhile to address this issue.  I just
do not want a patch that says "alnum seems to be good enough to
cover the ports I happen to care about" (or a patch does not even
say how it came to the conclusion to use :alnum:)---we should do
better and explain this change with something like "from THIS
SOURCE, the names users may want to use come from this set of names,
where THESE are defined as valid characters, hence we allow not just
digits (for numeric ports), but allowing also :alnum: is sufficient
to cover these symbolic names".

Thanks.


>  			$num_port = $nentry->{port};
>  			delete $nentry->{port};
>  		}
>
> base-commit: 9520f7d9985d8879bddd157309928fc0679c8e92

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Andreas Schwab]

On Jun 20 2025, Junio C Hamano wrote:

> Do we know symbolic port names are always limited to alnums?  Or on
> some systems some byte values in the fringe, like "_" or "-", are
> also allowed?

Valid service names are documented in RFC6335.  Specifically it allows
hyphens, but not underscores.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Jun 20 2025, Junio C Hamano wrote:
>
>> Do we know symbolic port names are always limited to alnums?  Or on
>> some systems some byte values in the fringe, like "_" or "-", are
>> also allowed?
>
> Valid service names are documented in RFC6335.  Specifically it allows
> hyphens, but not underscores.

Yes, something like that is what I wanted the original contributor
to write in the proposed log message to explain how the loosened
rule for accepted "port" was chosen (and relize that alnum is not
sufficient).

Thanks.

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[brian m. carlson]

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

On 2025-06-20 at 04:12:39, Maxim Cournoyer wrote:
> Symbolic ports were previously silently dropped, which made it
> impossible to use them with git-credential-netrc. This is a supported
> use case according to 'man git-send-email', for --smtp-server-port:
> 
>    [...] symbolic port names (e.g. "submission" instead of 587) are
>    also accepted.

Does this work with credential managers in general (that is, in
non-email contexts, such as HTTP)?  Also, do credential managers in
general properly find credentials when they're stored in one form and
looked up in another?  If so, is that still true when the lookup is in
the URL form (e.g., `smtp://mail.example.com:587/`)?  Is this documented
to work in the credential manual page?  (To be clear, I would be very
surprised if the answer to any of these were "yes" because I've
literally never seen this usage before with Git, but I am open to
updating my knowledge if that's the case.)

If not, then I think the proper thing to do is to have `git send-email`
rewrite the name into a port instead of having the netrc credential
helper learn to handle non-numeric ports.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Maxim Cournoyer]

Hi,

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2025-06-20 at 04:12:39, Maxim Cournoyer wrote:
>> Symbolic ports were previously silently dropped, which made it
>> impossible to use them with git-credential-netrc. This is a supported
>> use case according to 'man git-send-email', for --smtp-server-port:
>> 
>>    [...] symbolic port names (e.g. "submission" instead of 587) are
>>    also accepted.
>
> Does this work with credential managers in general (that is, in
> non-email contexts, such as HTTP)?  Also, do credential managers in
> general properly find credentials when they're stored in one form and
> looked up in another?  If so, is that still true when the lookup is in
> the URL form (e.g., `smtp://mail.example.com:587/`)?  Is this documented
> to work in the credential manual page?

I'm quite new to the credential-manager of git, so I do not have an
answer to these excellent questions.  But, as some perhaps useful
datapoint, at least using Emacs's auth-source library with a
~/.authinfo.gpg file (which is in the netrc format), if you use a
symbolic port name, you have to use it everywhere if you want
auth-source to match it correctly (it doesn't translate smtps to 465 for
example). If you put 'port smtps' in the .authinfo.gpg but specify the
SMTP port in the your Emacs MTA to a integer like 465, it won't match.

This could be considered a bug in auth-source.el, and git
credential-manager can do better by converting all port input values to
their integer form, as you suggested.  Then mismatched configurations
(e.g.: smtps in netrc and sendemail.smtpServerPort = 465 or vice-versa)
would be handled correctly.

> (To be clear, I would be very
> surprised if the answer to any of these were "yes" because I've
> literally never seen this usage before with Git, but I am open to
> updating my knowledge if that's the case.)
>
> If not, then I think the proper thing to do is to have `git send-email`
> rewrite the name into a port instead of having the netrc credential
> helper learn to handle non-numeric ports.

Did I understand correctly with my suggestion/rewording of yours above?
git-credential-netrc reads its input from the netrc file, which may well
have a symbolic port, so it should itself convert from symbolic to
actual port numbers, IIUC.

-- 
Thanks,
Maxim

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Maxim Cournoyer]

Hi,

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Jun 20 2025, Junio C Hamano wrote:
>
>> Do we know symbolic port names are always limited to alnums?  Or on
>> some systems some byte values in the fringe, like "_" or "-", are
>> also allowed?
>
> Valid service names are documented in RFC6335.  Specifically it allows
> hyphens, but not underscores.

Thanks for the reference. I'm thinking at the moment to improve the
check for a valid port using something like this Scheme code:

--8<---------------cut here---------------start------------->8---
(define (port? port)
 "Return the numeric value for PORT, else #f."
 (if (and (exact-integer? port)
          (positive? port)
          (<= port (1- (expt 2 16))))
     port
     (catch 'system-error
      (lambda () (servent:port (getservbyname port "")))
      (const #f))))
scheme@(guile-user)> (port? "465")
$14 = #f
scheme@(guile-user)> (port? 465)
$15 = 465
scheme@(guile-user)> (port? "smtps")
$16 = 465
scheme@(guile-user)> (port? "unknown")
$17 = #f
scheme@(guile-user)> (port? 120000)
$18 = #f
--8<---------------cut here---------------end--------------->8---

So basically, try to call getservname(2) on a non-numeric port. If this
fails, the port is invalid, else return the port number.

-- 
Thanks,
Maxim

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[brian m. carlson]

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

On 2025-06-21 at 13:29:28, Maxim Cournoyer wrote:
> I'm quite new to the credential-manager of git, so I do not have an
> answer to these excellent questions.  But, as some perhaps useful
> datapoint, at least using Emacs's auth-source library with a
> ~/.authinfo.gpg file (which is in the netrc format), if you use a
> symbolic port name, you have to use it everywhere if you want
> auth-source to match it correctly (it doesn't translate smtps to 465 for
> example). If you put 'port smtps' in the .authinfo.gpg but specify the
> SMTP port in the your Emacs MTA to a integer like 465, it won't match.
> 
> This could be considered a bug in auth-source.el, and git
> credential-manager can do better by converting all port input values to
> their integer form, as you suggested.  Then mismatched configurations
> (e.g.: smtps in netrc and sendemail.smtpServerPort = 465 or vice-versa)
> would be handled correctly.

Yes, I would say that we should be using numeric ports everywhere in the
credential protocol.  If `git send-email` receives "submission" as the
port, then it needs to convert that to "587" before it even requests a
credential.

The git-credential(1) documentation says this for the `host` entry in
the protocol:

    The remote hostname for a network credential. This includes the port
    number if one was specified (e.g., "example.com:8088").

Note that that says "port number", not "symbolic port".

So I think we'd need some answers as to what's going over the protocol
first and how it works for built-in Git functionality (e.g., HTTPS)
before we decide if this is a change we want to make.

> Did I understand correctly with my suggestion/rewording of yours above?
> git-credential-netrc reads its input from the netrc file, which may well
> have a symbolic port, so it should itself convert from symbolic to
> actual port numbers, IIUC.

The netrc format is actually underspecified and libcurl doesn't support
ports at all, so I would not say that using a symbolic port is a good
idea or reliably supported in general.  In fact, I would say that the
netrc credential helper is the only tool I can find that accepts ports
at all.  I've looked at multiple different tools and manual pages online
and the `port` or `protocol` key is not even mentioned.

If we do accept symbolic ports in the netrc file, then we need to
convert them to a numeric port before sending anything over the
protocol, which I don't believe your patch does.  Perl does offer
`getservbyname` for this purpose, so it shouldn't be too difficult to
make this change.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 0/3] git-credential-netrc: better symbolic port names support

[Maxim Cournoyer]

Change tested with 'make test' as well as
'make -C contrib/credential/netrc testverbose', plus manual tests (such as
sending this series!)

Maxim Cournoyer (3):
  contrib: use a more portable shebang for git-credential-netrc
  contrib: warn for invalid netrc file ports in git-credential-netrc
  contrib: better support symbolic port names in git-credential-netrc

 .../credential/netrc/git-credential-netrc.perl    | 14 +++++++++++---
 contrib/credential/netrc/test.pl                  |  8 ++++----
 git-send-email.perl                               | 11 +++++++++++
 perl/Git.pm                                       | 15 +++++++++++++++
 t/t9001-send-email.sh                             |  7 +++++++
 5 files changed, 48 insertions(+), 7 deletions(-)

base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
-- 
2.49.0

[PATCH v2 1/3] contrib: use a more portable shebang for git-credential-netrc

[Maxim Cournoyer]

While the installed scripts have their Perl shebang set to PERL_PATH,
it is nevertheless useful to be able to run the uninstalled script for
manual tests while developing. This change makes the shebang more
portable by having the perl command looked from PATH instead of from a
fixed location.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 9fb998ae09..514f68d00b 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;

base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
-- 
2.49.0

[PATCH v2 2/3] contrib: warn for invalid netrc file ports in git-credential-netrc

[Maxim Cournoyer]

Invalid ports were previously silently dropped; now a warning message
is produced.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 514f68d00b..09d77b4f69 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -267,9 +267,14 @@ sub load_netrc {
 		if (!defined $nentry->{machine}) {
 			next;
 		}
-		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
-			$num_port = $nentry->{port};
-			delete $nentry->{port};
+		if (defined $nentry->{port}) {
+			if ($nentry->{port} =~ m/^\d+$/) {
+				$num_port = $nentry->{port};
+				delete $nentry->{port};
+			} else {
+				printf(STDERR "ignoring invalid port `%s' " .
+				       "from netrc file\n", $nentry->{port});
+			}
 		}
 
 		# create the new entry for the credential helper protocol
-- 
2.49.0

[PATCH v2 3/3] contrib: better support symbolic port names in git-credential-netrc

[Maxim Cournoyer]

To improve support for symbolic port names in netrc files, this
changes does the following:

 - Treat symbolic port names as ports, not protocols in git-credential-netrc
 - Validate the SMTP server port provided to send-email
 - Convert the above symbolic port names to their numerical values.

Before this change, it was not possible to have a SMTP server port set
to "smtps" in a netrc file (e.g. Emacs' ~/.authinfo.gpg), as it would
be registered as a protocol and break the match for a "smtp" protocol
host, as queried for by git-send-email.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 .../credential/netrc/git-credential-netrc.perl    | 11 +++++++----
 contrib/credential/netrc/test.pl                  |  8 ++++----
 git-send-email.perl                               | 11 +++++++++++
 perl/Git.pm                                       | 15 +++++++++++++++
 t/t9001-send-email.sh                             |  7 +++++++
 5 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 09d77b4f69..72d6b6a974 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -268,13 +268,16 @@ sub load_netrc {
 			next;
 		}
 		if (defined $nentry->{port}) {
-			if ($nentry->{port} =~ m/^\d+$/) {
-				$num_port = $nentry->{port};
-				delete $nentry->{port};
-			} else {
+			$num_port = Git::is_port($nentry->{port});
+			unless ($num_port) {
 				printf(STDERR "ignoring invalid port `%s' " .
 				       "from netrc file\n", $nentry->{port});
 			}
+			# Since we've already validated and converted
+			# the port to its numerical value, do not
+			# capture it as the `protocol' value, as used
+			# to be the case for symbolic port names.
+			delete $nentry->{port};
 		}
 
 		# create the new entry for the credential helper protocol
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 67a0ede564..8a7fc2588a 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -45,7 +45,7 @@ BEGIN
 diag "Testing with invalid data\n";
 $cred = run_credential(['-f', $netrc, 'get'],
 		       "bad data");
-ok(scalar keys %$cred == 4, "Got first found keys with bad data");
+ok(scalar keys %$cred == 3, "Got first found keys with bad data");
 
 diag "Testing netrc file for a missing corovamilkbar entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
@@ -64,12 +64,12 @@ BEGIN
 
 diag "Testing netrc file for a username-specific entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
-		       { host => 'imap', username => 'bob' });
+		       { host => 'imap:993', username => 'bob' });
 
-ok(scalar keys %$cred == 2, "Got 2 username-specific keys");
+# Only the password field gets returned.
+ok(scalar keys %$cred == 1, "Got 1 username-specific keys");
 
 is($cred->{password}, 'bobwillknow', "Got correct user-specific password");
-is($cred->{protocol}, 'imaps', "Got correct user-specific protocol");
 
 diag "Testing netrc file for a host:port-specific entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
diff --git a/git-send-email.perl b/git-send-email.perl
index 659e6c588b..502c7d9e04 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -2101,6 +2101,17 @@ sub initialize_modified_loop_vars {
 		}
 	}
 
+	# Validate the SMTP server port, if provided.
+	if (defined $smtp_server_port) {
+		my $port = Git::is_port($smtp_server_port);
+		if ($port) {
+			$smtp_server_port = $port;
+		} else  {
+			die sprintf(__("error: invalid SMTP port '%s'\n"),
+				    $smtp_server_port);
+		}
+	}
+
 	# Run the loop once again to avoid gaps in the counter due to FIFO
 	# arguments provided by the user.
 	my $num = 1;
diff --git a/perl/Git.pm b/perl/Git.pm
index 6f47d653ab..1fa535e1ad 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1061,6 +1061,21 @@ sub _close_cat_blob {
 	delete @$self{@vars};
 }
 
+# Predicate to check whether PORT is a valid port number or service
+# name. The numerical value of PORT is returned, else undef if
+# invalid.
+sub is_port {
+    my ($port) = @_;
+
+    # Port can be either a positive integer within the 16-bit range...
+    if ($port =~ /^\d+$/ && $port > 0 && $port <= (2**16 - 1)) {
+        return $port;
+    }
+
+    # ... or a symbolic port (service name).
+    my $num = getservbyname($port, '');
+    return defined $num ? $num : undef;
+}
 
 =item credential_read( FILEHANDLE )
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 0c1af43f6f..3e749175ab 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,13 @@ test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '
 	test_cmp expected-cc commandline1
 '
 
+test_expect_failure $PREREQ 'invalid smtp server port value' '
+	clean_fake_sendmail &&
+	git send-email -1 --to=recipient@example.com \
+                --smtp-server-port=bogus-symbolic-name \
+		--smtp-server="$(pwd)/fake.sendmail"
+'
+
 test_expect_success $PREREQ 'setup expect' "
 cat >expected-show-all-headers <<\EOF
 0001-Second.patch
-- 
2.49.0

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Maxim Cournoyer]

Hi Junio,

tldr; all changes discussed implemented in posted v2.

Junio C Hamano <gitster@pobox.com> writes:

> Maxim Cournoyer <maxim@guixotic.coop> writes:
>
>> Subject: Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.
>
> Please downcase "Honor" and drop the final full stop, per convention
> (see "git shortlog --no-merges --since=2.months" for examples).

Done.

>> Symbolic ports were previously silently dropped, which made it
>> impossible to use them with git-credential-netrc.
>
> Wouldn't it make sense to issue a warning message when a defined
> $nentry->{port} is not unrecognized?  Wouldn't it make sense to
> do so even before we add this new feature?

I agree it's subpar that the current code silently drops the port when
it doesn't match the expected form. Since port values aren't validated
in 'git-send-email' at the moment, a proper fix would be to have routine
to validate ports in a common library and applied everywhere a port is
read from the user or a config file, ideally, in git-send-email or
elsewhere.  Maybe it could live in Git.pm ?

Edit: Done.

>> This is a supported
>> use case according to 'man git-send-email', for --smtp-server-port:
>>
>>    [...] symbolic port names (e.g. "submission" instead of 587) are
>>    also accepted.
>> ---
>
> Missing sign-off?  See Documentation/SubmittingPatches

Done.

>>  contrib/credential/netrc/git-credential-netrc.perl | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/credential/netrc/git-credential-netrc.perl
>> b/contrib/credential/netrc/git-credential-netrc.perl
>> index 9fb998ae09..ad06000b9f 100755
>> --- a/contrib/credential/netrc/git-credential-netrc.perl
>> +++ b/contrib/credential/netrc/git-credential-netrc.perl
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/perl
>> +#!/usr/bin/env perl
>
> An unrelated change to introduce the use of /usr/bin/env in this
> patch is unwelcome.  Besides, this is a source that is processed
> by the nearby Makefile, which uses the toplevel genererate-perl.sh
> to turn the "#!.../perl" line to name the correct $PERL_PATH before
> the build product gets installed, so I suspect that this change is
> totally unnecessary.

It was necessary on my system to test the uninstalled version, which I
simply symlinked to ~/.local/bin/git-credential-netrc for ease of
testing. I've split this small change in its own commit. Using env
in shebangs instead of hard-coded locations is good for portability in
general, and the generate-perl.sh substitution will work still.

>> @@ -267,7 +267,9 @@ sub load_netrc {
>>  		if (!defined $nentry->{machine}) {
>>  			next;
>>  		}
>> -		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
>> +		if (defined $nentry->{port} && $nentry->{port} =~ m/^[[:alnum:]]+$/) {
>> +			# Port may be either an integer or a symbolic
>> +			# name, e.g. "smtps".
>
> Do we know symbolic port names are always limited to alnums?  Or on
> some systems some byte values in the fringe, like "_" or "-", are
> also allowed?

Looking at /etc/services on my system, I see hyphens, indeed, e.g.:
're-mail-ck'.  That's now handled by the `is_port' predicate, which uses
the libc `getservbyname' call to determine if a non-numeric port is a
valid service/symbolic port name.

I've sent a v2 revision which hopefully includes all of the above
suggestion/changes.

-- 
Maxim

Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.

[Maxim Cournoyer]

Hi Junio,

tldr; all changes discussed implemented in posted v2.

Junio C Hamano <gitster@pobox.com> writes:

> Maxim Cournoyer <maxim@guixotic.coop> writes:
>
>> Subject: Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.
>
> Please downcase "Honor" and drop the final full stop, per convention
> (see "git shortlog --no-merges --since=2.months" for examples).

Done.

>> Symbolic ports were previously silently dropped, which made it
>> impossible to use them with git-credential-netrc.
>
> Wouldn't it make sense to issue a warning message when a defined
> $nentry->{port} is not unrecognized?  Wouldn't it make sense to
> do so even before we add this new feature?

I agree it's subpar that the current code silently drops the port when
it doesn't match the expected form. Since port values aren't validated
in 'git-send-email' at the moment, a proper fix would be to have routine
to validate ports in a common library and applied everywhere a port is
read from the user or a config file, ideally, in git-send-email or
elsewhere.  Maybe it could live in Git.pm ?

Edit: Done.

>> This is a supported
>> use case according to 'man git-send-email', for --smtp-server-port:
>>
>>    [...] symbolic port names (e.g. "submission" instead of 587) are
>>    also accepted.
>> ---
>
> Missing sign-off?  See Documentation/SubmittingPatches

Done.

>>  contrib/credential/netrc/git-credential-netrc.perl | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/credential/netrc/git-credential-netrc.perl
>> b/contrib/credential/netrc/git-credential-netrc.perl
>> index 9fb998ae09..ad06000b9f 100755
>> --- a/contrib/credential/netrc/git-credential-netrc.perl
>> +++ b/contrib/credential/netrc/git-credential-netrc.perl
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/perl
>> +#!/usr/bin/env perl
>
> An unrelated change to introduce the use of /usr/bin/env in this
> patch is unwelcome.  Besides, this is a source that is processed
> by the nearby Makefile, which uses the toplevel genererate-perl.sh
> to turn the "#!.../perl" line to name the correct $PERL_PATH before
> the build product gets installed, so I suspect that this change is
> totally unnecessary.

It was necessary on my system to test the uninstalled version, which I
simply symlinked to ~/.local/bin/git-credential-netrc for ease of
testing. I've split this small change in its own commit. Using env
in shebangs instead of hard-coded locations is good for portability in
general, and the generate-perl.sh substitution will work still.

>> @@ -267,7 +267,9 @@ sub load_netrc {
>>  		if (!defined $nentry->{machine}) {
>>  			next;
>>  		}
>> -		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
>> +		if (defined $nentry->{port} && $nentry->{port} =~ m/^[[:alnum:]]+$/) {
>> +			# Port may be either an integer or a symbolic
>> +			# name, e.g. "smtps".
>
> Do we know symbolic port names are always limited to alnums?  Or on
> some systems some byte values in the fringe, like "_" or "-", are
> also allowed?

Looking at /etc/services on my system, I see hyphens, indeed, e.g.:
're-mail-ck'.  That's now handled by the `is_port' predicate, which uses
the getservbyname(3) call to determine if a non-numeric port is a
valid service/symbolic port name.

I've sent a v2 revision which hopefully addresses all of the above
suggestions/changes.

-- 
Maxim

Re: [PATCH v2 3/3] contrib: better support symbolic port names in git-credential-netrc

[Junio C Hamano]

Maxim Cournoyer <maxim@guixotic.coop> writes:

> diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
> index 09d77b4f69..72d6b6a974 100755
> --- a/contrib/credential/netrc/git-credential-netrc.perl
> +++ b/contrib/credential/netrc/git-credential-netrc.perl
> @@ -268,13 +268,16 @@ sub load_netrc {
>  			next;
>  		}
>  		if (defined $nentry->{port}) {
> -			if ($nentry->{port} =~ m/^\d+$/) {
> -				$num_port = $nentry->{port};
> -				delete $nentry->{port};
> -			} else {
> +			$num_port = Git::is_port($nentry->{port});
> +			unless ($num_port) {
>  				printf(STDERR "ignoring invalid port `%s' " .
>  				       "from netrc file\n", $nentry->{port});
>  			}
> +			# Since we've already validated and converted
> +			# the port to its numerical value, do not
> +			# capture it as the `protocol' value, as used
> +			# to be the case for symbolic port names.
> +			delete $nentry->{port};
>  		}

OK, so we rewrite textual service names into port number, and
normalize the "host" member of the entry read from the file into
"host:port" form.  Earlier we did that only for numeric port
numbers.  Nice.

> diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
> index 67a0ede564..8a7fc2588a 100755
> --- a/contrib/credential/netrc/test.pl
> +++ b/contrib/credential/netrc/test.pl
> @@ -45,7 +45,7 @@ BEGIN
>  diag "Testing with invalid data\n";
>  $cred = run_credential(['-f', $netrc, 'get'],
>  		       "bad data");
> -ok(scalar keys %$cred == 4, "Got first found keys with bad data");
> +ok(scalar keys %$cred == 3, "Got first found keys with bad data");
>  
>  diag "Testing netrc file for a missing corovamilkbar entry\n";
>  $cred = run_credential(['-f', $netrc, 'get'],
> @@ -64,12 +64,12 @@ BEGIN
>  
>  diag "Testing netrc file for a username-specific entry\n";
>  $cred = run_credential(['-f', $netrc, 'get'],
> -		       { host => 'imap', username => 'bob' });
> +		       { host => 'imap:993', username => 'bob' });

Is this rewriting an existing test, instead of adding a new test to
trigger a feature that didn't have a test coverage, while keeping
the old test?  I am wondering if we want to ensure that both
":port"-less case and "host:port" case keep working even after the
change to -netrc credential helper in this patch.

> diff --git a/perl/Git.pm b/perl/Git.pm
> index 6f47d653ab..1fa535e1ad 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1061,6 +1061,21 @@ sub _close_cat_blob {
>  	delete @$self{@vars};
>  }
>  
> +# Predicate to check whether PORT is a valid port number or service
> +# name. The numerical value of PORT is returned, else undef if
> +# invalid.

Hmph.  It _can_ be used to validate a random end-user supplied
string names a port, either by being a port number in the valid
range or by being a valid service name.  But another use case in the
code after this patch applied that is equally if not more important
is to ensure that a valid port specified by the end-user is turned
into a port number.  We should not name such a sub as if its primary
functionality is to serve as a Boolean "is_foo".  Perhaps call it
port_num or something?

> +sub is_port {
> +    my ($port) = @_;
> +
> +    # Port can be either a positive integer within the 16-bit range...
> +    if ($port =~ /^\d+$/ && $port > 0 && $port <= (2**16 - 1)) {
> +        return $port;
> +    }
> +
> +    # ... or a symbolic port (service name).
> +    my $num = getservbyname($port, '');
> +    return defined $num ? $num : undef;

Wouldn't "return $num" work here?  getservbyname() would return
"undef" when the given $port is not a valid service name anyway, no?

Or even "return scalar getservbyname($port, 'tcp')" without an
intermediate variable $num?

> +}
>  
>  =item credential_read( FILEHANDLE )
>  
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 0c1af43f6f..3e749175ab 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -201,6 +201,13 @@ test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '
>  	test_cmp expected-cc commandline1
>  '
>  
> +test_expect_failure $PREREQ 'invalid smtp server port value' '
> +	clean_fake_sendmail &&
> +	git send-email -1 --to=recipient@example.com \
> +                --smtp-server-port=bogus-symbolic-name \
> +		--smtp-server="$(pwd)/fake.sendmail"
> +'
> +
>  test_expect_success $PREREQ 'setup expect' "
>  cat >expected-show-all-headers <<\EOF
>  0001-Second.patch

[PATCH v3 0/3] git-credential-netrc: better symbolic port names support

[Maxim Cournoyer]

Most suggestions from Junio have been applied in this revision.

Changes in v3:
 - rename is_port to port_num
 - directly return scalar value from getservbyname in port_num

Thanks,

Maxim Cournoyer (3):
  contrib: use a more portable shebang for git-credential-netrc
  contrib: warn for invalid netrc file ports in git-credential-netrc
  contrib: better support symbolic port names in git-credential-netrc

 contrib/credential/netrc/git-credential-netrc.perl | 14 +++++++++++---
 contrib/credential/netrc/test.pl                   |  8 ++++----
 git-send-email.perl                                | 11 +++++++++++
 perl/Git.pm                                        | 13 +++++++++++++
 t/t9001-send-email.sh                              |  7 +++++++
 5 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.49.0

[PATCH v3 1/3] contrib: use a more portable shebang for git-credential-netrc

[Maxim Cournoyer]

While the installed scripts have their Perl shebang set to PERL_PATH,
it is nevertheless useful to be able to run the uninstalled script for
manual tests while developing. This change makes the shebang more
portable by having the perl command looked from PATH instead of from a
fixed location.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 9fb998ae09..514f68d00b 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;
-- 
2.49.0

[PATCH v3 2/3] contrib: warn for invalid netrc file ports in git-credential-netrc

[Maxim Cournoyer]

Invalid ports were previously silently dropped; now a warning message
is produced.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 514f68d00b..09d77b4f69 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -267,9 +267,14 @@ sub load_netrc {
 		if (!defined $nentry->{machine}) {
 			next;
 		}
-		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
-			$num_port = $nentry->{port};
-			delete $nentry->{port};
+		if (defined $nentry->{port}) {
+			if ($nentry->{port} =~ m/^\d+$/) {
+				$num_port = $nentry->{port};
+				delete $nentry->{port};
+			} else {
+				printf(STDERR "ignoring invalid port `%s' " .
+				       "from netrc file\n", $nentry->{port});
+			}
 		}
 
 		# create the new entry for the credential helper protocol
-- 
2.49.0

[PATCH v3 3/3] contrib: better support symbolic port names in git-credential-netrc

[Maxim Cournoyer]

To improve support for symbolic port names in netrc files, this
changes does the following:

 - Treat symbolic port names as ports, not protocols in git-credential-netrc
 - Validate the SMTP server port provided to send-email
 - Convert the above symbolic port names to their numerical values.

Before this change, it was not possible to have a SMTP server port set
to "smtps" in a netrc file (e.g. Emacs' ~/.authinfo.gpg), as it would
be registered as a protocol and break the match for a "smtp" protocol
host, as queried for by git-send-email.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 11 +++++++----
 contrib/credential/netrc/test.pl                   |  8 ++++----
 git-send-email.perl                                | 11 +++++++++++
 perl/Git.pm                                        | 13 +++++++++++++
 t/t9001-send-email.sh                              |  7 +++++++
 5 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 09d77b4f69..3c0a532d0e 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -268,13 +268,16 @@ sub load_netrc {
 			next;
 		}
 		if (defined $nentry->{port}) {
-			if ($nentry->{port} =~ m/^\d+$/) {
-				$num_port = $nentry->{port};
-				delete $nentry->{port};
-			} else {
+			$num_port = Git::port_num($nentry->{port});
+			unless ($num_port) {
 				printf(STDERR "ignoring invalid port `%s' " .
 				       "from netrc file\n", $nentry->{port});
 			}
+			# Since we've already validated and converted
+			# the port to its numerical value, do not
+			# capture it as the `protocol' value, as used
+			# to be the case for symbolic port names.
+			delete $nentry->{port};
 		}
 
 		# create the new entry for the credential helper protocol
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 67a0ede564..8a7fc2588a 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -45,7 +45,7 @@ BEGIN
 diag "Testing with invalid data\n";
 $cred = run_credential(['-f', $netrc, 'get'],
 		       "bad data");
-ok(scalar keys %$cred == 4, "Got first found keys with bad data");
+ok(scalar keys %$cred == 3, "Got first found keys with bad data");
 
 diag "Testing netrc file for a missing corovamilkbar entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
@@ -64,12 +64,12 @@ BEGIN
 
 diag "Testing netrc file for a username-specific entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
-		       { host => 'imap', username => 'bob' });
+		       { host => 'imap:993', username => 'bob' });
 
-ok(scalar keys %$cred == 2, "Got 2 username-specific keys");
+# Only the password field gets returned.
+ok(scalar keys %$cred == 1, "Got 1 username-specific keys");
 
 is($cred->{password}, 'bobwillknow', "Got correct user-specific password");
-is($cred->{protocol}, 'imaps', "Got correct user-specific protocol");
 
 diag "Testing netrc file for a host:port-specific entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
diff --git a/git-send-email.perl b/git-send-email.perl
index 659e6c588b..d2cf9b717a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -2101,6 +2101,17 @@ sub initialize_modified_loop_vars {
 		}
 	}
 
+	# Validate the SMTP server port, if provided.
+	if (defined $smtp_server_port) {
+		my $port = Git::port_num($smtp_server_port);
+		if ($port) {
+			$smtp_server_port = $port;
+		} else  {
+			die sprintf(__("error: invalid SMTP port '%s'\n"),
+				    $smtp_server_port);
+		}
+	}
+
 	# Run the loop once again to avoid gaps in the counter due to FIFO
 	# arguments provided by the user.
 	my $num = 1;
diff --git a/perl/Git.pm b/perl/Git.pm
index 6f47d653ab..090cf77dab 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1061,6 +1061,19 @@ sub _close_cat_blob {
 	delete @$self{@vars};
 }
 
+# Given PORT, a port number or service name, return its numerical
+# value else undef.
+sub port_num {
+    my ($port) = @_;
+
+    # Port can be either a positive integer within the 16-bit range...
+    if ($port =~ /^\d+$/ && $port > 0 && $port <= (2**16 - 1)) {
+        return $port;
+    }
+
+    # ... or a symbolic port (service name).
+    return scalar getservbyname($port, '');
+}
 
 =item credential_read( FILEHANDLE )
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 0c1af43f6f..3e749175ab 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,13 @@ test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '
 	test_cmp expected-cc commandline1
 '
 
+test_expect_failure $PREREQ 'invalid smtp server port value' '
+	clean_fake_sendmail &&
+	git send-email -1 --to=recipient@example.com \
+                --smtp-server-port=bogus-symbolic-name \
+		--smtp-server="$(pwd)/fake.sendmail"
+'
+
 test_expect_success $PREREQ 'setup expect' "
 cat >expected-show-all-headers <<\EOF
 0001-Second.patch
-- 
2.49.0

Re: [PATCH v2 3/3] contrib: better support symbolic port names in git-credential-netrc

[Maxim Cournoyer]

Hi!

tl;dr: I've submitted a v3 with most of your suggestions implemented.

Junio C Hamano <gitster@pobox.com> writes:

[...]

>> diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
>> index 67a0ede564..8a7fc2588a 100755
>> --- a/contrib/credential/netrc/test.pl
>> +++ b/contrib/credential/netrc/test.pl
>> @@ -45,7 +45,7 @@ BEGIN
>>  diag "Testing with invalid data\n";
>>  $cred = run_credential(['-f', $netrc, 'get'],
>>  		       "bad data");
>> -ok(scalar keys %$cred == 4, "Got first found keys with bad data");
>> +ok(scalar keys %$cred == 3, "Got first found keys with bad data");
>>  
>>  diag "Testing netrc file for a missing corovamilkbar entry\n";
>>  $cred = run_credential(['-f', $netrc, 'get'],
>> @@ -64,12 +64,12 @@ BEGIN
>>  
>>  diag "Testing netrc file for a username-specific entry\n";
>>  $cred = run_credential(['-f', $netrc, 'get'],
>> -		       { host => 'imap', username => 'bob' });
>> +		       { host => 'imap:993', username => 'bob' });
>
> Is this rewriting an existing test, instead of adding a new test to
> trigger a feature that didn't have a test coverage, while keeping
> the old test?  I am wondering if we want to ensure that both
> ":port"-less case and "host:port" case keep working even after the
> change to -netrc credential helper in this patch.

That specific test *is* using a port, but a symbolic one (imaps), which
used to be captured as the 'protocol' in the Git credential hash/array.
Now it's captured properly as a port, which is represented in Git
credential by joining it with the host name. The test needed adjusting
for that.

[...]

> Hmph.  It _can_ be used to validate a random end-user supplied
> string names a port, either by being a port number in the valid
> range or by being a valid service name.  But another use case in the
> code after this patch applied that is equally if not more important
> is to ensure that a valid port specified by the end-user is turned
> into a port number.  We should not name such a sub as if its primary
> functionality is to serve as a Boolean "is_foo".  Perhaps call it
> port_num or something?

Naming is hard :-). I like your suggestion. Done.

>> +sub is_port {
>> +    my ($port) = @_;
>> +
>> +    # Port can be either a positive integer within the 16-bit range...
>> +    if ($port =~ /^\d+$/ && $port > 0 && $port <= (2**16 - 1)) {
>> +        return $port;
>> +    }
>> +
>> +    # ... or a symbolic port (service name).
>> +    my $num = getservbyname($port, '');
>> +    return defined $num ? $num : undef;
>
> Wouldn't "return $num" work here?  getservbyname() would return
> "undef" when the given $port is not a valid service name anyway, no?
>
> Or even "return scalar getservbyname($port, 'tcp')" without an
> intermediate variable $num?

I've re-read the doc (perldoc -f getservbyname) and you are right, in a
scalar context it would return an undef value when the service name was
not found in the local database. Done!

-- 
Thanks,
Maxim

Re: [PATCH v3 0/3] git-credential-netrc: better symbolic port names support

[Junio C Hamano]

Maxim Cournoyer <maxim@guixotic.coop> writes:

> Most suggestions from Junio have been applied in this revision.
>
> Changes in v3:
>  - rename is_port to port_num
>  - directly return scalar value from getservbyname in port_num
>
> Thanks,
>
> Maxim Cournoyer (3):
>   contrib: use a more portable shebang for git-credential-netrc
>   contrib: warn for invalid netrc file ports in git-credential-netrc
>   contrib: better support symbolic port names in git-credential-netrc
>
>  contrib/credential/netrc/git-credential-netrc.perl | 14 +++++++++++---
>  contrib/credential/netrc/test.pl                   |  8 ++++----
>  git-send-email.perl                                | 11 +++++++++++
>  perl/Git.pm                                        | 13 +++++++++++++
>  t/t9001-send-email.sh                              |  7 +++++++
>  5 files changed, 46 insertions(+), 7 deletions(-)

v2 and this iteration both have all messages set as replies to a
single message in the old thread.

Please make sure in your future submissions:

 - [0/n] is a reply to [0/m] of the previous iteration.

 - [1/n], [2/n], ... and [n/n] are all replies to [0/n] of the same
   iteration.

Re: [PATCH v3 3/3] contrib: better support symbolic port names in git-credential-netrc

[Junio C Hamano]

Maxim Cournoyer <maxim@guixotic.coop> writes:

> +	git send-email -1 --to=recipient@example.com \
> +                --smtp-server-port=bogus-symbolic-name \
> +		--smtp-server="$(pwd)/fake.sendmail"
> +'

There is a funny indent-with-spaces here.

Re: [PATCH v3 0/3] git-credential-netrc: better symbolic port names support

[Maxim Cournoyer]

Hi,

Junio C Hamano <gitster@pobox.com> writes:

[...]

> v2 and this iteration both have all messages set as replies to a
> single message in the old thread.
>
> Please make sure in your future submissions:
>
>  - [0/n] is a reply to [0/m] of the previous iteration.
>
>  - [1/n], [2/n], ... and [n/n] are all replies to [0/n] of the same
>    iteration.

OK. This means I need to submit with 'git send-email' in two steps,
right?

-- 
Thanks,
Maxim

Re: [PATCH v3 0/3] git-credential-netrc: better symbolic port names support

[Junio C Hamano]

Maxim Cournoyer <maxim@guixotic.coop> writes:

> Hi,
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
>> v2 and this iteration both have all messages set as replies to a
>> single message in the old thread.
>>
>> Please make sure in your future submissions:
>>
>>  - [0/n] is a reply to [0/m] of the previous iteration.
>>
>>  - [1/n], [2/n], ... and [n/n] are all replies to [0/n] of the same
>>    iteration.
>
> OK. This means I need to submit with 'git send-email' in two steps,
> right?

I do not think so.  Find description of the "--in-reply-to" option
in the documentation, and read about interactions with "--thread"
and "--no-chain-reply-to" there?

    So for example when `--thread` and `--no-chain-reply-to` are specified, the
    second and subsequent patches will be replies to the first one like in the
    illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`:

      [PATCH 0/2] Here is what I did...
        [PATCH 1/2] Clean up and tests
        [PATCH 2/2] Implementation
        [PATCH v2 0/3] Here is a reroll
          [PATCH v2 1/3] Clean up
          [PATCH v2 2/3] New tests
          [PATCH v2 3/3] Implementation

Re: [PATCH v3 0/3] git-credential-netrc: better symbolic port names support

[Maxim Cournoyer]

Hello,

[...]

>>> v2 and this iteration both have all messages set as replies to a
>>> single message in the old thread.
>>>
>>> Please make sure in your future submissions:
>>>
>>>  - [0/n] is a reply to [0/m] of the previous iteration.
>>>
>>>  - [1/n], [2/n], ... and [n/n] are all replies to [0/n] of the same
>>>    iteration.
>>
>> OK. This means I need to submit with 'git send-email' in two steps,
>> right?
>
> I do not think so.  Find description of the "--in-reply-to" option
> in the documentation, and read about interactions with "--thread"
> and "--no-chain-reply-to" there?
>
>     So for example when `--thread` and `--no-chain-reply-to` are specified, the
>     second and subsequent patches will be replies to the first one like in the
>     illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`:
>
>       [PATCH 0/2] Here is what I did...
>         [PATCH 1/2] Clean up and tests
>         [PATCH 2/2] Implementation
>         [PATCH v2 0/3] Here is a reroll
>           [PATCH v2 1/3] Clean up
>           [PATCH v2 2/3] New tests
>           [PATCH v2 3/3] Implementation

OK, so as a self-note; this is the default behavior (--thread and
--no-chain-reply-to) and the thing I got wrong was that --in-reply-to
should be set to the Message-ID of the previous revision's cover letter
(in my recent submissions I had kept the message ID of the original cover
letter instead). That's also explained in
documentation/myfirstcontribution.adoc.

I'll now send a v4 fixing the white space issue, making sure to
--in-reply-to=$message-id-of-v3-cover-letter.

-- 
Thanks,
Maxim

[PATCH v4 0/3] git-credential-netrc: better symbolic port names support

[Maxim Cournoyer]

This revision fixes a single white space in a new test added in 3/3.

Maxim Cournoyer (3):
  contrib: use a more portable shebang for git-credential-netrc
  contrib: warn for invalid netrc file ports in git-credential-netrc
  contrib: better support symbolic port names in git-credential-netrc

 contrib/credential/netrc/git-credential-netrc.perl | 14 +++++++++++---
 contrib/credential/netrc/test.pl                   |  8 ++++----
 git-send-email.perl                                | 11 +++++++++++
 perl/Git.pm                                        | 13 +++++++++++++
 t/t9001-send-email.sh                              |  7 +++++++
 5 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.49.0

[PATCH v4 1/3] contrib: use a more portable shebang for git-credential-netrc

[Maxim Cournoyer]

While the installed scripts have their Perl shebang set to PERL_PATH,
it is nevertheless useful to be able to run the uninstalled script for
manual tests while developing. This change makes the shebang more
portable by having the perl command looked from PATH instead of from a
fixed location.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 9fb998ae09..514f68d00b 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;
-- 
2.49.0

[PATCH v4 2/3] contrib: warn for invalid netrc file ports in git-credential-netrc

[Maxim Cournoyer]

Invalid ports were previously silently dropped; now a warning message
is produced.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 514f68d00b..09d77b4f69 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -267,9 +267,14 @@ sub load_netrc {
 		if (!defined $nentry->{machine}) {
 			next;
 		}
-		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
-			$num_port = $nentry->{port};
-			delete $nentry->{port};
+		if (defined $nentry->{port}) {
+			if ($nentry->{port} =~ m/^\d+$/) {
+				$num_port = $nentry->{port};
+				delete $nentry->{port};
+			} else {
+				printf(STDERR "ignoring invalid port `%s' " .
+				       "from netrc file\n", $nentry->{port});
+			}
 		}
 
 		# create the new entry for the credential helper protocol
-- 
2.49.0

[PATCH v4 3/3] contrib: better support symbolic port names in git-credential-netrc

[Maxim Cournoyer]

To improve support for symbolic port names in netrc files, this
changes does the following:

 - Treat symbolic port names as ports, not protocols in git-credential-netrc
 - Validate the SMTP server port provided to send-email
 - Convert the above symbolic port names to their numerical values.

Before this change, it was not possible to have a SMTP server port set
to "smtps" in a netrc file (e.g. Emacs' ~/.authinfo.gpg), as it would
be registered as a protocol and break the match for a "smtp" protocol
host, as queried for by git-send-email.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 contrib/credential/netrc/git-credential-netrc.perl | 11 +++++++----
 contrib/credential/netrc/test.pl                   |  8 ++++----
 git-send-email.perl                                | 11 +++++++++++
 perl/Git.pm                                        | 13 +++++++++++++
 t/t9001-send-email.sh                              |  7 +++++++
 5 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index 09d77b4f69..3c0a532d0e 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -268,13 +268,16 @@ sub load_netrc {
 			next;
 		}
 		if (defined $nentry->{port}) {
-			if ($nentry->{port} =~ m/^\d+$/) {
-				$num_port = $nentry->{port};
-				delete $nentry->{port};
-			} else {
+			$num_port = Git::port_num($nentry->{port});
+			unless ($num_port) {
 				printf(STDERR "ignoring invalid port `%s' " .
 				       "from netrc file\n", $nentry->{port});
 			}
+			# Since we've already validated and converted
+			# the port to its numerical value, do not
+			# capture it as the `protocol' value, as used
+			# to be the case for symbolic port names.
+			delete $nentry->{port};
 		}
 
 		# create the new entry for the credential helper protocol
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 67a0ede564..8a7fc2588a 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -45,7 +45,7 @@ BEGIN
 diag "Testing with invalid data\n";
 $cred = run_credential(['-f', $netrc, 'get'],
 		       "bad data");
-ok(scalar keys %$cred == 4, "Got first found keys with bad data");
+ok(scalar keys %$cred == 3, "Got first found keys with bad data");
 
 diag "Testing netrc file for a missing corovamilkbar entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
@@ -64,12 +64,12 @@ BEGIN
 
 diag "Testing netrc file for a username-specific entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
-		       { host => 'imap', username => 'bob' });
+		       { host => 'imap:993', username => 'bob' });
 
-ok(scalar keys %$cred == 2, "Got 2 username-specific keys");
+# Only the password field gets returned.
+ok(scalar keys %$cred == 1, "Got 1 username-specific keys");
 
 is($cred->{password}, 'bobwillknow', "Got correct user-specific password");
-is($cred->{protocol}, 'imaps', "Got correct user-specific protocol");
 
 diag "Testing netrc file for a host:port-specific entry\n";
 $cred = run_credential(['-f', $netrc, 'get'],
diff --git a/git-send-email.perl b/git-send-email.perl
index 659e6c588b..d2cf9b717a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -2101,6 +2101,17 @@ sub initialize_modified_loop_vars {
 		}
 	}
 
+	# Validate the SMTP server port, if provided.
+	if (defined $smtp_server_port) {
+		my $port = Git::port_num($smtp_server_port);
+		if ($port) {
+			$smtp_server_port = $port;
+		} else  {
+			die sprintf(__("error: invalid SMTP port '%s'\n"),
+				    $smtp_server_port);
+		}
+	}
+
 	# Run the loop once again to avoid gaps in the counter due to FIFO
 	# arguments provided by the user.
 	my $num = 1;
diff --git a/perl/Git.pm b/perl/Git.pm
index 6f47d653ab..090cf77dab 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1061,6 +1061,19 @@ sub _close_cat_blob {
 	delete @$self{@vars};
 }
 
+# Given PORT, a port number or service name, return its numerical
+# value else undef.
+sub port_num {
+    my ($port) = @_;
+
+    # Port can be either a positive integer within the 16-bit range...
+    if ($port =~ /^\d+$/ && $port > 0 && $port <= (2**16 - 1)) {
+        return $port;
+    }
+
+    # ... or a symbolic port (service name).
+    return scalar getservbyname($port, '');
+}
 
 =item credential_read( FILEHANDLE )
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 0c1af43f6f..e56e0c8d77 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,13 @@ test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '
 	test_cmp expected-cc commandline1
 '
 
+test_expect_failure $PREREQ 'invalid smtp server port value' '
+	clean_fake_sendmail &&
+	git send-email -1 --to=recipient@example.com \
+		--smtp-server-port=bogus-symbolic-name \
+		--smtp-server="$(pwd)/fake.sendmail"
+'
+
 test_expect_success $PREREQ 'setup expect' "
 cat >expected-show-all-headers <<\EOF
 0001-Second.patch
-- 
2.49.0

Re: [PATCH v4 0/3] git-credential-netrc: better symbolic port names support

[Junio C Hamano]

Maxim Cournoyer <maxim@guixotic.coop> writes:

> This revision fixes a single white space in a new test added in 3/3.

The contents exactly match what I locally have (as I fixed up the
previous round locally before you sent in this iteration).

[v4 0/3] does not look like a reply to [v3 0/3], though.  It has
these header lines

    Message-ID: <20250625142511.28857-1-maxim@guixotic.coop>
    In-Reply-To: <87ecv8k4y9.fsf@terra.mail-host-address-is-not-set>
    References: <87ecv8k4y9.fsf@terra.mail-host-address-is-not-set>

and refers to the message in the discussion thread of [v3 0/3] in
which you said "I'll now send a v4 fixing the white space issue,
making sure to --in-reply-to=$message-id-of-v3-cover-letter."

No need to resend this round just to fix the threading, of course.

Thanks.

> Maxim Cournoyer (3):
>   contrib: use a more portable shebang for git-credential-netrc
>   contrib: warn for invalid netrc file ports in git-credential-netrc
>   contrib: better support symbolic port names in git-credential-netrc
>
>  contrib/credential/netrc/git-credential-netrc.perl | 14 +++++++++++---
>  contrib/credential/netrc/test.pl                   |  8 ++++----
>  git-send-email.perl                                | 11 +++++++++++
>  perl/Git.pm                                        | 13 +++++++++++++
>  t/t9001-send-email.sh                              |  7 +++++++
>  5 files changed, 46 insertions(+), 7 deletions(-)

Re: [PATCH v4 0/3] git-credential-netrc: better symbolic port names support

[Maxim Cournoyer]

Hi,

Junio C Hamano <gitster@pobox.com> writes:

> Maxim Cournoyer <maxim@guixotic.coop> writes:
>
>> This revision fixes a single white space in a new test added in 3/3.
>
> The contents exactly match what I locally have (as I fixed up the
> previous round locally before you sent in this iteration).
>
> [v4 0/3] does not look like a reply to [v3 0/3], though.  It has
> these header lines
>
>     Message-ID: <20250625142511.28857-1-maxim@guixotic.coop>
>     In-Reply-To: <87ecv8k4y9.fsf@terra.mail-host-address-is-not-set>
>     References: <87ecv8k4y9.fsf@terra.mail-host-address-is-not-set>
>
> and refers to the message in the discussion thread of [v3 0/3] in
> which you said "I'll now send a v4 fixing the white space issue,
> making sure to --in-reply-to=$message-id-of-v3-cover-letter."

Hm, correct.  I picked the first message I saw as [PATCH v3 0/3] at
https://lore.kernel.org/git/ but it was a reply, no the original.  Maybe
I'll get it right in a future submission ^^'.

Thanks again for the previous comments/review.

Cheers,

-- 
Maxim