Re: [PATCH] contrib: Honor symbolic port in git-credential-netrc.
- From
- Maxim Cournoyer <maxim@guixotic.coop>
- Date
- Jun 23, 2025, 03:34 UTC
- Message-ID
- <871prbf7wm.fsf@terra.mail-host-address-is-not-set>
- In-Reply-To
- <xmqqmsa27cdn.fsf@gitster.g>
Hi Junio,
tldr; all changes discussed implemented in posted v2.
Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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.
Show 6 quoted lines
>> 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.
Show 8 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
Done.
Show 18 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.
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.
Show 12 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?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