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

[John Keeping]

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