# [PATCH] git-credential-netrc: fix uninitialized warning 7 messages from 2013-10-08 to 2013-10-08. Participants: Ted Zlatanov, Jonathan Nieder, Stefan Beller. Thread: https://gitlist.dev/t/35083 ## Ted Zlatanov, 2013-10-08 14:34 Subject: [PATCH] git-credential-netrc: fix uninitialized warning Message-ID: <87zjqjx25g.fsf@flea.lifelogs.com> URL: https://gitlist.dev/e/87zjqjx25g.fsf%40flea.lifelogs.com ``` Simple patch to avoid unitialized warning and log what we'll do. --- contrib/credential/netrc/git-credential-netrc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contrib/credential/netrc/git-credential-netrc b/contrib/credential/netrc/git-credential-netrc index 6c51c43..13e537b 100755 --- a/contrib/credential/netrc/git-credential-netrc +++ b/contrib/credential/netrc/git-credential-netrc @@ -369,7 +369,10 @@ sub find_netrc_entry { { my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry; foreach my $check (sort keys %$query) { - if (defined $query->{$check}) { + if (!defined $entry->{$check}) { + log_debug("OK: entry has no $check token, so any value satisfies check $check"); + } + elsif (defined $query->{$check}) { log_debug("compare %s [%s] to [%s] (entry: %s)", $check, $entry->{$check}, -- 1.8.1.5 ``` ## Jonathan Nieder, 2013-10-08 19:41 Subject: Re: [PATCH] git-credential-netrc: fix uninitialized warning Message-ID: <20131008194147.GF9464@google.com> URL: https://gitlist.dev/e/20131008194147.GF9464%40google.com In-Reply-To: <87zjqjx25g.fsf@flea.lifelogs.com> ``` Hi, Ted Zlatanov wrote: > Simple patch to avoid unitialized warning and log what we'll do. Sign-off? [...] > --- a/contrib/credential/netrc/git-credential-netrc > +++ b/contrib/credential/netrc/git-credential-netrc > @@ -369,7 +369,10 @@ sub find_netrc_entry { > { > my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry; > foreach my $check (sort keys %$query) { > - if (defined $query->{$check}) { > + if (!defined $entry->{$check}) { > + log_debug("OK: entry has no $check token, so any value satisfies check $check"); > + } > + elsif (defined $query->{$check}) { Style: elsewhere this file seems to use cuddled elses: } elsif (...) { Or more simply, would it make sense to wrap both 'defined' checks into a single "if", like so? if (defined $entry->{$check} && defined $query->{$check}) { ... } else { log_debug(...); } Thanks and hope that helps, Jonathan ``` ## Ted Zlatanov, 2013-10-08 19:55 Subject: Re: [PATCH] git-credential-netrc: fix uninitialized warning Message-ID: <87li23v8p5.fsf@flea.lifelogs.com> URL: https://gitlist.dev/e/87li23v8p5.fsf%40flea.lifelogs.com In-Reply-To: <20131008194147.GF9464@google.com> ``` On Tue, 8 Oct 2013 12:41:47 -0700 Jonathan Nieder wrote: JN> Ted Zlatanov wrote: >> Simple patch to avoid unitialized warning and log what we'll do. JN> Sign-off? I didn't realize it was a requirement, must I? JN> [...] >> --- a/contrib/credential/netrc/git-credential-netrc >> +++ b/contrib/credential/netrc/git-credential-netrc >> @@ -369,7 +369,10 @@ sub find_netrc_entry { >> { >> my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry; >> foreach my $check (sort keys %$query) { >> - if (defined $query->{$check}) { >> + if (!defined $entry->{$check}) { >> + log_debug("OK: entry has no $check token, so any value satisfies check $check"); >> + } >> + elsif (defined $query->{$check}) { JN> Style: elsewhere this file seems to use cuddled elses: JN> } elsif (...) { Ah, thanks, I missed that. JN> Or more simply, would it make sense to wrap both 'defined' checks into JN> a single "if", like so? JN> if (defined $entry->{$check} && defined $query->{$check}) { JN> ... JN> } else { JN> log_debug(...); JN> } I prefer the explicit version because we can issue a more precise log_debug message. Ted ``` ## Stefan Beller, 2013-10-08 19:58 Subject: Re: [PATCH] git-credential-netrc: fix uninitialized warning Message-ID: <525463F1.2050308@googlemail.com> URL: https://gitlist.dev/e/525463F1.2050308%40googlemail.com In-Reply-To: <87li23v8p5.fsf@flea.lifelogs.com> ``` On 10/08/2013 09:55 PM, Ted Zlatanov wrote: > JN> Sign-off? > > I didn't realize it was a requirement, must I? Yes, this is a requirement. See Documentation/SubmittingPatches to read what signing off actually means here. Stefan ``` ## Jonathan Nieder, 2013-10-08 20:02 Subject: Re: [PATCH] git-credential-netrc: fix uninitialized warning Message-ID: <20131008200235.GG9464@google.com> URL: https://gitlist.dev/e/20131008200235.GG9464%40google.com In-Reply-To: <87li23v8p5.fsf@flea.lifelogs.com> ``` Ted Zlatanov wrote: > On Tue, 8 Oct 2013 12:41:47 -0700 Jonathan Nieder wrote: > JN> Ted Zlatanov wrote: >>> Simple patch to avoid unitialized warning and log what we'll do. > JN> Sign-off? > > I didn't realize it was a requirement, must I? See Documentation/SubmittingPatches, section '(5) Sign your work' for what this means. If you just forgot to sign off, that's fine and I can forge it or go without. If you are unable to sign off because you don't have the right to submit the change under an open source license, I'd be a bit worried going forward. [...] > JN> Or more simply, would it make sense to wrap both 'defined' checks into > JN> a single "if", like so? > > JN> if (defined $entry->{$check} && defined $query->{$check}) { > JN> ... > JN> } else { > JN> log_debug(...); > JN> } > > I prefer the explicit version because we can issue a more precise > log_debug message. That's fine with me. After this patch, the code looks like if (!defined $entry->{$check}) { log_debug(...); } elsif (defined $query->{$check}) { ... } else { log_debug(...); } As a small nit, wouldn't it be more readable with the two !defined() cases together? if (!defined $entry->{$check}) { ... } elsif (!defined $query->{$check}) { ... } else { ... } Thanks again. Jonathan ``` ## Ted Zlatanov, 2013-10-08 20:04 Subject: Re: [PATCH] git-credential-netrc: fix uninitialized warning Message-ID: <87a9ijv8ax.fsf@flea.lifelogs.com> URL: https://gitlist.dev/e/87a9ijv8ax.fsf%40flea.lifelogs.com In-Reply-To: <525463F1.2050308@googlemail.com> ``` On Tue, 08 Oct 2013 21:58:41 +0200 Stefan Beller wrote: SB> On 10/08/2013 09:55 PM, Ted Zlatanov wrote: JN> Sign-off? >> >> I didn't realize it was a requirement, must I? SB> Yes, this is a requirement. See Documentation/SubmittingPatches SB> to read what signing off actually means here. OK, I didn't realize it was. Thanks for explaining. Ted ``` ## Ted Zlatanov, 2013-10-08 20:12 Subject: Re: [PATCH] git-credential-netrc: fix uninitialized warning Message-ID: <8761t7v7w7.fsf@flea.lifelogs.com> URL: https://gitlist.dev/e/8761t7v7w7.fsf%40flea.lifelogs.com In-Reply-To: <20131008200235.GG9464@google.com> ``` On Tue, 8 Oct 2013 13:02:35 -0700 Jonathan Nieder wrote: JN> Ted Zlatanov wrote: >> On Tue, 8 Oct 2013 12:41:47 -0700 Jonathan Nieder wrote: JN> Ted Zlatanov wrote: >>>> Simple patch to avoid unitialized warning and log what we'll do. JN> Sign-off? >> >> I didn't realize it was a requirement, must I? JN> See Documentation/SubmittingPatches, section '(5) Sign your work' JN> for what this means. JN> If you just forgot to sign off, that's fine and I can forge it or go JN> without. If you are unable to sign off because you don't have the JN> right to submit the change under an open source license, I'd be a bit JN> worried going forward. Right, I got it. Sorry, I didn't know that applied to trivial patches. JN> After this patch, the code looks like JN> if (!defined $entry->{$check}) { JN> log_debug(...); JN> } elsif (defined $query->{$check}) { JN> ... JN> } else { JN> log_debug(...); JN> } JN> As a small nit, wouldn't it be more readable with the two !defined() JN> cases together? JN> if (!defined $entry->{$check}) { JN> ... JN> } elsif (!defined $query->{$check}) { JN> ... JN> } else { JN> ... JN> } Because of way the "next ENTRY" line comes out, I like my way slightly better, but honestly it's fine either way :) If you like, go ahead and commit the rewrite the way it works for you, I have no objections at all. Ted ```