8 files changed, 132 insertions, 2 deletions

diff --git a/source/cache.c b/source/cache.cindex 105ecfec..e9167283 100644--- a/source/cache.c+++ b/source/cache.c@@ -242,6 +242,7 @@ static int fill_slot(struct cache_slot *slot) 	slot->fn();  	/* Make sure any buffered data is flushed to the file */+	html_flush(); 	if (fflush(stdout)) 		return errno; 
diff --git a/source/cache.c b/source/cache.c
index 105ecfec..e9167283 100644
--- a/source/cache.c
+++ b/source/cache.c
@@ -242,6 +242,7 @@ static int fill_slot(struct cache_slot *slot)
 	slot->fn();
 
 	/* Make sure any buffered data is flushed to the file */
+	html_flush();
 	if (fflush(stdout))
 		return errno;
 
diff --git a/source/cgit.c b/source/cgit.cindex 5cb52fdf..2ecfb4fd 100644--- a/source/cgit.c+++ b/source/cgit.c@@ -1087,6 +1087,9 @@ int cmd_main(int argc, const char **argv) 	isolate_git_environment(); 	cgit_init_filters(); 	atexit(cgit_cleanup_filters);+	// Registered second so it runs first, since exit is reached from error+	// paths and from the HEAD shortcut with a page still buffered.+	atexit(html_flush); 	set_die_routine(cgit_die_routine);  	prepare_context();
diff --git a/source/cgit.c b/source/cgit.c
index 5cb52fdf..2ecfb4fd 100644
--- a/source/cgit.c
+++ b/source/cgit.c
@@ -1087,6 +1087,9 @@ int cmd_main(int argc, const char **argv)
 	isolate_git_environment();
 	cgit_init_filters();
 	atexit(cgit_cleanup_filters);
+	// Registered second so it runs first, since exit is reached from error
+	// paths and from the HEAD shortcut with a page still buffered.
+	atexit(html_flush);
 	set_die_routine(cgit_die_routine);
 
 	prepare_context();
