{"thread":{"id":"36805","subject":"[PATCH] mailinfo: use strcmp() for string comparison","startedAt":"2014-06-01T09:00:40Z","lastAt":"2014-06-02T19:34:19Z","messageCount":2,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243091","messageId":"538AEBB8.9070505@web.de","threadId":"36805","inReplyTo":null,"subject":"[PATCH] mailinfo: use strcmp() for string comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-01T09:00:40Z","receivedAt":"2014-06-01T09:00:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The array header is defined as:\n\n\tstatic const char *header[MAX_HDR_PARSED] = {\n\t     \"From\",\"Subject\",\"Date\",\n\t};\n\nWhen looking for the index of a specfic string in that array, simply\nuse strcmp() instead of memcmp().  This avoids running over the end of\nthe string (e.g. with memcmp(\"Subject\", \"From\", 7)) and gets rid of\nmagic string length constants.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nThis is a minimal fix.  A good question, however, would be: Why do we\nkeep on looking up constant strings in a (short) constant string array\nanyway?\n\n builtin/mailinfo.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex 2c3cd8e..cf11c8d 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -334,7 +334,7 @@ static int check_header(const struct strbuf *line,\n \t}\n \tif (starts_with(line->buf, \"[PATCH]\") && isspace(line->buf[7])) {\n \t\tfor (i = 0; header[i]; i++) {\n-\t\t\tif (!memcmp(\"Subject\", header[i], 7)) {\n+\t\t\tif (!strcmp(\"Subject\", header[i])) {\n \t\t\t\thandle_header(&hdr_data[i], line);\n \t\t\t\tret = 1;\n \t\t\t\tgoto check_header_out;\n@@ -929,13 +929,13 @@ static void handle_info(void)\n \t\telse\n \t\t\tcontinue;\n \n-\t\tif (!memcmp(header[i], \"Subject\", 7)) {\n+\t\tif (!strcmp(header[i], \"Subject\")) {\n \t\t\tif (!keep_subject) {\n \t\t\t\tcleanup_subject(hdr);\n \t\t\t\tcleanup_space(hdr);\n \t\t\t}\n \t\t\toutput_header_lines(fout, \"Subject\", hdr);\n-\t\t} else if (!memcmp(header[i], \"From\", 4)) {\n+\t\t} else if (!strcmp(header[i], \"From\")) {\n \t\t\tcleanup_space(hdr);\n \t\t\thandle_from(hdr);\n \t\t\tfprintf(fout, \"Author: %s\\n\", name.buf);\n-- \n2.0.0\n"},{"id":"243149","messageId":"20140602193419.GC2510@sigill.intra.peff.net","threadId":"36805","inReplyTo":"538AEBB8.9070505@web.de","subject":"Re: [PATCH] mailinfo: use strcmp() for string comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-02T19:34:19Z","receivedAt":"2014-06-02T19:34:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 01, 2014 at 11:00:40AM +0200, René Scharfe wrote:\n\n> The array header is defined as:\n> \n> \tstatic const char *header[MAX_HDR_PARSED] = {\n> \t     \"From\",\"Subject\",\"Date\",\n> \t};\n> \n> When looking for the index of a specfic string in that array, simply\n> use strcmp() instead of memcmp().  This avoids running over the end of\n> the string (e.g. with memcmp(\"Subject\", \"From\", 7)) and gets rid of\n> magic string length constants.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n\nLooks correct to me.\n\n> ---\n> This is a minimal fix.  A good question, however, would be: Why do we\n> keep on looking up constant strings in a (short) constant string array\n> anyway?\n\nYeah, this code is quite confusing. I suspect it would be more readable\nto unroll any loops over the header array into a series of function\ncalls or even just cascading if/else. Some of the sites (e.g.,\ncheck_header) already have a mix, like:\n\n  for (i = 0; header[i]; i++)\n    if (cmp_header(line, header[i]))\n      ... do something for this header ...\n\n  if (cmp_header(line, \"some-other-header\"))\n      ... do something special for this header type ...\n  else if (cmp_header(line, \"another\"))\n      ... and something else again ...\n\nThe looping is not really helping much there in the first place, since\nit is not dealing with half of the headers. And then adding on top that\nthe loop has its own special cases found by comparing the string to\n\"Subject\", I think it would be simpler to just unroll it.\n\nThat's just from a quick 5-minute read of the code, though. This isn't\nan area I'm very familiar with, so maybe the refactor would get ugly.\n\n-Peff\n"}]}