{"thread":{"id":"32153","subject":"[PATCH] git-send-email: don't return undefined value in extract_valid_address()","startedAt":"2012-11-20T12:20:53Z","lastAt":"2012-11-20T22:14:05Z","messageCount":4,"participants":["Krzysztof Mazur","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"203557","messageId":"1353414053-25261-1-git-send-email-krzysiek@podlesie.net","threadId":"32153","inReplyTo":null,"subject":"[PATCH] git-send-email: don't return undefined value in extract_valid_address()","fromName":"Krzysztof Mazur","fromEmail":"krzysiek@podlesie.net","sentAt":"2012-11-20T12:20:53Z","receivedAt":"2012-11-20T12:20:53Z","isPatch":true,"sender":{"key":"krzysiek@podlesie.net","avatar":null},"body":"In the fallback check, when Email::Valid is not available, the\nextract_valid_address() does not check for success of matching regex,\nand $1, which can be undefined, is always returned. Now if match\nfails an empty string is returned.\n\nSigned-off-by: Krzysztof Mazur \n---\nThis fixes following warnings:\nUse of uninitialized value in string eq at ./git-send-email.perl line 1017.\nUse of uninitialized value in quotemeta at ./git-send-email.perl line 1017.\nW: unable to extract a valid address from: x a.patch\n\nwhen invalid email address was added by --cc-cmd,\n./git-send-email.perl --dry-run --to a@podlesie.net --cc-cmd=echo x a.patch\n\n git-send-email.perl | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5a7c29d..045f25f 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -831,12 +831,12 @@ sub extract_valid_address {\n \t$address =~ s/^\\s*<(.*)>\\s*$/$1/;\n \tif ($have_email_valid) {\n \t\treturn scalar Email::Valid->address($address);\n-\t} else {\n-\t\t# less robust/correct than the monster regexp in Email::Valid,\n-\t\t# but still does a 99% job, and one less dependency\n-\t\t$address =~ /($local_part_regexp\\@$domain_regexp)/;\n-\t\treturn $1;\n \t}\n+\n+\t# less robust/correct than the monster regexp in Email::Valid,\n+\t# but still does a 99% job, and one less dependency\n+\treturn $1 if $address =~ /($local_part_regexp\\@$domain_regexp)/;\n+\treturn \"\";\n }\n \n # Usually don't need to change anything below here.\n-- \n1.8.0.283.gc57d856\n"},{"id":"203569","messageId":"7v8v9wrpdz.fsf@alter.siamese.dyndns.org","threadId":"32153","inReplyTo":"1353414053-25261-1-git-send-email-krzysiek@podlesie.net","subject":"Re: [PATCH] git-send-email: don't return undefined value in extract_valid_address()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T20:27:36Z","receivedAt":"2012-11-20T20:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzysztof Mazur writes:\n\n> In the fallback check, when Email::Valid is not available, the\n> extract_valid_address() does not check for success of matching regex,\n> and $1, which can be undefined, is always returned. Now if match\n> fails an empty string is returned.\n\nThat much we can read from the code, but a bigger question is why\nwould it be a good thing for the callers? Wouldn't they want to\nbe able to distinguish a failure from an empty string?\n\n> Signed-off-by: Krzysztof Mazur \n> ---\n> This fixes following warnings:\n> Use of uninitialized value in string eq at ./git-send-email.perl line 1017.\n> Use of uninitialized value in quotemeta at ./git-send-email.perl line 1017.\n> W: unable to extract a valid address from: x a.patch\n>\n> when invalid email address was added by --cc-cmd,\n> ./git-send-email.perl --dry-run --to a@podlesie.net --cc-cmd=echo x a.patch\n\nIn other words, would we want to *hide* (not \"fix\") the warning?\nShouldn't we be barfing loudly and possibly erroring it out until\nthe user fixes her --cc-cmd?\n\n> git-send-email.perl | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 5a7c29d..045f25f 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -831,12 +831,12 @@ sub extract_valid_address {\n> \t$address =~ s/^\\s*<(.*)>\\s*$/$1/;\n> \tif ($have_email_valid) {\n> \t\treturn scalar Email::Valid->address($address);\n> -\t} else {\n> -\t\t# less robust/correct than the monster regexp in Email::Valid,\n> -\t\t# but still does a 99% job, and one less dependency\n> -\t\t$address =~ /($local_part_regexp\\@$domain_regexp)/;\n> -\t\treturn $1;\n> \t}\n> +\n> +\t# less robust/correct than the monster regexp in Email::Valid,\n> +\t# but still does a 99% job, and one less dependency\n> +\treturn $1 if $address =~ /($local_part_regexp\\@$domain_regexp)/;\n> +\treturn \"\";\n> }\n> \n> # Usually don't need to change anything below here.\n"},{"id":"203571","messageId":"20121120204736.GA7039@shrek.podlesie.net","threadId":"32153","inReplyTo":"7v8v9wrpdz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-send-email: don't return undefined value in extract_valid_address()","fromName":"Krzysztof Mazur","fromEmail":"krzysiek@podlesie.net","sentAt":"2012-11-20T20:47:36Z","receivedAt":"2012-11-20T20:47:36Z","isPatch":true,"sender":{"key":"krzysiek@podlesie.net","avatar":null},"body":"On Tue, Nov 20, 2012 at 12:27:36PM -0800, Junio C Hamano wrote:\n> Krzysztof Mazur writes:\n> \n> > In the fallback check, when Email::Valid is not available, the\n> > extract_valid_address() does not check for success of matching regex,\n> > and $1, which can be undefined, is always returned. Now if match\n> > fails an empty string is returned.\n\nMaybe the last line of comment should be changed to:\n\nfails an empty string is returned to indicate failure.\n\n> \n> That much we can read from the code, but a bigger question is why\n> would it be a good thing for the callers? Wouldn't they want to\n> be able to distinguish a failure from an empty string?\n\nIn this case returning empty string does not make sense, so it's\nreally used to indicate failure.\n\n> \n> > Signed-off-by: Krzysztof Mazur \n> > ---\n> > This fixes following warnings:\n> > Use of uninitialized value in string eq at ./git-send-email.perl line 1017.\n> > Use of uninitialized value in quotemeta at ./git-send-email.perl line 1017.\n> > W: unable to extract a valid address from: x a.patch\n> >\n> > when invalid email address was added by --cc-cmd,\n> > ./git-send-email.perl --dry-run --to a@podlesie.net --cc-cmd=echo x a.patch\n> \n> In other words, would we want to *hide* (not \"fix\") the warning?\n> Shouldn't we be barfing loudly and possibly erroring it out until\n> the user fixes her --cc-cmd?\n> \n\nYes, it's just to hide the warning, the error (warning in this case) it's\nalready correctly generated:\n\nW: unable to extract a valid address from: x a.patch\n\nMaybe we should change it to an error?\n\nKrzysiek\n"},{"id":"203575","messageId":"7vvcczrkgi.fsf@alter.siamese.dyndns.org","threadId":"32153","inReplyTo":"20121120204736.GA7039@shrek.podlesie.net","subject":"Re: [PATCH] git-send-email: don't return undefined value in extract_valid_address()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T22:14:05Z","receivedAt":"2012-11-20T22:14:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzysztof Mazur writes:\n\n> Yes, it's just to hide the warning, the error (warning in this case) it's\n> already correctly generated:\n>\n> W: unable to extract a valid address from: x a.patch\n\nBut it is of no use if the message is sent out without the intended\nrecipient, no? It is too late when you notice it.\n\n> Maybe we should change it to an error?\n\nAt least, when we are not giving the \"final sanity check [Y/n]?\"\nprompt, I think the code should error out.\n"}]}