{"thread":{"id":"36911","subject":"[PATCH 1/2] blame: factor out get_next_line()","startedAt":"2014-06-13T19:53:03Z","lastAt":"2014-06-13T21:46:42Z","messageCount":6,"participants":["René Scharfe","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"244168","messageId":"539B569F.1090800@web.de","threadId":"36911","inReplyTo":null,"subject":"[PATCH 1/2] blame: factor out get_next_line()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-13T19:53:03Z","receivedAt":"2014-06-13T19:53:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the code for finding the start of the next line into a helper\nfunction in order to reduce duplication.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/blame.c | 28 ++++++++++------------------\n 1 file changed, 10 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex a52a279..ad37edc 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2008,6 +2008,12 @@ static void output(struct scoreboard *sb, int option)\n \t}\n }\n \n+static const char *get_next_line(const char *start, const char *end)\n+{\n+\tconst char *nl = memchr(start, '\\n', end - start);\n+\treturn nl ? nl + 1 : NULL;\n+}\n+\n /*\n * To allow quick access to the contents of nth line in the\n * final image, prepare an index in the scoreboard.\n@@ -2021,15 +2027,8 @@ static int prepare_lines(struct scoreboard *sb)\n \tint *lineno;\n \tint num = 0, incomplete = 0;\n \n-\tfor (p = buf;;) {\n-\t\tp = memchr(p, '\\n', end - p);\n-\t\tif (p) {\n-\t\t\tp++;\n-\t\t\tnum++;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tbreak;\n-\t}\n+\tfor (p = get_next_line(buf, end); p; p = get_next_line(p, end))\n+\t\tnum++;\n \n \tif (len && end[-1] != '\\n')\n \t\tincomplete++; /* incomplete line at the end */\n@@ -2038,15 +2037,8 @@ static int prepare_lines(struct scoreboard *sb)\n \tlineno = sb->lineno;\n \n \t*lineno++ = 0;\n-\tfor (p = buf;;) {\n-\t\tp = memchr(p, '\\n', end - p);\n-\t\tif (p) {\n-\t\t\tp++;\n-\t\t\t*lineno++ = p - buf;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tbreak;\n-\t}\n+\tfor (p = get_next_line(buf, end); p; p = get_next_line(p, end))\n+\t\t*lineno++ = p - buf;\n \n \tif (incomplete)\n \t\t*lineno++ = len;\n-- \n2.0.0\n"},{"id":"244169","messageId":"539B5713.10300@web.de","threadId":"36911","inReplyTo":"539B569F.1090800@web.de","subject":"[PATCH 2/2] blame: simplify prepare_lines()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-13T19:54:59Z","receivedAt":"2014-06-13T19:54:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changing get_next_line() to return the end pointer instead of NULL in\ncase no newline character is found treats allows us to treat complete\nand incomplete lines the same, simplifying the code. Switching to\ncounting lines instead of EOLs allows us to start counting at the\nfirst character, instead of having to call get_next_line() first.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/blame.c | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex ad37edc..662e3fe 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2011,7 +2011,7 @@ static void output(struct scoreboard *sb, int option)\n static const char *get_next_line(const char *start, const char *end)\n {\n \tconst char *nl = memchr(start, '\\n', end - start);\n-\treturn nl ? nl + 1 : NULL;\n+\treturn nl ? nl + 1 : end;\n }\n \n /*\n@@ -2025,25 +2025,19 @@ static int prepare_lines(struct scoreboard *sb)\n \tconst char *end = buf + len;\n \tconst char *p;\n \tint *lineno;\n-\tint num = 0, incomplete = 0;\n+\tint num = 0;\n \n-\tfor (p = get_next_line(buf, end); p; p = get_next_line(p, end))\n+\tfor (p = buf; p < end; p = get_next_line(p, end))\n \t\tnum++;\n \n-\tif (len && end[-1] != '\\n')\n-\t\tincomplete++; /* incomplete line at the end */\n+\tsb->lineno = lineno = xmalloc(sizeof(*sb->lineno) * (num + 1));\n \n-\tsb->lineno = xmalloc(sizeof(*sb->lineno) * (num + incomplete + 1));\n-\tlineno = sb->lineno;\n-\n-\t*lineno++ = 0;\n-\tfor (p = get_next_line(buf, end); p; p = get_next_line(p, end))\n+\tfor (p = buf; p < end; p = get_next_line(p, end))\n \t\t*lineno++ = p - buf;\n \n-\tif (incomplete)\n-\t\t*lineno++ = len;\n+\t*lineno = len;\n \n-\tsb->num_lines = num + incomplete;\n+\tsb->num_lines = num;\n \treturn sb->num_lines;\n }\n \n-- \n2.0.0\n"},{"id":"244171","messageId":"20140613210542.GK8557@google.com","threadId":"36911","inReplyTo":"539B569F.1090800@web.de","subject":"Re: [PATCH 1/2] blame: factor out get_next_line()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-13T21:05:42Z","receivedAt":"2014-06-13T21:05:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"René Scharfe wrote:\n\n> Signed-off-by: Rene Scharfe \n> ---\n> builtin/blame.c | 28 ++++++++++------------------\n> 1 file changed, 10 insertions(+), 18 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\n[...]\n> +\tfor (p = get_next_line(buf, end); p; p = get_next_line(p, end))\n> +\t\tnum++;\n\nThis could say something like\n\n\tfor (p = buf; p = get_next_line(buf, end); ) {\n\t\tnum++;\n\nor\n\n\tp = buf;\n\twhile ((p = memchr(p, '\\n', end - p))) {\n\t\tp++;\n\t\tnum++;\n\t}\n\nbut what you have seems more readable.\n\nThanks,\nJonathan\n"},{"id":"244174","messageId":"20140613211339.GL8557@google.com","threadId":"36911","inReplyTo":"539B5713.10300@web.de","subject":"Re: [PATCH 2/2] blame: simplify prepare_lines()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-13T21:13:39Z","receivedAt":"2014-06-13T21:13:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"René Scharfe wrote:\n\n> -\tif (incomplete)\n> -\t\t*lineno++ = len;\n> +\t*lineno = len;\n> \n> -\tsb->num_lines = num + incomplete;\n> +\tsb->num_lines = num;\n\nThis will always treat whatever comes after the last newline as an\nincomplete line, even if it has zero length. Is that safe? (Not a\nrhetorical question --- I haven't looked carefully at the caller.)\n\nThanks,\nJonathan\n"},{"id":"244176","messageId":"539B6E31.1080901@web.de","threadId":"36911","inReplyTo":"20140613211339.GL8557@google.com","subject":"Re: [PATCH 2/2] blame: simplify prepare_lines()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-13T21:33:37Z","receivedAt":"2014-06-13T21:33:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.06.2014 23:13, schrieb Jonathan Nieder:\n> René Scharfe wrote:\n>\n>> -\tif (incomplete)\n>> -\t\t*lineno++ = len;\n>> +\t*lineno = len;\n>>\n>> -\tsb->num_lines = num + incomplete;\n>> +\tsb->num_lines = num;\n>\n> This will always treat whatever comes after the last newline as an\n> incomplete line, even if it has zero length. Is that safe? (Not a\n> rhetorical question --- I haven't looked carefully at the caller.)\n\nThere is no need to look at the caller -- the contents of the lineno \narray is not (intended to be) changed by the patch.\n\nThe original code is:\n\n\tp = memchr(p, '\\n', end - p);\n\tif (p) {\n\t\tp++;\n\t\t*lineno++ = p - buf;\n\t\tcontinue;\n\t}\n\nSuppose there is no incomplete line. For the last EOL of a buffer, p \npoints to buf[len - 1] after the memchr call. Then it is incremented. \nThen buf + len - buf is written into the last lineno member -- same as \nafter the patch.\n\nMakes sense?\n\nRené\n"},{"id":"244178","messageId":"20140613214642.GM8557@google.com","threadId":"36911","inReplyTo":"539B6E31.1080901@web.de","subject":"Re: [PATCH 2/2] blame: simplify prepare_lines()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-13T21:46:42Z","receivedAt":"2014-06-13T21:46:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRené Scharfe wrote:\n\n> There is no need to look at the caller -- the contents of the lineno\n> array is not (intended to be) changed by the patch.\n\nAh. If there is no incomplete line, the 'p < end' condition trips and\nit doesn't try to record the nonexistent incomplete line. Thanks for\nexplaining and sorry for the confusion.\n\nReviewed-by: Jonathan Nieder \n"}]}