# [PATCH 1/2] blame: factor out get_next_line() 6 messages from 2014-06-13 to 2014-06-13. Participants: René Scharfe, Jonathan Nieder. Thread: https://gitlist.dev/t/36911 ## René Scharfe, 2014-06-13 19:53 Subject: [PATCH 1/2] blame: factor out get_next_line() Message-ID: <539B569F.1090800@web.de> URL: https://gitlist.dev/e/539B569F.1090800%40web.de ``` Move the code for finding the start of the next line into a helper function in order to reduce duplication. Signed-off-by: Rene Scharfe --- builtin/blame.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index a52a279..ad37edc 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2008,6 +2008,12 @@ static void output(struct scoreboard *sb, int option) } } +static const char *get_next_line(const char *start, const char *end) +{ + const char *nl = memchr(start, '\n', end - start); + return nl ? nl + 1 : NULL; +} + /* * To allow quick access to the contents of nth line in the * final image, prepare an index in the scoreboard. @@ -2021,15 +2027,8 @@ static int prepare_lines(struct scoreboard *sb) int *lineno; int num = 0, incomplete = 0; - for (p = buf;;) { - p = memchr(p, '\n', end - p); - if (p) { - p++; - num++; - continue; - } - break; - } + for (p = get_next_line(buf, end); p; p = get_next_line(p, end)) + num++; if (len && end[-1] != '\n') incomplete++; /* incomplete line at the end */ @@ -2038,15 +2037,8 @@ static int prepare_lines(struct scoreboard *sb) lineno = sb->lineno; *lineno++ = 0; - for (p = buf;;) { - p = memchr(p, '\n', end - p); - if (p) { - p++; - *lineno++ = p - buf; - continue; - } - break; - } + for (p = get_next_line(buf, end); p; p = get_next_line(p, end)) + *lineno++ = p - buf; if (incomplete) *lineno++ = len; -- 2.0.0 ``` ## René Scharfe, 2014-06-13 19:54 Subject: [PATCH 2/2] blame: simplify prepare_lines() Message-ID: <539B5713.10300@web.de> URL: https://gitlist.dev/e/539B5713.10300%40web.de In-Reply-To: <539B569F.1090800@web.de> ``` Changing get_next_line() to return the end pointer instead of NULL in case no newline character is found treats allows us to treat complete and incomplete lines the same, simplifying the code. Switching to counting lines instead of EOLs allows us to start counting at the first character, instead of having to call get_next_line() first. Signed-off-by: Rene Scharfe --- builtin/blame.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index ad37edc..662e3fe 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2011,7 +2011,7 @@ static void output(struct scoreboard *sb, int option) static const char *get_next_line(const char *start, const char *end) { const char *nl = memchr(start, '\n', end - start); - return nl ? nl + 1 : NULL; + return nl ? nl + 1 : end; } /* @@ -2025,25 +2025,19 @@ static int prepare_lines(struct scoreboard *sb) const char *end = buf + len; const char *p; int *lineno; - int num = 0, incomplete = 0; + int num = 0; - for (p = get_next_line(buf, end); p; p = get_next_line(p, end)) + for (p = buf; p < end; p = get_next_line(p, end)) num++; - if (len && end[-1] != '\n') - incomplete++; /* incomplete line at the end */ + sb->lineno = lineno = xmalloc(sizeof(*sb->lineno) * (num + 1)); - sb->lineno = xmalloc(sizeof(*sb->lineno) * (num + incomplete + 1)); - lineno = sb->lineno; - - *lineno++ = 0; - for (p = get_next_line(buf, end); p; p = get_next_line(p, end)) + for (p = buf; p < end; p = get_next_line(p, end)) *lineno++ = p - buf; - if (incomplete) - *lineno++ = len; + *lineno = len; - sb->num_lines = num + incomplete; + sb->num_lines = num; return sb->num_lines; } -- 2.0.0 ``` ## Jonathan Nieder, 2014-06-13 21:05 Subject: Re: [PATCH 1/2] blame: factor out get_next_line() Message-ID: <20140613210542.GK8557@google.com> URL: https://gitlist.dev/e/20140613210542.GK8557%40google.com In-Reply-To: <539B569F.1090800@web.de> ``` René Scharfe wrote: > Signed-off-by: Rene Scharfe > --- > builtin/blame.c | 28 ++++++++++------------------ > 1 file changed, 10 insertions(+), 18 deletions(-) Reviewed-by: Jonathan Nieder [...] > + for (p = get_next_line(buf, end); p; p = get_next_line(p, end)) > + num++; This could say something like for (p = buf; p = get_next_line(buf, end); ) { num++; or p = buf; while ((p = memchr(p, '\n', end - p))) { p++; num++; } but what you have seems more readable. Thanks, Jonathan ``` ## Jonathan Nieder, 2014-06-13 21:13 Subject: Re: [PATCH 2/2] blame: simplify prepare_lines() Message-ID: <20140613211339.GL8557@google.com> URL: https://gitlist.dev/e/20140613211339.GL8557%40google.com In-Reply-To: <539B5713.10300@web.de> ``` René Scharfe wrote: > - if (incomplete) > - *lineno++ = len; > + *lineno = len; > > - sb->num_lines = num + incomplete; > + sb->num_lines = num; This will always treat whatever comes after the last newline as an incomplete line, even if it has zero length. Is that safe? (Not a rhetorical question --- I haven't looked carefully at the caller.) Thanks, Jonathan ``` ## René Scharfe, 2014-06-13 21:33 Subject: Re: [PATCH 2/2] blame: simplify prepare_lines() Message-ID: <539B6E31.1080901@web.de> URL: https://gitlist.dev/e/539B6E31.1080901%40web.de In-Reply-To: <20140613211339.GL8557@google.com> ``` Am 13.06.2014 23:13, schrieb Jonathan Nieder: > René Scharfe wrote: > >> - if (incomplete) >> - *lineno++ = len; >> + *lineno = len; >> >> - sb->num_lines = num + incomplete; >> + sb->num_lines = num; > > This will always treat whatever comes after the last newline as an > incomplete line, even if it has zero length. Is that safe? (Not a > rhetorical question --- I haven't looked carefully at the caller.) There is no need to look at the caller -- the contents of the lineno array is not (intended to be) changed by the patch. The original code is: p = memchr(p, '\n', end - p); if (p) { p++; *lineno++ = p - buf; continue; } Suppose there is no incomplete line. For the last EOL of a buffer, p points to buf[len - 1] after the memchr call. Then it is incremented. Then buf + len - buf is written into the last lineno member -- same as after the patch. Makes sense? René ``` ## Jonathan Nieder, 2014-06-13 21:46 Subject: Re: [PATCH 2/2] blame: simplify prepare_lines() Message-ID: <20140613214642.GM8557@google.com> URL: https://gitlist.dev/e/20140613214642.GM8557%40google.com In-Reply-To: <539B6E31.1080901@web.de> ``` Hi, René Scharfe wrote: > There is no need to look at the caller -- the contents of the lineno > array is not (intended to be) changed by the patch. Ah. If there is no incomplete line, the 'p < end' condition trips and it doesn't try to record the nonexistent incomplete line. Thanks for explaining and sorry for the confusion. Reviewed-by: Jonathan Nieder ```