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).
> 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?
> 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
> 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.
> @@ -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?
Overall, I think it is a worthwhile to address this issue. I just
do not want a patch that says "alnum seems to be good enough to
cover the ports I happen to care about" (or a patch does not even
say how it came to the conclusion to use :alnum:)---we should do
better and explain this change with something like "from THIS
SOURCE, the names users may want to use come from this set of names,
where THESE are defined as valid characters, hence we allow not just
digits (for numeric ports), but allowing also :alnum: is sufficient
to cover these symbolic names".
Thanks.
> $num_port = $nentry->{port};
> delete $nentry->{port};
> }
>
> base-commit: 9520f7d9985d8879bddd157309928fc0679c8e92
