[PATCH 1/3] diff --stat: allow custom diffstat output width.

[PATCH 1/3] diff --stat: allow custom diffstat output width.

[Junio C Hamano]

This adds two parameters to "diff --stat".

 . --stat-width=72 tells that the page should fit on 72-column output.

 . --stat-name-width=30 tells that the filename part is limited
   to 30 columns.

Without these flags, they default to 80 and 50 as before.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * Adrian Bunk on the kernel list wanted ot have "diffstat
   -w72", so here it is.

 diff.c |  132 ++++++++++++++++++++++++++++++++++++++++++++++------------------
 diff.h |    3 +
 2 files changed, 97 insertions(+), 38 deletions(-)

diff --git a/diff.c b/diff.c
index 443e248..6101365 100644
--- a/diff.c
+++ b/diff.c
@@ -545,21 +545,63 @@ static void diffstat_consume(void *priv,
 		x->deleted++;
 }
 
-static const char pluses[] = "++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++";
-static const char minuses[]= "----------------------------------------------------------------------";
 const char mime_boundary_leader[] = "------------";
 
-static void show_stats(struct diffstat_t* data)
+static int scale_linear(int it, int width, int max_change)
+{
+	/*
+	 * round(width * it / max_change);
+	 */
+	return (it * width * 2 + max_change) / (max_change * 2);
+}
+
+static void show_name(const char *prefix, const char *name, int len)
+{
+	printf(" %s%-*s |", prefix, len, name);
+}
+
+static void show_graph(char ch, int cnt)
+{
+	if (!cnt)
+		return;
+	while (cnt--)
+		putchar(ch);
+}
+
+static void show_stats(struct diffstat_t* data, struct diff_options *options)
 {
 	int i, len, add, del, total, adds = 0, dels = 0;
-	int max, max_change = 0, max_len = 0;
+	int max_change = 0, max_len = 0;
 	int total_files = data->nr;
+	int width, name_width;
 
 	if (data->nr == 0)
 		return;
 
+	width = options->stat_width ? options->stat_width : 80;
+	name_width = options->stat_name_width ? options->stat_name_width : 50;
+
+	/* Sanity: give at least 5 columns to the graph,
+	 * but leave at least 10 columns for the name.
+	 */
+	if (width < name_width + 15) {
+		if (25 < name_width)
+			name_width = width - 15;
+		else
+			width = name_width + 15;
+	}
+
+	/* Find the longest filename and max number of changes */
 	for (i = 0; i < data->nr; i++) {
 		struct diffstat_file *file = data->files[i];
+		int change = file->added + file->deleted;
+
+		if (0 < (len = quote_c_style(file->name, NULL, NULL, 0))) {
+			char *qname = xmalloc(len + 1);
+			quote_c_style(file->name, qname, NULL, 0);
+			free(file->name);
+			file->name = qname;
+		}
 
 		len = strlen(file->name);
 		if (max_len < len)
@@ -567,54 +609,53 @@ static void show_stats(struct diffstat_t
 
 		if (file->is_binary || file->is_unmerged)
 			continue;
-		if (max_change < file->added + file->deleted)
-			max_change = file->added + file->deleted;
+		if (max_change < change)
+			max_change = change;
 	}
 
+	/* Compute the width of the graph part;
+	 * 10 is for one blank at the beginning of the line plus
+	 * " | count " between the name and the graph.
+	 *
+	 * From here on, name_width is the width of the name area,
+	 * and width is the width of the graph area.
+	 */
+	name_width = (name_width < max_len) ? name_width : max_len;
+	if (width < (name_width + 10) + max_change)
+		width = width - (name_width + 10);
+	else
+		width = max_change;
+
 	for (i = 0; i < data->nr; i++) {
 		const char *prefix = "";
 		char *name = data->files[i]->name;
 		int added = data->files[i]->added;
 		int deleted = data->files[i]->deleted;
-
-		if (0 < (len = quote_c_style(name, NULL, NULL, 0))) {
-			char *qname = xmalloc(len + 1);
-			quote_c_style(name, qname, NULL, 0);
-			free(name);
-			data->files[i]->name = name = qname;
-		}
+		int name_len;
 
 		/*
 		 * "scale" the filename
 		 */
-		len = strlen(name);
-		max = max_len;
-		if (max > 50)
-			max = 50;
-		if (len > max) {
+		len = name_width;
+		name_len = strlen(name);
+		if (name_width < name_len) {
 			char *slash;
 			prefix = "...";
-			max -= 3;
-			name += len - max;
+			len -= 3;
+			name += name_len - len;
 			slash = strchr(name, '/');
 			if (slash)
 				name = slash;
 		}
-		len = max;
-
-		/*
-		 * scale the add/delete
-		 */
-		max = max_change;
-		if (max + len > 70)
-			max = 70 - len;
 
 		if (data->files[i]->is_binary) {
-			printf(" %s%-*s |  Bin\n", prefix, len, name);
+			show_name(prefix, name, len);
+			printf("  Bin\n");
 			goto free_diffstat_file;
 		}
 		else if (data->files[i]->is_unmerged) {
-			printf(" %s%-*s |  Unmerged\n", prefix, len, name);
+			show_name(prefix, name, len);
+			printf("  Unmerged\n");
 			goto free_diffstat_file;
 		}
 		else if (!data->files[i]->is_renamed &&
@@ -623,27 +664,34 @@ static void show_stats(struct diffstat_t
 			goto free_diffstat_file;
 		}
 
+		/*
+		 * scale the add/delete
+		 */
 		add = added;
 		del = deleted;
 		total = add + del;
 		adds += add;
 		dels += del;
 
-		if (max_change > 0) {
-			total = (total * max + max_change / 2) / max_change;
-			add = (add * max + max_change / 2) / max_change;
+		if (max_change < width)
+			;
+		else {
+			total = scale_linear(total, width, max_change);
+			add = scale_linear(add, width, max_change);
 			del = total - add;
 		}
-		printf(" %s%-*s |%5d %.*s%.*s\n", prefix,
-				len, name, added + deleted,
-				add, pluses, del, minuses);
+		show_name(prefix, name, len);
+		printf("%5d ", added + deleted);
+		show_graph('+', add);
+		show_graph('-', del);
+		putchar('\n');
 	free_diffstat_file:
 		free(data->files[i]->name);
 		free(data->files[i]);
 	}
 	free(data->files);
 	printf(" %d files changed, %d insertions(+), %d deletions(-)\n",
-			total_files, adds, dels);
+	       total_files, adds, dels);
 }
 
 struct checkdiff_t {
@@ -1681,6 +1729,14 @@ int diff_opt_parse(struct diff_options *
 	}
 	else if (!strcmp(arg, "--stat"))
 		options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	else if (!strncmp(arg, "--stat-width=", 13)) {
+		options->stat_width = strtoul(arg + 13, NULL, 10);
+		options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	}
+	else if (!strncmp(arg, "--stat-name-width=", 18)) {
+		options->stat_name_width = strtoul(arg + 18, NULL, 10);
+		options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	}
 	else if (!strcmp(arg, "--check"))
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))
@@ -2438,7 +2494,7 @@ void diff_flush(struct diff_options *opt
 			if (check_pair_status(p))
 				diff_flush_stat(p, options, &diffstat);
 		}
-		show_stats(&diffstat);
+		show_stats(&diffstat, options);
 		separator++;
 	}
 
diff --git a/diff.h b/diff.h
index b60a02e..e06d0f4 100644
--- a/diff.h
+++ b/diff.h
@@ -69,6 +69,9 @@ struct diff_options {
 	const char *stat_sep;
 	long xdl_opts;
 
+	int stat_width;
+	int stat_name_width;
+
 	int nr_paths;
 	const char **paths;
 	int *pathlens;
-- 
1.4.2.1.gf80a

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[David Rientjes]

On Tue, 26 Sep 2006, Junio C Hamano wrote:

> +static void show_graph(char ch, int cnt)
> +{
> +	if (!cnt)
> +		return;
> +	while (cnt--)
> +		putchar(ch);
> +}
> +

'if (cnt <= 0)' or 'while (cnt-- > 0)' is a better API./

> +static void show_stats(struct diffstat_t* data, struct diff_options *options)
>  {
>  	int i, len, add, del, total, adds = 0, dels = 0;
> -	int max, max_change = 0, max_len = 0;
> +	int max_change = 0, max_len = 0;
>  	int total_files = data->nr;
> +	int width, name_width;
>  
>  	if (data->nr == 0)
>  		return;
>  
> +	width = options->stat_width ? options->stat_width : 80;
> +	name_width = options->stat_name_width ? options->stat_name_width : 50;
> +
> +	/* Sanity: give at least 5 columns to the graph,
> +	 * but leave at least 10 columns for the name.
> +	 */
> +	if (width < name_width + 15) {
> +		if (25 < name_width)
> +			name_width = width - 15;
> +		else
> +			width = name_width + 15;
> +	}
> +

Constants go on the right side of comparison expressions.

> +	/* Find the longest filename and max number of changes */
>  	for (i = 0; i < data->nr; i++) {
>  		struct diffstat_file *file = data->files[i];
> +		int change = file->added + file->deleted;
> +
> +		if (0 < (len = quote_c_style(file->name, NULL, NULL, 0))) {
> +			char *qname = xmalloc(len + 1);
> +			quote_c_style(file->name, qname, NULL, 0);
> +			free(file->name);
> +			file->name = qname;
> +		}

Same.

> @@ -623,27 +664,34 @@ static void show_stats(struct diffstat_t
>  			goto free_diffstat_file;
>  		}
>  
> +		/*
> +		 * scale the add/delete
> +		 */
>  		add = added;
>  		del = deleted;
>  		total = add + del;
>  		adds += add;
>  		dels += del;
>  
> -		if (max_change > 0) {
> -			total = (total * max + max_change / 2) / max_change;
> -			add = (add * max + max_change / 2) / max_change;
> +		if (max_change < width)
> +			;
> +		else {
> +			total = scale_linear(total, width, max_change);
> +			add = scale_linear(add, width, max_change);
>  			del = total - add;
>  		}

if (max_change >= width)

> diff --git a/diff.h b/diff.h
> index b60a02e..e06d0f4 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -69,6 +69,9 @@ struct diff_options {
>  	const char *stat_sep;
>  	long xdl_opts;
>  
> +	int stat_width;
> +	int stat_name_width;
> +

Can you use unsigned char here instead?

		David

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Linus Torvalds]

On Tue, 26 Sep 2006, Junio C Hamano wrote:
>
> This adds two parameters to "diff --stat".
> 
>  . --stat-width=72 tells that the page should fit on 72-column output.
> 
>  . --stat-name-width=30 tells that the filename part is limited
>    to 30 columns.

Thinking some more about this, I have to say, I do hate the syntax.

It may be clear thanks to being verbose, but it's _hell_ to write.

It has the same problem the "--stat-with-patch" argument had: sure, it 
worked, but it was really really inconvenient, and just doing a 
combination of "--stat -p" is much nicer.

So how about just extending the existing "--stat" thing, and just making 
it do something like

	git diff --stat=72,30

instead (perhaps along with a config option to set the defaults to 
something else if we want to).

What do you think?

I'm just pretty sure I will never _ever_ bother to type 
--stat-name-width=30 in my life except right now to complain about it. I 
just can't see myself ever caring _that_ much. But "--stat=100" to see a 
wider stat, that I could see myself doing. Or "--stat=100,100" if I want 
to see long filenames too.

		Linus

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> So how about just extending the existing "--stat" thing, and just making 
> it do something like
>
> 	git diff --stat=72,30
>
> instead (perhaps along with a config option to set the defaults to 
> something else if we want to).
>
> What do you think?

I am all for it.

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Linus Torvalds]

On Thu, 28 Sep 2006, Junio C Hamano wrote:

> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > So how about just extending the existing "--stat" thing, and just making 
> > it do something like
> >
> > 	git diff --stat=72,30
> >
> > instead (perhaps along with a config option to set the defaults to 
> > something else if we want to).
> >
> > What do you think?
> 
> I am all for it.

Here. Something like this.

You should probably check the "width" and "name_width" values for sanity. 
The old code didn't, so I didn't do it, but this actually does check that 
the number was a real number (and didn't have crap after it). And it's 
easy to check the values, since all cases go through the same point.

I could have made it a more obvious "stupid" parser, I just think it's 
better to do it this way.

		Linus

----
diff --git a/diff.c b/diff.c
index 98c29bf..8546dde 100644
--- a/diff.c
+++ b/diff.c
@@ -1825,15 +1825,33 @@ int diff_opt_parse(struct diff_options *
 	else if (!strcmp(arg, "--patch-with-raw")) {
 		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;
 	}
-	else if (!strcmp(arg, "--stat"))
-		options->output_format |= DIFF_FORMAT_DIFFSTAT;
-	else if (!strncmp(arg, "--stat-width=", 13)) {
-		options->stat_width = strtoul(arg + 13, NULL, 10);
-		options->output_format |= DIFF_FORMAT_DIFFSTAT;
-	}
-	else if (!strncmp(arg, "--stat-name-width=", 18)) {
-		options->stat_name_width = strtoul(arg + 18, NULL, 10);
+	else if (!strncmp(arg, "--stat", 6)) {
+		char *end;
+		int width = options->stat_width;
+		int name_width = options->stat_name_width;
+		arg += 6;
+		end = arg;
+
+		switch (*arg) {
+		case '-':
+			if (!strncmp(arg, "-width=", 7))
+				width = strtoul(arg + 7, &end, 10);
+			else if (!strncmp(arg, "-name-width=", 12))
+				name_width = strtoul(arg + 12, &end, 10);
+			break;
+
+		case '=':
+			width = strtoul(arg+1, &end, 10);
+			if (*end == ',')
+				name_width = strtoul(end+1, &end, 10);
+		}
+
+		/* Important! This checks all the error cases! */
+		if (*end)
+			return 0;
 		options->output_format |= DIFF_FORMAT_DIFFSTAT;
+		options->stat_name_width = name_width;
+		options->stat_width = width;
 	}
 	else if (!strcmp(arg, "--check"))
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Adrian Bunk]

On Thu, Sep 28, 2006 at 01:54:27PM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 26 Sep 2006, Junio C Hamano wrote:
> >
> > This adds two parameters to "diff --stat".
> > 
> >  . --stat-width=72 tells that the page should fit on 72-column output.
> > 
> >  . --stat-name-width=30 tells that the filename part is limited
> >    to 30 columns.
> 
> Thinking some more about this, I have to say, I do hate the syntax.
> 
> It may be clear thanks to being verbose, but it's _hell_ to write.
> 
> It has the same problem the "--stat-with-patch" argument had: sure, it 
> worked, but it was really really inconvenient, and just doing a 
> combination of "--stat -p" is much nicer.
> 
> So how about just extending the existing "--stat" thing, and just making 
> it do something like
> 
> 	git diff --stat=72,30
> 
> instead (perhaps along with a config option to set the defaults to 
> something else if we want to).
> 
> What do you think?
>...

What about staying compatible with diffstat?

IOW, change --stat-width=72 to -w72, or at least allow it alternatively?

> 		Linus

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Johannes Schindelin]

Hi,

On Fri, 29 Sep 2006, Adrian Bunk wrote:

> IOW, change --stat-width=72 to -w72, or at least allow it alternatively?

With diff options, -w is identical to --ignore-all-space. (Yes, I know, it 
is not in Documentation/diff-options.txt, but it is in "man diff".)

Ciao,
Dscho

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> You should probably check the "width" and "name_width" values for sanity.

The output code forces (well, at least tries to force) some
sanity into these values when they are not, so probably we do
not need it.  If we want, we could check something like:

    	if (name_width < 10 || width - name_width < 15)
		die("at least 10 for name, and 15+name for width please");

The number 10 is totally arbitrary; among the total area that is
given by width, after subtracting name_width, 10 columns are
taken by left and right end space, vertical bar and numbers, so
the above gives at least 5 columns to draw the graph.

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> I could have made it a more obvious "stupid" parser, I just think it's 
> better to do it this way.
>
> +	else if (!strncmp(arg, "--stat", 6)) {
> +		char *end;
> +		int width = options->stat_width;
> +		int name_width = options->stat_name_width;
> +		arg += 6;
> +		end = arg;
> +
> +		switch (*arg) {
> +		case '-':
> +			if (!strncmp(arg, "-width=", 7))
> +				width = strtoul(arg + 7, &end, 10);
> +			else if (!strncmp(arg, "-name-width=", 12))
> +				name_width = strtoul(arg + 12, &end, 10);
> +			break;
> +
> +		case '=':
> +			width = strtoul(arg+1, &end, 10);
> +			if (*end == ',')
> +				name_width = strtoul(end+1, &end, 10);
> +		}
> +
> +		/* Important! This checks all the error cases! */
> +		if (*end)
> +			return 0;
>  		options->output_format |= DIFF_FORMAT_DIFFSTAT;
> +		options->stat_name_width = name_width;
> +		options->stat_width = width;
>  	}

This is simply too clever; -pedantic does not like assignment of
arg to end (constness -- and strtoul takes pointer to non-const
char *, so making the type of end const char * is not an answer
either).

And I do not like casting constness away: end = (char *) arg.

Hmmmm.

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Linus Torvalds]

On Thu, 28 Sep 2006, Junio C Hamano wrote:
> 
> This is simply too clever; -pedantic does not like assignment of
> arg to end (constness -- and strtoul takes pointer to non-const
> char *, so making the type of end const char * is not an answer
> either).

The _code_ really is right. The problem is "strtoul()" interfaces and a 
C typing oddity.

> And I do not like casting constness away: end = (char *) arg.

You could fix it by doing something like this:

	static inline unsigned long sane_strtoul(const char *n,
						 const char **p,
						 int base)
	{
		char *end;
		unsigned long res;

		res = strtoul(n, &end, base);
		*p = end;
		return res;
	}

because the only reason strtoul() warns now is that C type-rules don't 
allow the (obviously safe - but pointers migth have strange 
representations) conversion of "char **" into "const char **", even though 
"char *" can be converted into "const char *".

At that point, the cast is probably simpler, but the above should be 
strictly correct pedantic ANSI C.

I didn't even try it, though.

			Linus

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 28 Sep 2006, Junio C Hamano wrote:
>> 
>> This is simply too clever; -pedantic does not like assignment of
>> arg to end (constness -- and strtoul takes pointer to non-const
>> char *, so making the type of end const char * is not an answer
>> either).
>
> The _code_ really is right. The problem is "strtoul()" interfaces and a 
> C typing oddity.

Yes, the code is right (not only because you wrote it ;-) and
not just right but it is crystal clear to humans what is going
on and why the final "if (*end)" is all that is needed without
any "return 0" in the middle.

> At that point, the cast is probably simpler,...

... which I ended up doing anyway.

Thanks.

Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.

[Linus Torvalds]

On Thu, 28 Sep 2006, Linus Torvalds wrote:
> 
> because the only reason strtoul() warns now is that C type-rules don't 
> allow the (obviously safe - but pointers migth have strange 
> representations) conversion of "char **" into "const char **", even though 
> "char *" can be converted into "const char *".

I phrased that badly. 

IF C pointer conversion allowed implicit addition of "const" past the 
top-most level, ANSI C would have just done "strtoul()" as 

	unsigned long strtoul(const char *n, const char **p, int);

ie they could just have added the "const" not just to the first argument, 
and legact programs (without const) would still have worked fine.

But _because_ that's not how C type rules work, we have the current 
situation where the first argument is a "const char *", and the second 
argument _logically_ should be a pointer to such an entity, but because 
that would have caused bogus warnings for any code that just used a 
regular "char **" without any const at all, that wasn't an option.

So that explains why ANSI C has insane imbalances like this. It's easy to 
add a "const" to a _first-level_ pointer to say "we allow both const and 
regular pointers to this thing", but sadly you can't do it for a pointer 
to such a pointer.

			Linus