Render each diff body while its file is open

3 files changed, 222 insertions, 16 deletions

diff --git a/source/html.c b/source/html.cindex dbb0a57..6e023d1 100644--- a/source/html.c+++ b/source/html.c@@ -113,8 +113,35 @@ void html_flush(void) 	html_write(html_buf, len); } +/*+ * While a capture is in effect, page output is collected into the caller's+ * buffer instead of being written. The diff view uses this to render a file's+ * body at the point it already has the file open, rather than walking the whole+ * tree a second time to produce what the diffstat above it has to be printed+ * before.+ *+ * A capture must not span anything that writes to stdout by another route, a+ * filter in particular, since that output would escape the capture.+ */+static struct strbuf *html_capture;++void html_capture_begin(struct strbuf *sb)+{+	html_flush();+	html_capture = sb;+}++void html_capture_end(void)+{+	html_capture = NULL;+}+ void html_raw(const char *data, size_t size) {+	if (html_capture) {+		strbuf_add(html_capture, data, size);+		return;+	} 	if (size >= HTML_WRITE_BUFSIZE) { 		// Nothing is gained by copying a blob through the buffer. 		html_flush();
diff --git a/source/html.c b/source/html.c
index dbb0a57..6e023d1 100644
--- a/source/html.c
+++ b/source/html.c
@@ -113,8 +113,35 @@ void html_flush(void)
 	html_write(html_buf, len);
 }
 
+/*
+ * While a capture is in effect, page output is collected into the caller's
+ * buffer instead of being written. The diff view uses this to render a file's
+ * body at the point it already has the file open, rather than walking the whole
+ * tree a second time to produce what the diffstat above it has to be printed
+ * before.
+ *
+ * A capture must not span anything that writes to stdout by another route, a
+ * filter in particular, since that output would escape the capture.
+ */
+static struct strbuf *html_capture;
+
+void html_capture_begin(struct strbuf *sb)
+{
+	html_flush();
+	html_capture = sb;
+}
+
+void html_capture_end(void)
+{
+	html_capture = NULL;
+}
+
 void html_raw(const char *data, size_t size)
 {
+	if (html_capture) {
+		strbuf_add(html_capture, data, size);
+		return;
+	}
 	if (size >= HTML_WRITE_BUFSIZE) {
 		// Nothing is gained by copying a blob through the buffer.
 		html_flush();
diff --git a/source/html.h b/source/html.hindex 353254e..0818ab5 100644--- a/source/html.h+++ b/source/html.h@@ -17,6 +17,11 @@ extern void html(const char *txt);  * other than html_raw writes to stdout. */ extern void html_flush(void); +/* Collect page output into sb rather than writing it, until html_capture_end.+ * Must not span a filter or anything else that writes to stdout directly. */+extern void html_capture_begin(struct strbuf *sb);+extern void html_capture_end(void);+ __attribute__((format (printf,1,2))) extern void htmlf(const char *format,...); 
diff --git a/source/html.h b/source/html.h
index 353254e..0818ab5 100644
--- a/source/html.h
+++ b/source/html.h
@@ -17,6 +17,11 @@ extern void html(const char *txt);
  * other than html_raw writes to stdout. */
 extern void html_flush(void);
 
+/* Collect page output into sb rather than writing it, until html_capture_end.
+ * Must not span a filter or anything else that writes to stdout directly. */
+extern void html_capture_begin(struct strbuf *sb);
+extern void html_capture_end(void);
+
 __attribute__((format (printf,1,2)))
 extern void htmlf(const char *format,...);
 
diff --git a/source/ui-diff.c b/source/ui-diff.cindex ebf9425..a0790f7 100644--- a/source/ui-diff.c+++ b/source/ui-diff.c@@ -14,14 +14,7 @@ #include "ui-shared.h" #include "ui-ssdiff.h" -struct object_id old_rev_oid[1];-struct object_id new_rev_oid[1];--static int files, slots;-static int total_adds, total_rems, max_changes;-static int lines_added, lines_removed;--static struct fileinfo {+struct fileinfo { 	char status; 	struct object_id old_oid[1]; 	struct object_id new_oid[1];@@ -34,17 +27,71 @@ static struct fileinfo { 	unsigned long old_size; 	unsigned long new_size; 	unsigned int binary:1;-} *items;+	struct strbuf body;+};++/*+ * The diffstat has to be printed before the file bodies, but both are produced+ * by the same walk over the tree. So each body is rendered into the item it+ * belongs to while that file is already open, and replayed once the stat table+ * above it has been written. Rendering separately meant a second walk with its+ * own rename detection and its own xdiff of every file.+ *+ * A body stops being collected past max-diff-lines, since a file over that is+ * replaced by a link, and collection stops altogether past the budget below,+ * after which the bodies are produced the old way. The budget is a bound on+ * one request, not something to configure.+ *+ * Only a view that caps its bodies collects them. Without max-diff-lines to+ * stop it, a single body grows with the file it came from, which for a+ * side-by-side diff is several times the blob itself, and the budget below+ * cannot help because it is only reached once a body is already complete.+ */+#define CGIT_DIFF_BODY_BUDGET (8 * 1024 * 1024)++/* The revisions being compared, read by ui-ssdiff when it builds line links. */+struct object_id old_rev_oid[1];+struct object_id new_rev_oid[1];++/* One entry per file in the diff, filled by the walk and replayed afterwards. */+static struct fileinfo *items;+static int files, slots;++/* Totals for the diffstat, accumulated across that same walk. */+static int total_adds, total_rems, max_changes;+static int lines_added, lines_removed;++/* How much body text has been collected, and whether collecting is still+ * worthwhile. Once the budget is passed the bodies are produced the old way. */+static size_t body_bytes;+static int bodies_usable; -static int use_ssdiff = 0;+/* Which renderer the current file's lines go to, and whether the file has+ * already passed max-diff-lines and so stopped being rendered. */+static linediff_fn render_line_fn;+static int render_suppressed;++/* The file being rendered right now, and the path the view is held to. */ static struct diff_filepair *current_filepair; static const char *current_prefix; +static int use_ssdiff;+ /* Caps apply only to whole-commit views. A single-file diff page must  * always render fully, since it is where the capped views link to. */ static int cap_diffs; static int item_idx; +/* The bodies have either been replayed or been given up on, so in both cases+ * what they hold is finished with. */+static void release_bodies(void)+{+	int i;++	for (i = 0; i < files; i++)+		strbuf_release(&items[i].body);+}+ struct diff_filespec *cgit_get_current_old_file(void) { 	return current_filepair->one;@@ -130,6 +177,8 @@ static void print_fileinfo(struct fileinfo *info) 	html("</tr></table></td></tr>\n"); } +/* Counts every line, and renders it too until the file passes max-diff-lines,+ * at which point the body is going to be replaced by a link anyway. */ static void count_diff_lines(char *line, int len) { 	if (line && (len > 0)) {@@ -138,6 +187,14 @@ static void count_diff_lines(char *line, int len) 		else if (line[0] == '-') 			lines_removed++; 	}+	if (!render_line_fn || render_suppressed)+		return;+	if (cap_diffs && ctx.cfg.max_diff_lines > 0 &&+	    lines_added + lines_removed > ctx.cfg.max_diff_lines) {+		render_suppressed = 1;+		return;+	}+	render_line_fn(line, len); }  static int show_filepair(struct diff_filepair *pair)@@ -155,6 +212,103 @@ static int show_filepair(struct diff_filepair *pair) 	return 0; } +static void print_line(char *line, int len);+static void header(const struct object_id *oid1, char *path1, int mode1,+		   const struct object_id *oid2, char *path2, int mode2);++/* Render one file's body into its own buffer, the way filepair_cb would have+ * written it straight out on a second walk. The line count is not known until+ * the diff has run, so the header is collected first and what follows it is+ * decided afterwards. */+static void collect_filepair_body(struct diff_filepair *pair, int idx,+				  int *binary, unsigned long *old_size,+				  unsigned long *new_size)+{+	struct strbuf *body = &items[idx].body;+	linediff_fn line_fn = use_ssdiff ? cgit_ssdiff_line_cb : print_line;+	size_t header_len;++	current_filepair = pair;++	html_capture_begin(body);+	if (use_ssdiff)+		cgit_ssdiff_header_begin();+	header(&pair->one->oid, pair->one->path, pair->one->mode,+	       &pair->two->oid, pair->two->path, pair->two->mode);+	if (use_ssdiff)+		cgit_ssdiff_header_end();+	// Everything from here on is dropped if the file turns out to be over+	// the line budget, so remember where the header ended.+	header_len = body->len;++	if (S_ISGITLINK(pair->one->mode) || S_ISGITLINK(pair->two->mode)) {+		/* The body shows the two Subproject lines, but the stat has+		 * always counted what a diff of the pair produces, so run it+		 * for the count alone. */+		render_line_fn = NULL;+		cgit_diff_files(&pair->one->oid, &pair->two->oid, old_size,+				new_size, binary, 0, ctx.qry.ignorews,+				count_diff_lines);+		if (S_ISGITLINK(pair->one->mode)) {+			char *l = cgit_fmt("-Subproject %s", oid_to_hex(&pair->one->oid));+			line_fn(l, strlen(l) + 1);+		}+		if (S_ISGITLINK(pair->two->mode)) {+			char *l = cgit_fmt("+Subproject %s", oid_to_hex(&pair->two->oid));+			line_fn(l, strlen(l) + 1);+		}+	} else {+		render_line_fn = line_fn;+		render_suppressed = 0;+		if (cgit_diff_files(&pair->one->oid, &pair->two->oid, old_size,+				    new_size, binary, ctx.qry.context,+				    ctx.qry.ignorews, count_diff_lines))+			cgit_print_error("Error running diff");+		render_line_fn = NULL;+		if (*binary) {+			if (use_ssdiff)+				html("<tr><td colspan='4'>Binary files differ</td></tr>");+			else+				html("Binary files differ");+		}+	}+	if (use_ssdiff)+		cgit_ssdiff_footer();++	if (render_suppressed) {+		/* Over the line budget, so the body is a link instead. Setting+		 * the length back would leave the buffer holding everything it+		 * grew to while rendering, which the budget below cannot see+		 * because it only counts what is kept, so rebuild it at the+		 * size actually kept. */+		char *header_text = xmemdupz(body->buf, header_len);++		strbuf_release(body);+		strbuf_attach(body, header_text, header_len, header_len + 1);+		if (use_ssdiff)+			html("<tr><td colspan='4' class='truncated'>");+		else+			html("<div class='truncated'>");+		html("This diff is too large to be rendered inline. ");+		cgit_diff_link("View it on its own page", NULL, NULL,+			       ctx.qry.head, ctx.qry.oid, ctx.qry.oid2,+			       pair->two->path);+		html(".");+		if (use_ssdiff) {+			html("</td></tr>");+			cgit_ssdiff_footer();+		} else+			html("</div>");+	}+	html_capture_end();++	body_bytes += body->len;+	if (body_bytes > CGIT_DIFF_BODY_BUDGET) {+		bodies_usable = 0;+		release_bodies();+	}+}+ static void inspect_filepair(struct diff_filepair *pair) { 	int binary = 0;@@ -167,8 +321,6 @@ static void inspect_filepair(struct diff_filepair *pair) 	files++; 	lines_added = 0; 	lines_removed = 0;-	cgit_diff_files(&pair->one->oid, &pair->two->oid, &old_size, &new_size,-			&binary, 0, ctx.qry.ignorews, count_diff_lines); 	if (files >= slots) { 		if (slots == 0) 			slots = 4;@@ -176,6 +328,17 @@ static void inspect_filepair(struct diff_filepair *pair) 			slots = slots * 2; 		items = xrealloc(items, slots * sizeof(struct fileinfo)); 	}+	memset(&items[files-1], 0, sizeof(items[files-1]));+	strbuf_init(&items[files-1].body, 0);++	if (bodies_usable)+		collect_filepair_body(pair, files - 1, &binary,+				      &old_size, &new_size);+	else+		cgit_diff_files(&pair->one->oid, &pair->two->oid, &old_size,+				&new_size, &binary, 0, ctx.qry.ignorews,+				count_diff_lines);+ 	items[files-1].status = pair->status; 	oidcpy(items[files-1].old_oid, &pair->one->oid); 	oidcpy(items[files-1].new_oid, &pair->two->oid);@@ -222,7 +385,6 @@ static void cgit_print_diffstat(const struct object_id *old_oid, 	html("</div>"); } - /*  * print a single line returned from xdiff  */@@ -503,6 +665,9 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,  	difftype = ctx.qry.has_difftype ? ctx.qry.difftype : ctx.cfg.difftype; 	use_ssdiff = difftype == DIFF_SSDIFF;+	/* A stat-only view never shows a body, and only a capped view bounds+	 * the size of the ones it does show. */+	bodies_usable = difftype != DIFF_STATONLY && cap_diffs;  	if (show_ctrls) { 		cgit_print_layout_start();@@ -548,9 +713,18 @@ void cgit_print_diff(const char *new_rev, const char *old_rev, 		html("<table summary='diff' class='diff'>"); 		html("<tr><td>"); 	}-	item_idx = 0;-	cgit_diff_tree(old_rev_oid, new_rev_oid, filepair_cb, prefix,-		       ctx.qry.ignorews);+	if (bodies_usable) {+		int i;+		for (i = 0; i < files; i++)+			html_raw(items[i].body.buf, items[i].body.len);+		release_bodies();+	} else {+		/* The bodies outgrew the budget, so they are produced the way+		 * they were before, by walking the tree again. */+		item_idx = 0;+		cgit_diff_tree(old_rev_oid, new_rev_oid, filepair_cb, prefix,+			       ctx.qry.ignorews);+	} 	if (!use_ssdiff) 		html("</td></tr>"); 	html("</table>");
diff --git a/source/ui-diff.c b/source/ui-diff.c
index ebf9425..a0790f7 100644
--- a/source/ui-diff.c
+++ b/source/ui-diff.c
@@ -14,14 +14,7 @@
 #include "ui-shared.h"
 #include "ui-ssdiff.h"
 
-struct object_id old_rev_oid[1];
-struct object_id new_rev_oid[1];
-
-static int files, slots;
-static int total_adds, total_rems, max_changes;
-static int lines_added, lines_removed;
-
-static struct fileinfo {
+struct fileinfo {
 	char status;
 	struct object_id old_oid[1];
 	struct object_id new_oid[1];
@@ -34,17 +27,71 @@ static struct fileinfo {
 	unsigned long old_size;
 	unsigned long new_size;
 	unsigned int binary:1;
-} *items;
+	struct strbuf body;
+};
+
+/*
+ * The diffstat has to be printed before the file bodies, but both are produced
+ * by the same walk over the tree. So each body is rendered into the item it
+ * belongs to while that file is already open, and replayed once the stat table
+ * above it has been written. Rendering separately meant a second walk with its
+ * own rename detection and its own xdiff of every file.
+ *
+ * A body stops being collected past max-diff-lines, since a file over that is
+ * replaced by a link, and collection stops altogether past the budget below,
+ * after which the bodies are produced the old way. The budget is a bound on
+ * one request, not something to configure.
+ *
+ * Only a view that caps its bodies collects them. Without max-diff-lines to
+ * stop it, a single body grows with the file it came from, which for a
+ * side-by-side diff is several times the blob itself, and the budget below
+ * cannot help because it is only reached once a body is already complete.
+ */
+#define CGIT_DIFF_BODY_BUDGET (8 * 1024 * 1024)
+
+/* The revisions being compared, read by ui-ssdiff when it builds line links. */
+struct object_id old_rev_oid[1];
+struct object_id new_rev_oid[1];
+
+/* One entry per file in the diff, filled by the walk and replayed afterwards. */
+static struct fileinfo *items;
+static int files, slots;
+
+/* Totals for the diffstat, accumulated across that same walk. */
+static int total_adds, total_rems, max_changes;
+static int lines_added, lines_removed;
+
+/* How much body text has been collected, and whether collecting is still
+ * worthwhile. Once the budget is passed the bodies are produced the old way. */
+static size_t body_bytes;
+static int bodies_usable;
 
-static int use_ssdiff = 0;
+/* Which renderer the current file's lines go to, and whether the file has
+ * already passed max-diff-lines and so stopped being rendered. */
+static linediff_fn render_line_fn;
+static int render_suppressed;
+
+/* The file being rendered right now, and the path the view is held to. */
 static struct diff_filepair *current_filepair;
 static const char *current_prefix;
 
+static int use_ssdiff;
+
 /* Caps apply only to whole-commit views. A single-file diff page must
  * always render fully, since it is where the capped views link to. */
 static int cap_diffs;
 static int item_idx;
 
+/* The bodies have either been replayed or been given up on, so in both cases
+ * what they hold is finished with. */
+static void release_bodies(void)
+{
+	int i;
+
+	for (i = 0; i < files; i++)
+		strbuf_release(&items[i].body);
+}
+
 struct diff_filespec *cgit_get_current_old_file(void)
 {
 	return current_filepair->one;
@@ -130,6 +177,8 @@ static void print_fileinfo(struct fileinfo *info)
 	html("</tr></table></td></tr>\n");
 }
 
+/* Counts every line, and renders it too until the file passes max-diff-lines,
+ * at which point the body is going to be replaced by a link anyway. */
 static void count_diff_lines(char *line, int len)
 {
 	if (line && (len > 0)) {
@@ -138,6 +187,14 @@ static void count_diff_lines(char *line, int len)
 		else if (line[0] == '-')
 			lines_removed++;
 	}
+	if (!render_line_fn || render_suppressed)
+		return;
+	if (cap_diffs && ctx.cfg.max_diff_lines > 0 &&
+	    lines_added + lines_removed > ctx.cfg.max_diff_lines) {
+		render_suppressed = 1;
+		return;
+	}
+	render_line_fn(line, len);
 }
 
 static int show_filepair(struct diff_filepair *pair)
@@ -155,6 +212,103 @@ static int show_filepair(struct diff_filepair *pair)
 	return 0;
 }
 
+static void print_line(char *line, int len);
+static void header(const struct object_id *oid1, char *path1, int mode1,
+		   const struct object_id *oid2, char *path2, int mode2);
+
+/* Render one file's body into its own buffer, the way filepair_cb would have
+ * written it straight out on a second walk. The line count is not known until
+ * the diff has run, so the header is collected first and what follows it is
+ * decided afterwards. */
+static void collect_filepair_body(struct diff_filepair *pair, int idx,
+				  int *binary, unsigned long *old_size,
+				  unsigned long *new_size)
+{
+	struct strbuf *body = &items[idx].body;
+	linediff_fn line_fn = use_ssdiff ? cgit_ssdiff_line_cb : print_line;
+	size_t header_len;
+
+	current_filepair = pair;
+
+	html_capture_begin(body);
+	if (use_ssdiff)
+		cgit_ssdiff_header_begin();
+	header(&pair->one->oid, pair->one->path, pair->one->mode,
+	       &pair->two->oid, pair->two->path, pair->two->mode);
+	if (use_ssdiff)
+		cgit_ssdiff_header_end();
+	// Everything from here on is dropped if the file turns out to be over
+	// the line budget, so remember where the header ended.
+	header_len = body->len;
+
+	if (S_ISGITLINK(pair->one->mode) || S_ISGITLINK(pair->two->mode)) {
+		/* The body shows the two Subproject lines, but the stat has
+		 * always counted what a diff of the pair produces, so run it
+		 * for the count alone. */
+		render_line_fn = NULL;
+		cgit_diff_files(&pair->one->oid, &pair->two->oid, old_size,
+				new_size, binary, 0, ctx.qry.ignorews,
+				count_diff_lines);
+		if (S_ISGITLINK(pair->one->mode)) {
+			char *l = cgit_fmt("-Subproject %s", oid_to_hex(&pair->one->oid));
+			line_fn(l, strlen(l) + 1);
+		}
+		if (S_ISGITLINK(pair->two->mode)) {
+			char *l = cgit_fmt("+Subproject %s", oid_to_hex(&pair->two->oid));
+			line_fn(l, strlen(l) + 1);
+		}
+	} else {
+		render_line_fn = line_fn;
+		render_suppressed = 0;
+		if (cgit_diff_files(&pair->one->oid, &pair->two->oid, old_size,
+				    new_size, binary, ctx.qry.context,
+				    ctx.qry.ignorews, count_diff_lines))
+			cgit_print_error("Error running diff");
+		render_line_fn = NULL;
+		if (*binary) {
+			if (use_ssdiff)
+				html("<tr><td colspan='4'>Binary files differ</td></tr>");
+			else
+				html("Binary files differ");
+		}
+	}
+	if (use_ssdiff)
+		cgit_ssdiff_footer();
+
+	if (render_suppressed) {
+		/* Over the line budget, so the body is a link instead. Setting
+		 * the length back would leave the buffer holding everything it
+		 * grew to while rendering, which the budget below cannot see
+		 * because it only counts what is kept, so rebuild it at the
+		 * size actually kept. */
+		char *header_text = xmemdupz(body->buf, header_len);
+
+		strbuf_release(body);
+		strbuf_attach(body, header_text, header_len, header_len + 1);
+		if (use_ssdiff)
+			html("<tr><td colspan='4' class='truncated'>");
+		else
+			html("<div class='truncated'>");
+		html("This diff is too large to be rendered inline. ");
+		cgit_diff_link("View it on its own page", NULL, NULL,
+			       ctx.qry.head, ctx.qry.oid, ctx.qry.oid2,
+			       pair->two->path);
+		html(".");
+		if (use_ssdiff) {
+			html("</td></tr>");
+			cgit_ssdiff_footer();
+		} else
+			html("</div>");
+	}
+	html_capture_end();
+
+	body_bytes += body->len;
+	if (body_bytes > CGIT_DIFF_BODY_BUDGET) {
+		bodies_usable = 0;
+		release_bodies();
+	}
+}
+
 static void inspect_filepair(struct diff_filepair *pair)
 {
 	int binary = 0;
@@ -167,8 +321,6 @@ static void inspect_filepair(struct diff_filepair *pair)
 	files++;
 	lines_added = 0;
 	lines_removed = 0;
-	cgit_diff_files(&pair->one->oid, &pair->two->oid, &old_size, &new_size,
-			&binary, 0, ctx.qry.ignorews, count_diff_lines);
 	if (files >= slots) {
 		if (slots == 0)
 			slots = 4;
@@ -176,6 +328,17 @@ static void inspect_filepair(struct diff_filepair *pair)
 			slots = slots * 2;
 		items = xrealloc(items, slots * sizeof(struct fileinfo));
 	}
+	memset(&items[files-1], 0, sizeof(items[files-1]));
+	strbuf_init(&items[files-1].body, 0);
+
+	if (bodies_usable)
+		collect_filepair_body(pair, files - 1, &binary,
+				      &old_size, &new_size);
+	else
+		cgit_diff_files(&pair->one->oid, &pair->two->oid, &old_size,
+				&new_size, &binary, 0, ctx.qry.ignorews,
+				count_diff_lines);
+
 	items[files-1].status = pair->status;
 	oidcpy(items[files-1].old_oid, &pair->one->oid);
 	oidcpy(items[files-1].new_oid, &pair->two->oid);
@@ -222,7 +385,6 @@ static void cgit_print_diffstat(const struct object_id *old_oid,
 	html("</div>");
 }
 
-
 /*
  * print a single line returned from xdiff
  */
@@ -503,6 +665,9 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 
 	difftype = ctx.qry.has_difftype ? ctx.qry.difftype : ctx.cfg.difftype;
 	use_ssdiff = difftype == DIFF_SSDIFF;
+	/* A stat-only view never shows a body, and only a capped view bounds
+	 * the size of the ones it does show. */
+	bodies_usable = difftype != DIFF_STATONLY && cap_diffs;
 
 	if (show_ctrls) {
 		cgit_print_layout_start();
@@ -548,9 +713,18 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 		html("<table summary='diff' class='diff'>");
 		html("<tr><td>");
 	}
-	item_idx = 0;
-	cgit_diff_tree(old_rev_oid, new_rev_oid, filepair_cb, prefix,
-		       ctx.qry.ignorews);
+	if (bodies_usable) {
+		int i;
+		for (i = 0; i < files; i++)
+			html_raw(items[i].body.buf, items[i].body.len);
+		release_bodies();
+	} else {
+		/* The bodies outgrew the budget, so they are produced the way
+		 * they were before, by walking the tree again. */
+		item_idx = 0;
+		cgit_diff_tree(old_rev_oid, new_rev_oid, filepair_cb, prefix,
+			       ctx.qry.ignorews);
+	}
 	if (!use_ssdiff)
 		html("</td></tr>");
 	html("</table>");