Hi Junio,
tldr; all changes discussed implemented in posted v2.
Junio C Hamano <gitster@xxxxxxxxx> writes:
> Maxim Cournoyer <maxim@xxxxxxxxxxxxx> 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
