Fix hunk numbering and a leak in the ssdiff view

A hunk header that states no length was numbered from the wrong
line, and the line a deferred row copied was never freed.

2 files changed, 69 insertions, 18 deletions

diff --git a/source/ui-ssdiff.c b/source/ui-ssdiff.cindex 750dc3d..7cea09e 100644--- a/source/ui-ssdiff.c+++ b/source/ui-ssdiff.c@@ -91,22 +91,21 @@ static char *longest_common_subsequence(char *A, char *B)  static int line_from_hunk(char *line, char type) {-	char *buf1, *buf2;-	int len, res;+	char *p;+	long res; -	buf1 = strchr(line, type);-	if (buf1 == NULL)+	p = strchr(line, type);+	if (p == NULL) 		return 0;-	buf1 += 1;-	buf2 = strchr(buf1, ',');-	if (buf2 == NULL)+	p += 1;+	// git omits the length when a hunk covers a single line, as in+	// "@@ -1 +1 @@", so the number runs to whatever follows it rather than+	// to a comma that may belong to the other side of the header or be+	// missing altogether.+	res = strtol(p, NULL, 10);+	if (res < 0 || res > INT_MAX) 		return 0;-	len = buf2 - buf1;-	buf2 = xmalloc(len + 1);-	strlcpy(buf2, buf1, len + 1);-	res = atoi(buf2);-	free(buf2);-	return res;+	return (int)res; }  static char *replace_tabs(char *line)@@ -190,7 +189,24 @@ static void deferred_new_add(char *line, int line_no) 	} } -static void print_part_with_lcs(char *class, char *line, char *lcs)+/* The item owns the copy of the line taken when it was deferred, so both go+ * together. print_ssdiff_line only reads the line and frees what it derives+ * from it, and never keeps the pointer it was handed. */+static void free_deferred(struct deferred_lines *item)+{+	free(item->line);+	free(item);+}++static void flush_run(struct strbuf *run)+{+	if (!run->len)+		return;+	html_txt(run->buf);+	strbuf_reset(run);+}++static void print_part_with_lcs(const char *class, char *line, char *lcs) { 	int line_len = strlen(line); 	int i, j;@@ -309,7 +325,7 @@ static void print_deferred_old_lines(void) 		print_ssdiff_line("del", iter_old->line_no, 				  iter_old->line, -1, NULL, 0); 		tmp = iter_old->next;-		free(iter_old);+		free_deferred(iter_old); 		iter_old = tmp; 	} }@@ -322,7 +338,7 @@ static void print_deferred_new_lines(void) 		print_ssdiff_line("add", -1, NULL, 				  iter_new->line_no, iter_new->line, 0); 		tmp = iter_new->next;-		free(iter_new);+		free_deferred(iter_new); 		iter_new = tmp; 	} }@@ -350,13 +366,13 @@ static void print_deferred_changed_lines(void) 					  iter_new->line_no, iter_new->line, 0); 		if (iter_old) { 			tmp = iter_old->next;-			free(iter_old);+			free_deferred(iter_old); 			iter_old = tmp; 		}  		if (iter_new) { 			tmp = iter_new->next;-			free(iter_new);+			free_deferred(iter_new); 			iter_new = tmp; 		} 	}
diff --git a/source/ui-ssdiff.c b/source/ui-ssdiff.c
index 750dc3d..7cea09e 100644
--- a/source/ui-ssdiff.c
+++ b/source/ui-ssdiff.c
@@ -91,22 +91,21 @@ static char *longest_common_subsequence(char *A, char *B)
 
 static int line_from_hunk(char *line, char type)
 {
-	char *buf1, *buf2;
-	int len, res;
+	char *p;
+	long res;
 
-	buf1 = strchr(line, type);
-	if (buf1 == NULL)
+	p = strchr(line, type);
+	if (p == NULL)
 		return 0;
-	buf1 += 1;
-	buf2 = strchr(buf1, ',');
-	if (buf2 == NULL)
+	p += 1;
+	// git omits the length when a hunk covers a single line, as in
+	// "@@ -1 +1 @@", so the number runs to whatever follows it rather than
+	// to a comma that may belong to the other side of the header or be
+	// missing altogether.
+	res = strtol(p, NULL, 10);
+	if (res < 0 || res > INT_MAX)
 		return 0;
-	len = buf2 - buf1;
-	buf2 = xmalloc(len + 1);
-	strlcpy(buf2, buf1, len + 1);
-	res = atoi(buf2);
-	free(buf2);
-	return res;
+	return (int)res;
 }
 
 static char *replace_tabs(char *line)
@@ -190,7 +189,24 @@ static void deferred_new_add(char *line, int line_no)
 	}
 }
 
