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

[Maxim Cournoyer <maxim@guixotic.coop>]

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