[PATCH] ui-diff: Add configuration option to limit number of files

[PATCH] ui-diff: Add configuration option to limit number of files

[tim.nordell]

If a large commit is made, the generated HTML file can be very
large and take the server a while to generate the HTML for the
diff.  Add a configuration option to limit the maximum number
of files presented within a diff.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/cgit.c b/cgit.c
index 0ca1baa..87ba811 100644
--- a/cgit.c
+++ b/cgit.c
@@ -248,6 +248,12 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.max_blob_size = atoi(value);
 	else if (!strcmp(name, "max-repo-count"))
 		ctx.cfg.max_repo_count = atoi(value);
+	else if (!strcmp(name, "max-diff-files"))
+	{
+		ctx.cfg.max_diff_files = atoi(value);
+		if(ctx.cfg.max_diff_files < 1)
+			ctx.cfg.max_diff_files = INT_MAX;
+	}
 	else if (!strcmp(name, "max-commit-count"))
 		ctx.cfg.max_commit_count = atoi(value);
 	else if (!strcmp(name, "project-list"))
@@ -401,6 +407,7 @@ static void prepare_context(void)
 	ctx.cfg.enable_tree_linenumbers = 1;
 	ctx.cfg.enable_git_config = 0;
 	ctx.cfg.max_repo_count = 50;
+	ctx.cfg.max_diff_files = INT_MAX;
 	ctx.cfg.max_commit_count = 50;
 	ctx.cfg.max_lock_attempts = 5;
 	ctx.cfg.max_msg_len = 80;
