Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 20, 2025, 13:48 UTC
- Message-ID
- <xmqqmsa27cdn.fsf@gitster.g>
- In-Reply-To
- <20250620041239.27839-1-maxim@guixotic.coop>
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).
> 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?
Show 6 quoted lines
> 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
Show 10 quoted lines
> 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.
Show 8 quoted lines
> @@ -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.
Show 5 quoted lines
> $num_port = $nentry->{port};
> delete $nentry->{port};
> }
>
> base-commit: 9520f7d9985d8879bddd157309928fc0679c8e92