threads / patch / 35083

patchgit-credential-netrc: fix uninitialized warning

Subject: [PATCH] git-credential-netrc: fix uninitialized warning

## tl;dr

7 messages between Oct 8, 2013 and Oct 8, 2013. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Ted Zlatanov· Oct 8, 2013, 14:34 UTC · lore

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(-)

Show changes to contrib/credential/netrc/git-credential-netrc +4 −1

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· Oct 8, 2013, 19:41 UTC · re: Ted Zlatanov · lore

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?

[...]

Show 11 quoted lines

> --- 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· Oct 8, 2013, 19:55 UTC · re: Jonathan Nieder · lore

Re: [PATCH] git-credential-netrc: fix uninitialized warning

On Tue, 8 Oct 2013 12:41:47 -0700 Jonathan Nieder <jrnieder@gmail.com> 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> [...]

Show 11 quoted lines

>> --- 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· Oct 8, 2013, 19:58 UTC · re: Ted Zlatanov · lore

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

Ted Zlatanov· Oct 8, 2013, 20:04 UTC · re: Stefan Beller · lore

Re: [PATCH] git-credential-netrc: fix uninitialized warning

On Tue, 08 Oct 2013 21:58:41 +0200 Stefan Beller <stefanbeller@googlemail.com> 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

Jonathan Nieder· Oct 8, 2013, 20:02 UTC · re: Ted Zlatanov · lore

Re: [PATCH] git-credential-netrc: fix uninitialized warning

Ted Zlatanov wrote:

> On Tue, 8 Oct 2013 12:41:47 -0700 Jonathan Nieder <jrnieder@gmail.com> 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.

[...]

Show 11 quoted lines

> 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· Oct 8, 2013, 20:12 UTC · re: Jonathan Nieder · lore

Re: [PATCH] git-credential-netrc: fix uninitialized warning

On Tue, 8 Oct 2013 13:02:35 -0700 Jonathan Nieder <jrnieder@gmail.com> wrote:

JN> Ted Zlatanov wrote:

>> On Tue, 8 Oct 2013 12:41:47 -0700 Jonathan Nieder <jrnieder@gmail.com> 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