[PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions

[PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions

[Johannes Schindelin]

The number of '-' and '+' is still linear. The idea is that
scaled-length := floor(a * length + b) with the following constraints: if 
length == 1, scaled-length == 1, and the combined length of plusses 
and minusses should not be larger than the width by a small margin. Thus,

	a + b == 1

and
	a * max_plusses + b + a * max_minusses + b = width + 1

The solution is

	a * x + b = ((width - 1) * (x - 1) + max_change - 1)
		 / (max_change - 1)

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	While testing this, I hit a bug which was hard to squash: commit
	v1.3.3~14 _always_ showed no minusses and plusses in the diffstat.

	Until I realized that the offending diffstat was in the commit
	_message_ :-)

 diff.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 98c29bf..53c30bd 100644
--- a/diff.c
+++ b/diff.c
@@ -640,9 +640,12 @@ const char mime_boundary_leader[] = "---
 static int scale_linear(int it, int width, int max_change)
 {
 	/*
-	 * round(width * it / max_change);
+	 * make sure that at least one '-' is printed if there were deletions,
+	 * and likewise for '+'.
 	 */
-	return (it * width * 2 + max_change) / (max_change * 2);
+	if (max_change < 2)
+		return it;
+	return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);
 }
 
 static void show_name(const char *prefix, const char *name, int len,
@@ -774,9 +777,9 @@ static void show_stats(struct diffstat_t
 		dels += del;
 
 		if (width <= max_change) {
-			total = scale_linear(total, width, max_change);
 			add = scale_linear(add, width, max_change);
-			del = total - add;
+			del = scale_linear(del, width, max_change);
+			total = add + del;
 		}
 		show_name(prefix, name, len, reset, set);
 		printf("%5d ", added + deleted);
-- 
1.4.2.1.g89d5d-dirty

Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The number of '-' and '+' is still linear. The idea is that
> scaled-length := floor(a * length + b) with the following constraints: if 
> length == 1, scaled-length == 1, and the combined length of plusses 
> and minusses should not be larger than the width by a small margin. Thus,
>
> 	a + b == 1
>
> and
> 	a * max_plusses + b + a * max_minusses + b = width + 1
>
> The solution is
>
> 	a * x + b = ((width - 1) * (x - 1) + max_change - 1)
> 		 / (max_change - 1)
>
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Nice looking math in the log message aside,...

> diff --git a/diff.c b/diff.c
> index 98c29bf..53c30bd 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -640,9 +640,12 @@ const char mime_boundary_leader[] = "---
>  static int scale_linear(int it, int width, int max_change)
>  {
>  	/*
> -	 * round(width * it / max_change);
> +	 * make sure that at least one '-' is printed if there were deletions,
> +	 * and likewise for '+'.
>  	 */
> -	return (it * width * 2 + max_change) / (max_change * 2);
> +	if (max_change < 2)
> +		return it;
> +	return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);
>  }

This does not feel right.  Maybe the first conditional is to see 
if it is less than 2, not max_change?

>  static void show_name(const char *prefix, const char *name, int len,
> @@ -774,9 +777,9 @@ static void show_stats(struct diffstat_t
>  		dels += del;
>  
>  		if (width <= max_change) {
> -			total = scale_linear(total, width, max_change);
>  			add = scale_linear(add, width, max_change);
> -			del = total - add;
> +			del = scale_linear(del, width, max_change);
> +			total = add + del;
>  		}
>  		show_name(prefix, name, len, reset, set);
>  		printf("%5d ", added + deleted);

The original was written a bit differently because it used
round() not floor(), and avoided the case of both ends end up
rounding up.

This version of scale_linear() rounds down so you can get away
with this without busting the total width.

But think about the case where width=7, max_change=10, 5
lines are added and 5 lines are removed.

The original makes total 7 to use full width, and gives 4 pluses
and 3 minuses, which is not quite right when your eyes notice
that they are not of equal length, but it makes it to use the
full width.  I think this version gives 3 pluses and minuses,
and does not use the full width.  However, if you have a file
that adds 10 without removing, it will be drawn as 7 pluses.  In
other words, the line drawn for a new file that is 10 lines
long, and the line for a modified file that added 5 lines and
removed 5 lines, are drawn differently.

I _think_ the graph length is reasonably long enough that the
actual differences coming from rounding (3 vs 4 in the above
description for the current implementation) is less annoying
than the total number of pluses and minuses not lining up for
two files that had both 10 changes, one (add=10,del=0) and the
other (add=5,del=5).  Illustration.

Compute total and add, make del=total-add:

	foo |   10 ++++---
        bar |   10 +++++++

Compute add and del independently:

	foo |   10 +++---
        bar |   10 +++++++

So I'd suggest either force the width to even number (if odd
drop one), of keep the current del=total-add.

How about doing something like this instead?

 - first scale the total but make sure there is one column for
   each of non-zero add and delete;

 - scale add but make sure it is at least one if non-zero;

 - del is the remainder of total after add is taken, but if
   there are too many adds than dels, that can become zero.  In
   that case adjust by giving one column from add to del.

diff --git a/diff.c b/diff.c
index 3fd7a52..9b9a6d8 100644
--- a/diff.c
+++ b/diff.c
@@ -684,9 +684,15 @@ static void show_stats(struct diffstat_t
 		dels += del;
 
 		if (width <= max_change) {
-			total = scale_linear(total, width, max_change);
-			add = scale_linear(add, width, max_change);
+			int fl = !!add + !!del;
+			total = scale_linear(total, width-fl, max_change) + fl;
+			if (add)
+				add = scale_linear(add, width-fl, max_change) + 1;
 			del = total - add;
+			if (!del && deleted) {
+				add--;
+				del++;
+			}
 		}
 		show_name(prefix, name, len, reset, set);
 		printf("%5d ", added + deleted);

Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions

[Johannes Schindelin]

Hi,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/diff.c b/diff.c
> > index 98c29bf..53c30bd 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -640,9 +640,12 @@ const char mime_boundary_leader[] = "---
> >  static int scale_linear(int it, int width, int max_change)
> >  {
> >  	/*
> > -	 * round(width * it / max_change);
> > +	 * make sure that at least one '-' is printed if there were deletions,
> > +	 * and likewise for '+'.
> >  	 */
> > -	return (it * width * 2 + max_change) / (max_change * 2);
> > +	if (max_change < 2)
> > +		return it;
> > +	return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);
> >  }
> 
> This does not feel right.  Maybe the first conditional is to see 
> if it is less than 2, not max_change?

No, it is very much a mathematical constraint: we are going to divide by 
(max_change - 1), which is 0 or -1 in said cases. In _both_ cases, "it" 
happens to be what we actually want. (Okay, in reality we should check for 
width > 0, but I think width == 0 would not make _any_ sense.)

> But think about the case where width=7, max_change=10, 5
> lines are added and 5 lines are removed.
> 
> The original makes total 7 to use full width, and gives 4 pluses
> and 3 minuses, which is not quite right when your eyes notice
> that they are not of equal length, but it makes it to use the
> full width.  I think this version gives 3 pluses and minuses,
> and does not use the full width.

That is right.

However, I would argue it's actually an improvement. If you have as many 
additions as deletions, there should be an equal number of '-' and '+'.

After all, the integer number of symbols is just an approximation anyway.

Ciao,
Dscho

Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> However, I would argue it's actually an improvement. If you have as many 
> additions as deletions, there should be an equal number of '-' and '+'.

That's one way to look at it.

Now how would you explain that a file that had 5 adds and 5
deletes gets three pluses and three minuses while another file
that had 10 adds and no delets gets seven pluses?

Compute total and add, make del=total-add:

	foo |   10 ++++---
        bar |   10 +++++++

Compute add and del independently:

	foo |   10 +++---
        bar |   10 +++++++

> After all, the integer number of symbols is just an approximation anyway.

Yes, and my guess is that the graph is usually much wider than 7
columns as depicted above, so 3 vs 4 inconsistency in the former
is less noticeable than 6 vs 7 inconsistency in the latter to
the eye.  If we have to make a compromise, I think getting the
lines for the files that have the same number of changes to line
up at the right end would be more important.

Another way is what I suggested earlier -- if the width is odd,
drop one to avoid this problem altogether.  That would also be
acceptable and probably be more consistent.

Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions

[Johannes Schindelin]

Hi,

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

> Another way is what I suggested earlier -- if the width is odd, drop one 
> to avoid this problem altogether.  That would also be acceptable and 
> probably be more consistent.

I see that you care deeply about the 7 column example. In that case, yes, 
I would rather have columns be rounded down to the next even number.

I care more about the consistency: if there are changes, I want to see 
them, and it should be linear: the more changes, the more plusses or 
minusses.

Ciao,
Dscho

P.S.: here's a patch on top of my last one:
-- snip --
[PATCH] force even diffstat width

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 diff.c |    3 ++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 53c30bd..2a898e6 100644
--- a/diff.c
+++ b/diff.c
@@ -729,6 +729,9 @@ static void show_stats(struct diffstat_t
 	else
 		width = max_change;
 
+	/* force width to be even */
+	width &= ~1;
+
 	for (i = 0; i < data->nr; i++) {
 		const char *prefix = "";
 		char *name = data->files[i]->name;
-- 
1.4.2.1.g1a99e1-dirty