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

[Maxim Cournoyer <maxim@guixotic.coop>]

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