Re: [PATCH] git-credential-netrc: fix uninitialized warning
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-credential-netrc: fix uninitialized warning
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-credential-netrc: fix uninitialized warning
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-credential-netrc: fix uninitialized warning
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH] git-credential-netrc: fix uninitialized warning
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-credential-netrc: fix uninitialized warning
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-credential-netrc: fix uninitialized warning
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html