-static void print_part_with_lcs(char *class, char *line, char *lcs)
+/* The item owns the copy of the line taken when it was deferred, so both go
+ * together. print_ssdiff_line only reads the line and frees what it derives
+ * from it, and never keeps the pointer it was handed. */
+static void free_deferred(struct deferred_lines *item)
+{
+	free(item->line);
+	free(item);
+}
+
+static void flush_run(struct strbuf *run)
+{
+	if (!run->len)
+		return;
+	html_txt(run->buf);
+	strbuf_reset(run);
+}
+
+static void print_part_with_lcs(const char *class, char *line, char *lcs)
 {
 	int line_len = strlen(line);
 	int i, j;
@@ -309,7 +325,7 @@ static void print_deferred_old_lines(void)
 		print_ssdiff_line("del", iter_old->line_no,
 				  iter_old->line, -1, NULL, 0);
 		tmp = iter_old->next;
-		free(iter_old);
+		free_deferred(iter_old);
 		iter_old = tmp;
 	}
 }
@@ -322,7 +338,7 @@ static void print_deferred_new_lines(void)
 		print_ssdiff_line("add", -1, NULL,
 				  iter_new->line_no, iter_new->line, 0);
 		tmp = iter_new->next;
-		free(iter_new);
+		free_deferred(iter_new);
 		iter_new = tmp;
 	}
 }
@@ -350,13 +366,13 @@ static void print_deferred_changed_lines(void)
 					  iter_new->line_no, iter_new->line, 0);
 		if (iter_old) {
 			tmp = iter_old->next;
-			free(iter_old);
+			free_deferred(iter_old);
 			iter_old = tmp;
 		}
 
 		if (iter_new) {
 			tmp = iter_new->next;
-			free(iter_new);
+			free_deferred(iter_new);
 			iter_new = tmp;
 		}
 	}
diff --git a/tests/t0106-diff.sh b/tests/t0106-diff.shindex 82b645e..3e0d15c 100755--- a/tests/t0106-diff.sh+++ b/tests/t0106-diff.sh@@ -16,4 +16,39 @@ test_expect_success 'find added line' ' 	grep "<div class=.add.>+5</div>" tmp ' +# --- Side-by-side line numbers for a hunk that covers a single line ---------+# git writes the length only when a hunk spans more than one line, so a header+# reads "@@ -1 +1 @@". Requiring a comma after the number left those hunks+# numbered from zero, which dropped the line-number link entirely.+test_expect_success 'set up a repo with single-line hunks' '+	mkrepo repos/hunk1 1 &&+	(+		cd repos/hunk1 &&+		printf "only\n" >one.txt &&+		git add one.txt &&+		git commit -m "add one.txt" &&+		printf "ONLY\n" >one.txt &&+		git commit -am "change one.txt"+	) &&+	{+		echo "virtual-root=/" &&+		echo "cache-size=0" &&+		echo "repo.url=hunk1" &&+		echo "repo.path=$PWD/repos/hunk1/.git"+	} >hunk1rc &&+	git -C repos/hunk1 diff HEAD~1 HEAD >raw.diff &&+	grep "^@@ -1 +1 @@" raw.diff+'++test_expect_success 'ssdiff numbers a comma-less hunk from its real line' '+	CGIT_CONFIG="$PWD/hunk1rc" QUERY_STRING="url=hunk1/diff/&dt=1" cgit >tmp &&+	grep "#n1.>1</a>" tmp &&+	! grep "#n0.>0</a>" tmp+'++test_expect_success 'ssdiff still numbers a hunk that carries a length' '+	cgit_query "url=foo/diff&dt=1" >tmp &&+	grep "#n1.>1</a>" tmp+'+ test_done
diff --git a/tests/t0106-diff.sh b/tests/t0106-diff.sh
index 82b645e..3e0d15c 100755
--- a/tests/t0106-diff.sh
+++ b/tests/t0106-diff.sh
@@ -16,4 +16,39 @@ test_expect_success 'find added line' '
 	grep "<div class=.add.>+5</div>" tmp
 '
 
+# --- Side-by-side line numbers for a hunk that covers a single line ---------
+# git writes the length only when a hunk spans more than one line, so a header
+# reads "@@ -1 +1 @@". Requiring a comma after the number left those hunks
+# numbered from zero, which dropped the line-number link entirely.
+test_expect_success 'set up a repo with single-line hunks' '
+	mkrepo repos/hunk1 1 &&
+	(
+		cd repos/hunk1 &&
+		printf "only\n" >one.txt &&
+		git add one.txt &&
+		git commit -m "add one.txt" &&
+		printf "ONLY\n" >one.txt &&
+		git commit -am "change one.txt"
+	) &&
+	{
+		echo "virtual-root=/" &&
+		echo "cache-size=0" &&
+		echo "repo.url=hunk1" &&
+		echo "repo.path=$PWD/repos/hunk1/.git"
+	} >hunk1rc &&
+	git -C repos/hunk1 diff HEAD~1 HEAD >raw.diff &&
+	grep "^@@ -1 +1 @@" raw.diff
+'
+
+test_expect_success 'ssdiff numbers a comma-less hunk from its real line' '
+	CGIT_CONFIG="$PWD/hunk1rc" QUERY_STRING="url=hunk1/diff/&dt=1" cgit >tmp &&
+	grep "#n1.>1</a>" tmp &&
+	! grep "#n0.>0</a>" tmp
+'
+
+test_expect_success 'ssdiff still numbers a hunk that carries a length' '
+	cgit_query "url=foo/diff&dt=1" >tmp &&
+	grep "#n1.>1</a>" tmp
+'
+
 test_done