Re: [PATCH v2 3/3] contrib: better support symbolic port names in git-credential-netrc
Maxim Cournoyer <maxim@guixotic.coop> writes:
Show 23 quoted lines
> 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.
Show 19 quoted lines
> 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.
Show 11 quoted lines
> 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?
Show 11 quoted lines
> +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?
Show 22 quoted lines
> +}
>
> =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
