{"thread":{"id":"38467","subject":"[PATCH 0/2] silence clang-3.6 warnings","startedAt":"2015-01-28T17:53:14Z","lastAt":"2015-01-29T06:22:53Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"255370","messageId":"20150128175314.GA13362@peff.net","threadId":"38467","inReplyTo":null,"subject":"[PATCH 0/2] silence clang-3.6 warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-28T17:53:14Z","receivedAt":"2015-01-28T17:53:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was fooling around with clang-3.6 today (as opposed to 3.5, which is\nthe stock \"clang\" shipped with Debian), and noticed two new warnings.\nThe first one is a real bug, and the second is just a cleanup (though I\nagree with clang on it).\n\n [1/2]: read_and_strip_branch: fix typo'd address-of operator\n [2/2]: do not check truth value of flex arrays\n\n-Peff\n"},{"id":"255373","messageId":"20150128175735.GA8172@peff.net","threadId":"38467","inReplyTo":"20150128175314.GA13362@peff.net","subject":"[PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-28T17:57:35Z","receivedAt":"2015-01-28T17:57:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are chomping newlines from the end of a strbuf, we\nmust check \"sb.len != 0\" before accessing \"sb.buf[sb.len - 1]\".\nHowever, this code mistakenly checks \"&sb.len\", which is\nalways true (it is a part of an auto struct, so the address\nis always non-zero). This could lead to us accessing memory\noutside the strbuf when we read an empty file.\n\nSigned-off-by: Jeff King \n---\nThis dates back to 8b87cfd (wt-status: move strbuf into\nread_and_strip_branch(), 2013-03-16), so it is not a bug that needs\naddressed during the -rc period.\n\nThis is the most minimal fix, but I kind of wonder if it should just be\nusing strbuf_rtrim (or even strbuf_trim) in the first place.\n\n wt-status.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex b54eac5..29666d0 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1140,7 +1140,7 @@ static char *read_and_strip_branch(const char *path)\n \tif (strbuf_read_file(&sb, git_path(\"%s\", path), 0) <= 0)\n \t\tgoto got_nothing;\n \n-\twhile (&sb.len && sb.buf[sb.len - 1] == '\\n')\n+\twhile (sb.len && sb.buf[sb.len - 1] == '\\n')\n \t\tstrbuf_setlen(&sb, sb.len - 1);\n \tif (!sb.len)\n \t\tgoto got_nothing;\n-- \n2.3.0.rc1.287.g761fd19\n"},{"id":"255376","messageId":"20150128175850.GB8172@peff.net","threadId":"38467","inReplyTo":"20150128175314.GA13362@peff.net","subject":"[PATCH 2/2] do not check truth value of flex arrays","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-28T17:58:50Z","receivedAt":"2015-01-28T17:58:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is no point in checking \"!ref->name\" when ref is a\n\"struct ref\". The name field is a flex-array, and there\nalways has a non-zero address. This is almost certainly not\nhurting anything, but it does cause clang-3.6 to complain.\n\nSigned-off-by: Jeff King \n---\nNote that even if \"ref\" is NULL, this is still going to be\n\"0+offsetof(name)\". So short of unsigned pointer wrap-around, I do not\nthink this check can ever do anything.\n\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex dd63bc2..515ac9b 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -760,7 +760,7 @@ static int fetch_git(struct discovery *heads,\n \n \tfor (i = 0; i < nr_heads; i++) {\n \t\tstruct ref *ref = to_fetch[i];\n-\t\tif (!ref->name || !*ref->name)\n+\t\tif (!*ref->name)\n \t\t\tdie(\"cannot fetch by sha1 over smart http\");\n \t\tpacket_buf_write(&preamble, \"%s %s\\n\",\n \t\t\t\t sha1_to_hex(ref->old_sha1), ref->name);\n-- \n2.3.0.rc1.287.g761fd19\n"},{"id":"255372","messageId":"xmqq1tmehc25.fsf@gitster.dls.corp.google.com","threadId":"38467","inReplyTo":"20150128175735.GA8172@peff.net","subject":"Re: [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-28T20:42:26Z","receivedAt":"2015-01-28T20:42:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we are chomping newlines from the end of a strbuf, we\n> must check \"sb.len != 0\" before accessing \"sb.buf[sb.len - 1]\".\n> However, this code mistakenly checks \"&sb.len\", which is\n> always true (it is a part of an auto struct, so the address\n> is always non-zero). This could lead to us accessing memory\n> outside the strbuf when we read an empty file.\n>\n> Signed-off-by: Jeff King \n> ---\n> This dates back to 8b87cfd (wt-status: move strbuf into\n> read_and_strip_branch(), 2013-03-16), so it is not a bug that needs\n> addressed during the -rc period.\n>\n> This is the most minimal fix, but I kind of wonder if it should just be\n> using strbuf_rtrim (or even strbuf_trim) in the first place.\n\nYeah. Or strbuf_chomp(), which does not exist ;-)\n\nIt is tempting to apply this directly to maint and merge up\nimmediately, as there is no way this 1-byte change will break things\n(of course that is not necessarily true for random 1-byte changes,\nthough).\n\nIt sometimes gets really hard to resist that temptation during the\npre-release freeze period.\n\n> wt-status.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index b54eac5..29666d0 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1140,7 +1140,7 @@ static char *read_and_strip_branch(const char *path)\n> \tif (strbuf_read_file(&sb, git_path(\"%s\", path), 0) <= 0)\n> \t\tgoto got_nothing;\n> \n> -\twhile (&sb.len && sb.buf[sb.len - 1] == '\\n')\n> +\twhile (sb.len && sb.buf[sb.len - 1] == '\\n')\n> \t\tstrbuf_setlen(&sb, sb.len - 1);\n> \tif (!sb.len)\n> \t\tgoto got_nothing;\n"},{"id":"255393","messageId":"20150128225755.GA15719@peff.net","threadId":"38467","inReplyTo":"xmqq1tmehc25.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-28T22:57:56Z","receivedAt":"2015-01-28T22:57:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2015 at 12:42:26PM -0800, Junio C Hamano wrote:\n\n> > This is the most minimal fix, but I kind of wonder if it should just be\n> > using strbuf_rtrim (or even strbuf_trim) in the first place.\n> \n> Yeah. Or strbuf_chomp(), which does not exist ;-)\n\nThis is not the first time I've seen this chomp/trim distinction come\nup. However, the thing that has prevented me from writing strbuf_chomp\nis that the trim is almost always a more reasonable choice.\n\nTake this instance. We are opening and reading a whole file. Surely we\nneed to drop the final newline, which is not interesting. But we are not\njust doing that; we are dropping _all_ trailing newlines. So \"foo\\n\\n\"\nbecomes \"foo\". But \"foo\\n \\n\" does not. That doesn't make much sense.\n\nIOW, I would venture to say that chomping like this falls into one of\ntwo categories:\n\n 1. You want to clean up any extraneous cruft. Multiple lines, extra\n whitespace, etc.\n\n 2. You want to read one line, but don't want the trailing newline.\n\nAnd strbuf_getline already handles case (2).\n\nEnd mini-rant. :)\n\n> It is tempting to apply this directly to maint and merge up\n> immediately, as there is no way this 1-byte change will break things\n> (of course that is not necessarily true for random 1-byte changes,\n> though).\n> \n> It sometimes gets really hard to resist that temptation during the\n> pre-release freeze period.\n\nThat's part of why I did the simplest fix instead of strbuf_rtrim. To\ntempt you. :)\n\n-Peff\n"},{"id":"255399","messageId":"xmqqy4omds1u.fsf@gitster.dls.corp.google.com","threadId":"38467","inReplyTo":"20150128225755.GA15719@peff.net","subject":"Re: [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-29T06:22:53Z","receivedAt":"2015-01-29T06:22:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is not the first time I've seen this chomp/trim distinction come\n> up. However, the thing that has prevented me from writing strbuf_chomp\n> is that the trim is almost always a more reasonable choice.\n> ...\n> End mini-rant. :)\n\nThanks.\n"}]}