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

[Junio C Hamano <gitster@pobox.com>]

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