[PATCH 1/2] blame: factor out get_next_line()

[PATCH 1/2] blame: factor out get_next_line()

[René Scharfe]

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 <l.s.r@web.de>
---
 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

[PATCH 2/2] blame: simplify prepare_lines()

[René Scharfe]

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 <l.s.r@web.de>
---
 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

Re: [PATCH 1/2] blame: factor out get_next_line()

[Jonathan Nieder]

René Scharfe wrote:

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/blame.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]
> +	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

Re: [PATCH 2/2] blame: simplify prepare_lines()

[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.)

Thanks,
Jonathan

Re: [PATCH 2/2] blame: simplify prepare_lines()

[René Scharfe]

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é

Re: [PATCH 2/2] blame: simplify prepare_lines()

[Jonathan Nieder]

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 <jrnieder@gmail.com>