diff --git a/source/filter.c b/source/filter.cindex e15a5e98..f1955347 100644--- a/source/filter.c+++ b/source/filter.c@@ -216,6 +216,10 @@ static inline int hook_lua_filter(lua_State *lua_state, void (*fn)(const char *t 	save_filter = current_write_filter; 	unhook_write(); 	fn(str);+	// fn buffers, so empty it while the hook is still off. Re-hooking first+	// would send the page's own bytes back into the filter that asked for+	// them to be written.+	html_flush(); 	hook_write(save_filter, save_filter_write);  	return 0;@@ -368,6 +372,9 @@ int cgit_open_filter(struct cgit_filter *filter, ...) 	va_list ap; 	if (!filter) 		return 0;+	// Whatever is still buffered belongs to the page, not to the filter+	// that is about to take over stdout.+	html_flush(); 	va_start(ap, filter); 	result = filter->open(filter, ap); 	va_end(ap);@@ -378,6 +385,9 @@ int cgit_close_filter(struct cgit_filter *filter) { 	if (!filter) 		return 0;+	// Anything written while the filter was open is meant for it, so this+	// has to go out before the filter hands stdout back.+	html_flush(); 	return filter->close(filter); } 
diff --git a/source/filter.c b/source/filter.c
index e15a5e98..f1955347 100644
--- a/source/filter.c
+++ b/source/filter.c
@@ -216,6 +216,10 @@ static inline int hook_lua_filter(lua_State *lua_state, void (*fn)(const char *t
 	save_filter = current_write_filter;
 	unhook_write();
 	fn(str);
+	// fn buffers, so empty it while the hook is still off. Re-hooking first
+	// would send the page's own bytes back into the filter that asked for
+	// them to be written.
+	html_flush();
 	hook_write(save_filter, save_filter_write);
 
 	return 0;
@@ -368,6 +372,9 @@ int cgit_open_filter(struct cgit_filter *filter, ...)
 	va_list ap;
 	if (!filter)
 		return 0;
+	// Whatever is still buffered belongs to the page, not to the filter
+	// that is about to take over stdout.
+	html_flush();
 	va_start(ap, filter);
 	result = filter->open(filter, ap);
 	va_end(ap);
@@ -378,6 +385,9 @@ int cgit_close_filter(struct cgit_filter *filter)
 {
 	if (!filter)
 		return 0;
+	// Anything written while the filter was open is meant for it, so this
+	// has to go out before the filter hands stdout back.
+	html_flush();
 	return filter->close(filter);
 }
 
diff --git a/source/html.c b/source/html.cindex c3ab6967..dbb0a57f 100644--- a/source/html.c+++ b/source/html.c@@ -9,6 +9,9 @@ #include "cgit.h" #include "html.h" #include "url.h"+#define HTML_WRITE_BUFSIZE (64 * 1024)+static char html_buf[HTML_WRITE_BUFSIZE];+static size_t html_buflen;  /* Percent-encoding of each character, except: a-zA-Z0-9!$()*,./:;@- */ static const char* url_escape_table[256] = {@@ -78,12 +81,52 @@ char *cgit_fmtalloc(const char *format, ...) 	return strbuf_detach(&sb, NULL); } -void html_raw(const char *data, size_t size)+/*+ * Page output is collected here and written out in whole buffers. A page is+ * built from a great many small fragments, and writing each one cost a syscall+ * apiece: a tree listing spent more time entering the kernel than generating+ * anything.+ *+ * cgit does not own stdout by itself, so the buffer has to be emptied before+ * anything else writes there. Those points are a filter taking over stdout,+ * the header block that precedes output produced by git itself, the cache+ * slot being measured, and process exit.+ */+static void html_write(const char *data, size_t size) {-	if (write(STDOUT_FILENO, data, size) != size)+	// A blob, snapshot or patch reaches this with a size well past what one+	// write can move onto a pipe, so a short write is ordinary rather than+	// an error and has to be resumed instead of reported.+	if (write_in_full(STDOUT_FILENO, data, size) < 0) 		die_errno("write error on html output"); } +void html_flush(void)+{+	size_t len = html_buflen;++	if (!len)+		return;+	// Clear the length first: html_write can die, and the error page it+	// produces would otherwise try to flush the same bytes again.+	html_buflen = 0;+	html_write(html_buf, len);+}++void html_raw(const char *data, size_t size)+{+	if (size >= HTML_WRITE_BUFSIZE) {+		// Nothing is gained by copying a blob through the buffer.+		html_flush();+		html_write(data, size);+		return;+	}+	if (html_buflen + size > HTML_WRITE_BUFSIZE)+		html_flush();+	memcpy(html_buf + html_buflen, data, size);+	html_buflen += size;+}+ void html(const char *txt) { 	html_raw(txt, strlen(txt));
diff --git a/source/html.c b/source/html.c
index c3ab6967..dbb0a57f 100644
--- a/source/html.c
+++ b/source/html.c
@@ -9,6 +9,9 @@
 #include "cgit.h"
 #include "html.h"
 #include "url.h"
+#define HTML_WRITE_BUFSIZE (64 * 1024)
+static char html_buf[HTML_WRITE_BUFSIZE];
+static size_t html_buflen;
 
 /* Percent-encoding of each character, except: a-zA-Z0-9!$()*,./:;@- */
 static const char* url_escape_table[256] = {
@@ -78,12 +81,52 @@ char *cgit_fmtalloc(const char *format, ...)
 	return strbuf_detach(&sb, NULL);
 }
 
-void html_raw(const char *data, size_t size)
+/*
+ * Page output is collected here and written out in whole buffers. A page is
+ * built from a great many small fragments, and writing each one cost a syscall
+ * apiece: a tree listing spent more time entering the kernel than generating
+ * anything.
+ *
+ * cgit does not own stdout by itself, so the buffer has to be emptied before
+ * anything else writes there. Those points are a filter taking over stdout,
+ * the header block that precedes output produced by git itself, the cache
+ * slot being measured, and process exit.
+ */
+static void html_write(const char *data, size_t size)
 {
-	if (write(STDOUT_FILENO, data, size) != size)
+	// A blob, snapshot or patch reaches this with a size well past what one
+	// write can move onto a pipe, so a short write is ordinary rather than
+	// an error and has to be resumed instead of reported.
+	if (write_in_full(STDOUT_FILENO, data, size) < 0)
 		die_errno("write error on html output");
 }
 
+void html_flush(void)
+{
+	size_t len = html_buflen;
+
+	if (!len)
+		return;
+	// Clear the length first: html_write can die, and the error page it
+	// produces would otherwise try to flush the same bytes again.
+	html_buflen = 0;
+	html_write(html_buf, len);
+}
+
+void html_raw(const char *data, size_t size)
+{
+	if (size >= HTML_WRITE_BUFSIZE) {
+		// Nothing is gained by copying a blob through the buffer.
+		html_flush();
+		html_write(data, size);
+		return;
+	}
+	if (html_buflen + size > HTML_WRITE_BUFSIZE)
+		html_flush();
+	memcpy(html_buf + html_buflen, data, size);
+	html_buflen += size;
+}
+
 void html(const char *txt)
 {
 	html_raw(txt, strlen(txt));
diff --git a/source/html.h b/source/html.hindex 75726235..353254e9 100644--- a/source/html.h+++ b/source/html.h@@ -13,6 +13,10 @@ extern void html(const char *txt);  * blob limits do not bound. */ #define HTML_BATCH (64 * 1024) +/* Write out whatever page output is still buffered. Call before anything+ * other than html_raw writes to stdout. */+extern void html_flush(void);+ __attribute__((format (printf,1,2))) extern void htmlf(const char *format,...); 
diff --git a/source/html.h b/source/html.h
index 75726235..353254e9 100644
--- a/source/html.h
+++ b/source/html.h
@@ -13,6 +13,10 @@ extern void html(const char *txt);
  * blob limits do not bound. */
 #define HTML_BATCH (64 * 1024)
 
+/* Write out whatever page output is still buffered. Call before anything
+ * other than html_raw writes to stdout. */
+extern void html_flush(void);
+
 __attribute__((format (printf,1,2)))
 extern void htmlf(const char *format,...);
 
diff --git a/source/ui-shared.c b/source/ui-shared.cindex 125a2350..23da2868 100644--- a/source/ui-shared.c+++ b/source/ui-shared.c@@ -808,6 +808,10 @@ void cgit_print_http_headers(void) 	if (ctx.page.etag) 		htmlf("ETag: \"%s\"\n", ctx.page.etag); 	html("\n");+	// Several pages follow the header block with output produced by git+	// rather than by html_raw, the patch, raw diff and snapshot views among+	// them. Emptying the buffer here keeps the headers ahead of it.+	html_flush(); 	if (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD")) 		exit(0); }
diff --git a/source/ui-shared.c b/source/ui-shared.c
index 125a2350..23da2868 100644
--- a/source/ui-shared.c
+++ b/source/ui-shared.c
@@ -808,6 +808,10 @@ void cgit_print_http_headers(void)
 	if (ctx.page.etag)
 		htmlf("ETag: \"%s\"\n", ctx.page.etag);
 	html("\n");
+	// Several pages follow the header block with output produced by git
+	// rather than by html_raw, the patch, raw diff and snapshot views among
+	// them. Emptying the buffer here keeps the headers ahead of it.
+	html_flush();
 	if (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD"))
 		exit(0);
 }
diff --git a/tests/t0020-validate-cache.sh b/tests/t0020-validate-cache.shindex 657765d8..7e6334cc 100755--- a/tests/t0020-validate-cache.sh+++ b/tests/t0020-validate-cache.sh@@ -75,4 +75,53 @@ test_expect_success 'verify cache-size=1021' ' 	test_cmp output.full output.second ' +# --- A cached page must hold everything the uncached one produced -----------+# Page output is buffered, so the slot is only complete if the buffer is+# emptied before the generated content is measured. The blob here is larger+# than one buffer, so a missing flush would truncate the cached copy.+test_expect_success 'set up a repo with a page larger than the output buffer' '+	mkrepo repos/bigpage 1 &&+	(+		cd repos/bigpage &&+		awk "BEGIN{for(i=0;i<20000;i++) print \"line \" i \" of the big file\"}" >big.txt &&+		git add big.txt &&+		git commit -m "add big.txt"+	) &&+	{+		echo "virtual-root=/" &&+		echo "cache-root=$PWD/cache2" &&+		echo "cache-size=64" &&+		echo "enable-tree-linenumbers=1" &&+		echo "repo.url=bigpage" &&+		echo "repo.path=$PWD/repos/bigpage/.git"+	} >bigrc &&+	rm -rf cache2 && mkdir cache2+'++bigq() { CGIT_CONFIG="$PWD/bigrc" QUERY_STRING="$1" cgit; }++test_expect_success 'the first request fills the slot and the second replays it' '+	bigq "url=bigpage/tree/big.txt" >big.first &&+	test $(wc -c <big.first) -gt 65536 &&+	bigq "url=bigpage/tree/big.txt" >big.second &&+	strip_headers <big.first >big.first.body &&+	strip_headers <big.second >big.second.body &&+	test_cmp big.first.body big.second.body+'++test_expect_success 'the cached body matches one generated with the cache off' '+	sed -e "s/^cache-size=64$/cache-size=0/" bigrc >bignocache &&+	CGIT_CONFIG="$PWD/bignocache" QUERY_STRING="url=bigpage/tree/big.txt" cgit >big.nocache &&+	strip_headers <big.nocache >big.nocache.body &&+	# The footer carries the time the page was generated, which differs+	# between the two runs by construction.+	sed -e "s/generated by .*//" big.nocache.body >big.a &&+	sed -e "s/generated by .*//" big.second.body >big.b &&+	test_cmp big.a big.b+'++test_expect_success 'the page ends where it should, so nothing was dropped' '+	tail -c 200 big.second.body | grep "</html>"+'+ test_done
diff --git a/tests/t0020-validate-cache.sh b/tests/t0020-validate-cache.sh
index 657765d8..7e6334cc 100755
--- a/tests/t0020-validate-cache.sh
+++ b/tests/t0020-validate-cache.sh
@@ -75,4 +75,53 @@ test_expect_success 'verify cache-size=1021' '
 	test_cmp output.full output.second
 '
 
+# --- A cached page must hold everything the uncached one produced -----------
+# Page output is buffered, so the slot is only complete if the buffer is
+# emptied before the generated content is measured. The blob here is larger
+# than one buffer, so a missing flush would truncate the cached copy.
+test_expect_success 'set up a repo with a page larger than the output buffer' '
+	mkrepo repos/bigpage 1 &&
+	(
+		cd repos/bigpage &&
+		awk "BEGIN{for(i=0;i<20000;i++) print \"line \" i \" of the big file\"}" >big.txt &&
+		git add big.txt &&
+		git commit -m "add big.txt"
+	) &&
+	{
+		echo "virtual-root=/" &&
+		echo "cache-root=$PWD/cache2" &&
+		echo "cache-size=64" &&
+		echo "enable-tree-linenumbers=1" &&
+		echo "repo.url=bigpage" &&
+		echo "repo.path=$PWD/repos/bigpage/.git"
+	} >bigrc &&
+	rm -rf cache2 && mkdir cache2
+'
+
+bigq() { CGIT_CONFIG="$PWD/bigrc" QUERY_STRING="$1" cgit; }
+
+test_expect_success 'the first request fills the slot and the second replays it' '
+	bigq "url=bigpage/tree/big.txt" >big.first &&
+	test $(wc -c <big.first) -gt 65536 &&
+	bigq "url=bigpage/tree/big.txt" >big.second &&
+	strip_headers <big.first >big.first.body &&
+	strip_headers <big.second >big.second.body &&
+	test_cmp big.first.body big.second.body
+'
+
+test_expect_success 'the cached body matches one generated with the cache off' '
+	sed -e "s/^cache-size=64$/cache-size=0/" bigrc >bignocache &&
+	CGIT_CONFIG="$PWD/bignocache" QUERY_STRING="url=bigpage/tree/big.txt" cgit >big.nocache &&
+	strip_headers <big.nocache >big.nocache.body &&
+	# The footer carries the time the page was generated, which differs
+	# between the two runs by construction.
+	sed -e "s/generated by .*//" big.nocache.body >big.a &&
+	sed -e "s/generated by .*//" big.second.body >big.b &&
+	test_cmp big.a big.b
+'
+
+test_expect_success 'the page ends where it should, so nothing was dropped' '
+	tail -c 200 big.second.body | grep "</html>"
+'
+
 test_done
diff --git a/tests/t0111-filter.sh b/tests/t0111-filter.shindex 2fdc3669..da4172f6 100755--- a/tests/t0111-filter.sh+++ b/tests/t0111-filter.sh@@ -41,6 +41,22 @@ do 	test_expect_success "check whether the $prefix email filter works for committers" ' 		grep "<committer@example.com> commit C O MITTER &LT;COMMITTER@EXAMPLE.COM&GT;" tmp 	'++	# Page output is buffered, so anything written before a filter opens has+	# to leave the buffer before the filter takes over stdout, and anything+	# written while it is open has to leave before stdout is handed back.+	# A missed flush reorders the page rather than losing it, so check that+	# the markup around the filtered text is still on the right side of it.+	test_expect_success "the $prefix source filter output stays inside its cell" "+		cgit_url 'filter-$prefix/tree/a%2bb' >tmp &&+		tr -d '\n' <tmp >flat.out &&+		grep '<code>a+b HELLO</code>' flat.out+	"++	test_expect_success "the $prefix about filter output stays inside its div" "+		cgit_url 'filter-$prefix/about/' >tmp &&+		grep \"<div id='summary'>a+b HELLO\" tmp+	" done  test_done
diff --git a/tests/t0111-filter.sh b/tests/t0111-filter.sh
index 2fdc3669..da4172f6 100755
--- a/tests/t0111-filter.sh
+++ b/tests/t0111-filter.sh
@@ -41,6 +41,22 @@ do
 	test_expect_success "check whether the $prefix email filter works for committers" '
 		grep "<committer@example.com> commit C O MITTER &LT;COMMITTER@EXAMPLE.COM&GT;" tmp
 	'
+
+	# Page output is buffered, so anything written before a filter opens has
+	# to leave the buffer before the filter takes over stdout, and anything
+	# written while it is open has to leave before stdout is handed back.
+	# A missed flush reorders the page rather than losing it, so check that
+	# the markup around the filtered text is still on the right side of it.
+	test_expect_success "the $prefix source filter output stays inside its cell" "
+		cgit_url 'filter-$prefix/tree/a%2bb' >tmp &&
+		tr -d '\n' <tmp >flat.out &&
+		grep '<code>a+b HELLO</code>' flat.out
+	"
+
+	test_expect_success "the $prefix about filter output stays inside its div" "
+		cgit_url 'filter-$prefix/about/' >tmp &&
+		grep \"<div id='summary'>a+b HELLO\" tmp
+	"
 done
 
 test_done