Hi!
tl;dr: I've submitted a v3 with most of your suggestions implemented.
Junio C Hamano <gitster@xxxxxxxxx> 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
