{"thread":{"id":"5736","subject":"[PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions","startedAt":"2006-09-28T15:37:39Z","lastAt":"2006-09-29T04:07:26Z","messageCount":5,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"27891","messageId":"Pine.LNX.4.63.0609281735040.14200@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5736","inReplyTo":null,"subject":"[PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-09-28T15:37:39Z","receivedAt":"2006-09-28T15:37:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe number of '-' and '+' is still linear. The idea is that\nscaled-length := floor(a * length + b) with the following constraints: if \nlength == 1, scaled-length == 1, and the combined length of plusses \nand minusses should not be larger than the width by a small margin. Thus,\n\n\ta + b == 1\n\nand\n\ta * max_plusses + b + a * max_minusses + b = width + 1\n\nThe solution is\n\n\ta * x + b = ((width - 1) * (x - 1) + max_change - 1)\n\t\t / (max_change - 1)\n\nSigned-off-by: Johannes Schindelin \n\n---\n\n\tWhile testing this, I hit a bug which was hard to squash: commit\n\tv1.3.3~14 _always_ showed no minusses and plusses in the diffstat.\n\n\tUntil I realized that the offending diffstat was in the commit\n\t_message_ :-)\n\n diff.c | 11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 98c29bf..53c30bd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -640,9 +640,12 @@ const char mime_boundary_leader[] = \"---\n static int scale_linear(int it, int width, int max_change)\n {\n \t/*\n-\t * round(width * it / max_change);\n+\t * make sure that at least one '-' is printed if there were deletions,\n+\t * and likewise for '+'.\n \t */\n-\treturn (it * width * 2 + max_change) / (max_change * 2);\n+\tif (max_change < 2)\n+\t\treturn it;\n+\treturn ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n }\n \n static void show_name(const char *prefix, const char *name, int len,\n@@ -774,9 +777,9 @@ static void show_stats(struct diffstat_t\n \t\tdels += del;\n \n \t\tif (width <= max_change) {\n-\t\t\ttotal = scale_linear(total, width, max_change);\n \t\t\tadd = scale_linear(add, width, max_change);\n-\t\t\tdel = total - add;\n+\t\t\tdel = scale_linear(del, width, max_change);\n+\t\t\ttotal = add + del;\n \t\t}\n \t\tshow_name(prefix, name, len, reset, set);\n \t\tprintf(\"%5d \", added + deleted);\n-- \n1.4.2.1.g89d5d-dirty\n"},{"id":"27905","messageId":"7v1wpvvrww.fsf@assigned-by-dhcp.cox.net","threadId":"5736","inReplyTo":"Pine.LNX.4.63.0609281735040.14200@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-28T17:10:07Z","receivedAt":"2006-09-28T17:10:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The number of '-' and '+' is still linear. The idea is that\n> scaled-length := floor(a * length + b) with the following constraints: if \n> length == 1, scaled-length == 1, and the combined length of plusses \n> and minusses should not be larger than the width by a small margin. Thus,\n>\n> \ta + b == 1\n>\n> and\n> \ta * max_plusses + b + a * max_minusses + b = width + 1\n>\n> The solution is\n>\n> \ta * x + b = ((width - 1) * (x - 1) + max_change - 1)\n> \t\t / (max_change - 1)\n>\n> Signed-off-by: Johannes Schindelin \n\nNice looking math in the log message aside,...\n\n> diff --git a/diff.c b/diff.c\n> index 98c29bf..53c30bd 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -640,9 +640,12 @@ const char mime_boundary_leader[] = \"---\n> static int scale_linear(int it, int width, int max_change)\n> {\n> \t/*\n> -\t * round(width * it / max_change);\n> +\t * make sure that at least one '-' is printed if there were deletions,\n> +\t * and likewise for '+'.\n> \t */\n> -\treturn (it * width * 2 + max_change) / (max_change * 2);\n> +\tif (max_change < 2)\n> +\t\treturn it;\n> +\treturn ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n> }\n\nThis does not feel right. Maybe the first conditional is to see \nif it is less than 2, not max_change?\n\n> static void show_name(const char *prefix, const char *name, int len,\n> @@ -774,9 +777,9 @@ static void show_stats(struct diffstat_t\n> \t\tdels += del;\n> \n> \t\tif (width <= max_change) {\n> -\t\t\ttotal = scale_linear(total, width, max_change);\n> \t\t\tadd = scale_linear(add, width, max_change);\n> -\t\t\tdel = total - add;\n> +\t\t\tdel = scale_linear(del, width, max_change);\n> +\t\t\ttotal = add + del;\n> \t\t}\n> \t\tshow_name(prefix, name, len, reset, set);\n> \t\tprintf(\"%5d \", added + deleted);\n\nThe original was written a bit differently because it used\nround() not floor(), and avoided the case of both ends end up\nrounding up.\n\nThis version of scale_linear() rounds down so you can get away\nwith this without busting the total width.\n\nBut think about the case where width=7, max_change=10, 5\nlines are added and 5 lines are removed.\n\nThe original makes total 7 to use full width, and gives 4 pluses\nand 3 minuses, which is not quite right when your eyes notice\nthat they are not of equal length, but it makes it to use the\nfull width. I think this version gives 3 pluses and minuses,\nand does not use the full width. However, if you have a file\nthat adds 10 without removing, it will be drawn as 7 pluses. In\nother words, the line drawn for a new file that is 10 lines\nlong, and the line for a modified file that added 5 lines and\nremoved 5 lines, are drawn differently.\n\nI _think_ the graph length is reasonably long enough that the\nactual differences coming from rounding (3 vs 4 in the above\ndescription for the current implementation) is less annoying\nthan the total number of pluses and minuses not lining up for\ntwo files that had both 10 changes, one (add=10,del=0) and the\nother (add=5,del=5). Illustration.\n\nCompute total and add, make del=total-add:\n\n\tfoo | 10 ++++---\n bar | 10 +++++++\n\nCompute add and del independently:\n\n\tfoo | 10 +++---\n bar | 10 +++++++\n\nSo I'd suggest either force the width to even number (if odd\ndrop one), of keep the current del=total-add.\n\nHow about doing something like this instead?\n\n - first scale the total but make sure there is one column for\n each of non-zero add and delete;\n\n - scale add but make sure it is at least one if non-zero;\n\n - del is the remainder of total after add is taken, but if\n there are too many adds than dels, that can become zero. In\n that case adjust by giving one column from add to del.\n\ndiff --git a/diff.c b/diff.c\nindex 3fd7a52..9b9a6d8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -684,9 +684,15 @@ static void show_stats(struct diffstat_t\n \t\tdels += del;\n \n \t\tif (width <= max_change) {\n-\t\t\ttotal = scale_linear(total, width, max_change);\n-\t\t\tadd = scale_linear(add, width, max_change);\n+\t\t\tint fl = !!add + !!del;\n+\t\t\ttotal = scale_linear(total, width-fl, max_change) + fl;\n+\t\t\tif (add)\n+\t\t\t\tadd = scale_linear(add, width-fl, max_change) + 1;\n \t\t\tdel = total - add;\n+\t\t\tif (!del && deleted) {\n+\t\t\t\tadd--;\n+\t\t\t\tdel++;\n+\t\t\t}\n \t\t}\n \t\tshow_name(prefix, name, len, reset, set);\n \t\tprintf(\"%5d \", added + deleted);\n"},{"id":"27921","messageId":"Pine.LNX.4.63.0609281942360.14200@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5736","inReplyTo":"7v1wpvvrww.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-09-28T19:14:40Z","receivedAt":"2006-09-28T19:14:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 28 Sep 2006, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/diff.c b/diff.c\n> > index 98c29bf..53c30bd 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -640,9 +640,12 @@ const char mime_boundary_leader[] = \"---\n> > static int scale_linear(int it, int width, int max_change)\n> > {\n> > \t/*\n> > -\t * round(width * it / max_change);\n> > +\t * make sure that at least one '-' is printed if there were deletions,\n> > +\t * and likewise for '+'.\n> > \t */\n> > -\treturn (it * width * 2 + max_change) / (max_change * 2);\n> > +\tif (max_change < 2)\n> > +\t\treturn it;\n> > +\treturn ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n> > }\n> \n> This does not feel right. Maybe the first conditional is to see \n> if it is less than 2, not max_change?\n\nNo, it is very much a mathematical constraint: we are going to divide by \n(max_change - 1), which is 0 or -1 in said cases. In _both_ cases, \"it\" \nhappens to be what we actually want. (Okay, in reality we should check for \nwidth > 0, but I think width == 0 would not make _any_ sense.)\n\n> But think about the case where width=7, max_change=10, 5\n> lines are added and 5 lines are removed.\n> \n> The original makes total 7 to use full width, and gives 4 pluses\n> and 3 minuses, which is not quite right when your eyes notice\n> that they are not of equal length, but it makes it to use the\n> full width. I think this version gives 3 pluses and minuses,\n> and does not use the full width.\n\nThat is right.\n\nHowever, I would argue it's actually an improvement. If you have as many \nadditions as deletions, there should be an equal number of '-' and '+'.\n\nAfter all, the integer number of symbols is just an approximation anyway.\n\nCiao,\nDscho\n"},{"id":"27967","messageId":"7vd59fquc5.fsf@assigned-by-dhcp.cox.net","threadId":"5736","inReplyTo":"Pine.LNX.4.63.0609281942360.14200@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-29T02:28:58Z","receivedAt":"2006-09-29T02:28:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> However, I would argue it's actually an improvement. If you have as many \n> additions as deletions, there should be an equal number of '-' and '+'.\n\nThat's one way to look at it.\n\nNow how would you explain that a file that had 5 adds and 5\ndeletes gets three pluses and three minuses while another file\nthat had 10 adds and no delets gets seven pluses?\n\nCompute total and add, make del=total-add:\n\n\tfoo | 10 ++++---\n bar | 10 +++++++\n\nCompute add and del independently:\n\n\tfoo | 10 +++---\n bar | 10 +++++++\n\n> After all, the integer number of symbols is just an approximation anyway.\n\nYes, and my guess is that the graph is usually much wider than 7\ncolumns as depicted above, so 3 vs 4 inconsistency in the former\nis less noticeable than 6 vs 7 inconsistency in the latter to\nthe eye. If we have to make a compromise, I think getting the\nlines for the files that have the same number of changes to line\nup at the right end would be more important.\n\nAnother way is what I suggested earlier -- if the width is odd,\ndrop one to avoid this problem altogether. That would also be\nacceptable and probably be more consistent.\n"},{"id":"27971","messageId":"Pine.LNX.4.63.0609290551510.14200@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5736","inReplyTo":"7vd59fquc5.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] --stat: ensure at least one '-' for deletions, and one '+' for additions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-09-29T04:07:26Z","receivedAt":"2006-09-29T04:07:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 28 Sep 2006, Junio C Hamano wrote:\n\n> Another way is what I suggested earlier -- if the width is odd, drop one \n> to avoid this problem altogether. That would also be acceptable and \n> probably be more consistent.\n\nI see that you care deeply about the 7 column example. In that case, yes, \nI would rather have columns be rounded down to the next even number.\n\nI care more about the consistency: if there are changes, I want to see \nthem, and it should be linear: the more changes, the more plusses or \nminusses.\n\nCiao,\nDscho\n\nP.S.: here's a patch on top of my last one:\n-- snip --\n[PATCH] force even diffstat width\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 3 ++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 53c30bd..2a898e6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -729,6 +729,9 @@ static void show_stats(struct diffstat_t\n \telse\n \t\twidth = max_change;\n \n+\t/* force width to be even */\n+\twidth &= ~1;\n+\n \tfor (i = 0; i < data->nr; i++) {\n \t\tconst char *prefix = \"\";\n \t\tchar *name = data->files[i]->name;\n-- \n1.4.2.1.g1a99e1-dirty\n"}]}