diff --git a/cgit.h b/cgit.h
index 350d25d..b2b2ae9 100644
--- a/cgit.h
+++ b/cgit.h
@@ -243,6 +243,7 @@ struct cgit_config {
 	int enable_git_config;
 	int local_time;
 	int max_atom_items;
+	int max_diff_files;
 	int max_repo_count;
 	int max_commit_count;
 	int max_lock_attempts;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 94901bd..3bca0ab 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -420,6 +420,14 @@ side-by-side-diffs::
 	If set to "1" shows side-by-side diffs instead of unidiffs per
 	default. Default value: "0".
 
+max-diff-files::
+	This value controls the maximum number of files presented in a diff
+	view to help control the size of HTML files presented in a web browser.
+	(In a very large commit, a client web browser may choke on a large
+	commit, such as the first commit in the Linux kernel repository.)
+	A value of "0" turns this feature off.
+	Default value: "0".
+
 snapshots::
 	Text which specifies the default set of snapshot formats that cgit
 	generates links for. The value is a space-separated list of zero or
diff --git a/ui-diff.c b/ui-diff.c
index 52ed942..65045a4 100644
--- a/ui-diff.c
+++ b/ui-diff.c
@@ -18,6 +18,7 @@ unsigned char new_rev_sha1[20];
 static int files, slots;
 static int total_adds, total_rems, max_changes;
 static int lines_added, lines_removed;
+static int filepair_cnt;
 
 static struct fileinfo {
 	char status;
@@ -206,8 +207,18 @@ static void cgit_print_diffstat(const unsigned char *old_sha1,
 	max_changes = 0;
 	cgit_diff_tree(old_sha1, new_sha1, inspect_filepair, prefix,
 		       ctx.qry.ignorews);
-	for (i = 0; i<files; i++)
+	for (i = 0; i<files && i < ctx.cfg.max_diff_files; i++)
 		print_fileinfo(&items[i]);
+	/* If needed, add a row indicating that we didn't display everything */
+	if(i < files)
+	{
+		html("<tr>");
+		html("<td class='mode'>...</td>");
+		html("<td class='upd'>...</td>");
+		html("<td class='right'>...</td>");
+		html("<td class='graph'>...</td>");
+		html("</tr>");
+	}
 	html("</table>");
 	html("<div class='diffstat-summary'>");
 	htmlf("%d files changed, %d insertions, %d deletions",
@@ -302,6 +313,11 @@ static void filepair_cb(struct diff_filepair *pair)
 	if (!show_filepair(pair))
 		return;
 
+	if(filepair_cnt++ >= ctx.cfg.max_diff_files)
+	{
+		return;
+	}
+
 	current_filepair = pair;
 	if (use_ssdiff) {
 		cgit_ssdiff_header_begin();
@@ -490,6 +506,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 		html("<table summary='diff' class='diff'>");
 		html("<tr><td>");
 	}
+        filepair_cnt = 0;
 	cgit_diff_tree(old_rev_sha1, new_rev_sha1, filepair_cb, prefix,
 		       ctx.qry.ignorews);
 	if (!use_ssdiff)
-- 
2.4.9

[PATCH] ui-diff: Add configuration option to limit number of files

[john]

On Fri, Feb 26, 2016 at 02:57:22PM -0600, Tim Nordell wrote:
> If a large commit is made, the generated HTML file can be very
> large and take the server a while to generate the HTML for the
> diff.  Add a configuration option to limit the maximum number
> of files presented within a diff.
> 
> Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>
> 
> diff --git a/cgit.c b/cgit.c
> index 0ca1baa..87ba811 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -248,6 +248,12 @@ static void config_cb(const char *name, const char *value)
>  		ctx.cfg.max_blob_size = atoi(value);
>  	else if (!strcmp(name, "max-repo-count"))
>  		ctx.cfg.max_repo_count = atoi(value);
> +	else if (!strcmp(name, "max-diff-files"))
> +	{

style: brace on the previous line (see
git/Documentation/CodingGuidelines).

> +		ctx.cfg.max_diff_files = atoi(value);
> +		if(ctx.cfg.max_diff_files < 1)
> +			ctx.cfg.max_diff_files = INT_MAX;
> +	}
>  	else if (!strcmp(name, "max-commit-count"))

style: } else if (...

>  		ctx.cfg.max_commit_count = atoi(value);
>  	else if (!strcmp(name, "project-list"))
> @@ -401,6 +407,7 @@ static void prepare_context(void)
>  	ctx.cfg.enable_tree_linenumbers = 1;
>  	ctx.cfg.enable_git_config = 0;
>  	ctx.cfg.max_repo_count = 50;
> +	ctx.cfg.max_diff_files = INT_MAX;
>  	ctx.cfg.max_commit_count = 50;
>  	ctx.cfg.max_lock_attempts = 5;
>  	ctx.cfg.max_msg_len = 80;
> diff --git a/cgit.h b/cgit.h
> index 350d25d..b2b2ae9 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -243,6 +243,7 @@ struct cgit_config {
>  	int enable_git_config;
>  	int local_time;
>  	int max_atom_items;
> +	int max_diff_files;
>  	int max_repo_count;
>  	int max_commit_count;
>  	int max_lock_attempts;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 94901bd..3bca0ab 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -420,6 +420,14 @@ side-by-side-diffs::
>  	If set to "1" shows side-by-side diffs instead of unidiffs per
>  	default. Default value: "0".
>  
> +max-diff-files::
> +	This value controls the maximum number of files presented in a diff
> +	view to help control the size of HTML files presented in a web browser.
> +	(In a very large commit, a client web browser may choke on a large
> +	commit, such as the first commit in the Linux kernel repository.)
> +	A value of "0" turns this feature off.
> +	Default value: "0".
> +
>  snapshots::
>  	Text which specifies the default set of snapshot formats that cgit
>  	generates links for. The value is a space-separated list of zero or
> diff --git a/ui-diff.c b/ui-diff.c
> index 52ed942..65045a4 100644
> --- a/ui-diff.c
> +++ b/ui-diff.c
> @@ -18,6 +18,7 @@ unsigned char new_rev_sha1[20];
>  static int files, slots;
>  static int total_adds, total_rems, max_changes;
>  static int lines_added, lines_removed;
> +static int filepair_cnt;
>  
>  static struct fileinfo {
>  	char status;
> @@ -206,8 +207,18 @@ static void cgit_print_diffstat(const unsigned char *old_sha1,
>  	max_changes = 0;
>  	cgit_diff_tree(old_sha1, new_sha1, inspect_filepair, prefix,
>  		       ctx.qry.ignorews);
> -	for (i = 0; i<files; i++)
> +	for (i = 0; i<files && i < ctx.cfg.max_diff_files; i++)
>  		print_fileinfo(&items[i]);
> +	/* If needed, add a row indicating that we didn't display everything */
> +	if(i < files)
> +	{
> +		html("<tr>");
> +		html("<td class='mode'>...</td>");
> +		html("<td class='upd'>...</td>");
> +		html("<td class='right'>...</td>");
> +		html("<td class='graph'>...</td>");
> +		html("</tr>");
> +	}

I wonder if people will miss the fact that the diff stat has been
truncated.  Would it be better to just drop the entire thing if there
are too many changed files and replace it with something like:

	This diff is really big, are you sure you want to see it?

and provide a link that bypasses this limit?

>  	html("</table>");
>  	html("<div class='diffstat-summary'>");
>  	htmlf("%d files changed, %d insertions, %d deletions",
> @@ -302,6 +313,11 @@ static void filepair_cb(struct diff_filepair *pair)
>  	if (!show_filepair(pair))
>  		return;
>  
> +	if(filepair_cnt++ >= ctx.cfg.max_diff_files)
> +	{
> +		return;
> +	}

No need for braces here.

> +
>  	current_filepair = pair;
>  	if (use_ssdiff) {
>  		cgit_ssdiff_header_begin();
> @@ -490,6 +506,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
>  		html("<table summary='diff' class='diff'>");
>  		html("<tr><td>");
>  	}
> +        filepair_cnt = 0;

style: indentation looks wrong here (SP instead of TAB?)

>  	cgit_diff_tree(old_rev_sha1, new_rev_sha1, filepair_cb, prefix,
>  		       ctx.qry.ignorews);
>  	if (!use_ssdiff)
> -- 
> 2.4.9
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH] ui-diff: Add configuration option to limit number of files

[tim.nordell]

On 02/28/16 06:38, John Keeping wrote:
> I wonder if people will miss the fact that the diff stat has been
> truncated.  Would it be better to just drop the entire thing if there
> are too many changed files and replace it with something like:
>
> 	This diff is really big, are you sure you want to see it?
>
> and provide a link that bypasses this limit?
>

Good idea.  What if the diffstat is truncated, and rather than showing 
any diffs, it shows the message above?  It's still sometimes useful to 
see a snippet of what changed even if it's huge.  I suppose maybe the 
"forced" view could be a link to downloading the patch locally?

There are circumstances in which I really don't want an HTML view of the 
full changeset to ever be rendered to someone remotely.  An extreme 
example of this is when the kernel tree was introduced (commit tagged 
v2.6.12-rc2 in any CGIT hosted kernel repo).  I won't link it here, but 
it takes well over 100 seconds to generate on kernel.org's server, and 
content-wise is well over 300MiB since it contains essentially the 
entire source code of kernel v2.6.12-rc2 in HTML form since it was the 
introduction commit to the kernel.

- Tim