{"thread":{"id":"29619","subject":"diff --stat","startedAt":"2012-02-14T00:11:46Z","lastAt":"2012-02-14T22:06:26Z","messageCount":7,"participants":["Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"184627","messageId":"7v4nuub7el.fsf@alter.siamese.dyndns.org","threadId":"29619","inReplyTo":null,"subject":"diff --stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T00:11:46Z","receivedAt":"2012-02-14T00:11:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hrm, what is wrong with this picture?\n\n$ git -c diff.color.old=red show --format='%s' --stat\nMerge branch 'jk/diff-highlight' into pu\n\n contrib/diff-highlight/README | 109 ++++++++++++++++++++++++++++++--\n contrib/diff-highlight/diff-highlight | 109 ++++++++++++++++++++++++---------\n 2 files changed, 181 insertions(+), 37 deletions(-)\n\nThey both have 109 lines changed but the end of the graph lines do not\ncoincide...\n"},{"id":"184676","messageId":"20120214195036.GD12072@sigill.intra.peff.net","threadId":"29619","inReplyTo":"7v4nuub7el.fsf@alter.siamese.dyndns.org","subject":"Re: diff --stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T19:50:36Z","receivedAt":"2012-02-14T19:50:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2012 at 04:11:46PM -0800, Junio C Hamano wrote:\n\n> Hrm, what is wrong with this picture?\n> \n> $ git -c diff.color.old=red show --format='%s' --stat\n> Merge branch 'jk/diff-highlight' into pu\n> \n> contrib/diff-highlight/README | 109 ++++++++++++++++++++++++++++++--\n> contrib/diff-highlight/diff-highlight | 109 ++++++++++++++++++++++++---------\n> 2 files changed, 181 insertions(+), 37 deletions(-)\n> \n> They both have 109 lines changed but the end of the graph lines do not\n> coincide...\n\nI think it is rounding error. The first one is +102/-7, and the second\none is +79/-30. When we get to scale_linear, we try to scale 109 change\nmarkers into a 33-character width. So the right scaling factor is ~.303.\nSo our \"true\" scaled widths should be:\n\n README, added: 30.9\n README, deleted: 2.1\n highlight, added: 23.9\n highlight, deleted: 9.1\n\nHowever, we're dealing with integer numbers of characters, so we need to\nround. In this case, it seems that our rounding produces (30, 2) in the\nfirst instance and (24, 9) in the second. Which is odd. You'd think\nwe'd either always round to the nearest integer, or always round down.\nBut we end up rounding 30.9 down and 23.9 up. So it may be a subtle\nloss-of-precision error in scale_linear.\n\nHmm. Looking at scale_linear, the formula is:\n\n return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n\nI don't see how that can be accurate, since the magnitude of the \"-1\"\ntweak will vary based on the value of \"it\". This code is due to\n3ed74e6, but I don't quite follow the logic in the commit message.\n\n-Peff\n"},{"id":"184679","messageId":"7vfwed6uws.fsf@alter.siamese.dyndns.org","threadId":"29619","inReplyTo":"20120214195036.GD12072@sigill.intra.peff.net","subject":"Re: diff --stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T20:07:31Z","receivedAt":"2012-02-14T20:07:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmm. Looking at scale_linear, the formula is:\n>\n> return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n>\n> I don't see how that can be accurate, since the magnitude of the \"-1\"\n> tweak will vary based on the value of \"it\". This code is due to\n> 3ed74e6, but I don't quite follow the logic in the commit message.\n\nDoesn't it need +1 at the end, I wonder? We want to map:\n\n - zero to zero\n - any number to at least 1\n\nso scaling a non-zero \"it\" so that maximum maps to (width-1) and then\nadding 1 would be the right way for the latter case.\n\nOf course, an easy way out without worrying about the correct math is to\nscale the total and the smaller one and then declare that the scaled\nlarger one is the difference between the two. That way, both of these two\nfiles have 109 in total so the length of the entire graph would be the\nsame ;-).\n"},{"id":"184686","messageId":"20120214202934.GA23291@sigill.intra.peff.net","threadId":"29619","inReplyTo":"7vfwed6uws.fsf@alter.siamese.dyndns.org","subject":"Re: diff --stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T20:29:34Z","receivedAt":"2012-02-14T20:29:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2012 at 12:07:31PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Hmm. Looking at scale_linear, the formula is:\n> >\n> > return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n> >\n> > I don't see how that can be accurate, since the magnitude of the \"-1\"\n> > tweak will vary based on the value of \"it\". This code is due to\n> > 3ed74e6, but I don't quite follow the logic in the commit message.\n> \n> Doesn't it need +1 at the end, I wonder? We want to map:\n> \n> - zero to zero\n> - any number to at least 1\n> \n> so scaling a non-zero \"it\" so that maximum maps to (width-1) and then\n> adding 1 would be the right way for the latter case.\n\nYeah, that makes more sense to me. I think you could also get by with\nsimply rounding the above properly to the nearest integer (so a little\nbit of error that makes 23.9 into 24.0 would be OK, because we would end\nup rounding 30.9 to 31.0, too). This seems like the most obvious\nsolution to me:\n\n static int divide_and_round(int a, int b)\n {\n return (2 * a + b) / (2 * b);\n }\n\n /*\n * We want non-zero changes to have at least 1 marker, so special-case\n * zero, then scale to width-1, and add back in 1.\n */\n static int scale_linear(int it, int width, int max_change)\n {\n if (!it)\n return 0;\n return 1 + divide_and_round(it * (width-1), max_change);\n }\n\nAny \"must show at least 1\" scheme is going to have some error in the\nscaling (because we are rounding up all of the low end). I have a\nfeeling that the scheme from 3ed74e6 was trying to distribute that error\nmore evenly throughout the scale, and the scheme above is lumping all of\nit at the start (i.e., the difference between what constitutes one\nmarker and two markers is much greater than the difference between two\nand three).\n\nBut that's just a vague feeling. For some reason my brain is not doing\nwell with math today, so I'll forego writing a proof.\n\n> Of course, an easy way out without worrying about the correct math is to\n> scale the total and the smaller one and then declare that the scaled\n> larger one is the difference between the two. That way, both of these two\n> files have 109 in total so the length of the entire graph would be the\n> same ;-).\n\nIt looks like we actually did that, pre-3ed74e6. I think it's a valid\nstrategy. It is just pushing the error around, but I don't know that\npeople are counting the +/- markers on the line and comparing them\nexactly. A little error there is less of a big deal than error between\ntwo lines which are supposed to have the same number of changes (you'll\nnote that we don't give the per-file added/deleted numbers exactly, so\nthey are a good place to hide error. :) ).\n\n-Peff\n"},{"id":"184701","messageId":"7vty2t5bmw.fsf@alter.siamese.dyndns.org","threadId":"29619","inReplyTo":"20120214202934.GA23291@sigill.intra.peff.net","subject":"Re: diff --stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T21:49:11Z","receivedAt":"2012-02-14T21:49:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 14, 2012 at 12:07:31PM -0800, Junio C Hamano wrote:\n>\n>> Of course, an easy way out without worrying about the correct math is to\n>> scale the total and the smaller one and then declare that the scaled\n>> larger one is the difference between the two. That way, both of these two\n>> files have 109 in total so the length of the entire graph would be the\n>> same ;-).\n>\n> It looks like we actually did that, pre-3ed74e6. I think it's a valid\n> strategy. It is just pushing the error around,...\n\nYes, that is exactly why I suggested that approach. We have to deal with\nrounding error somewhere no matter what we do, and the balance between\nadd/del is much less noticeable than the change with the same total not\nlining up.\n\nAnyway, here is an obvious patch to fix this. We did not have any test\nthat failed with this change, as all our test vectors fit comfortably on\nthe default 80-column output.\n\n-- >8 --\nSubject: diff --stat: resurrect \"same for same\" heuristic\n\nWhen commit 3ed74e6 (diff --stat: ensure at least one '-' for deletions,\nand one '+' for additions, 2006-09-28) improved the output for files with\ntiny modifications, we accidentally broke rounding logic to ensure that\ntwo equal sized changes are shown with the bars of the same length.\n\nThis updates the logic to compute the length of the graph bars, using the\nsame \"non-zero changes is shown with at least one column\" scaling logic,\nbut by scaling the sum of additions and deletions to come up with the\ntotal length of the bar (this ensures that two equal sized changes result\nin bars of the same length), and then scaling the smaller of the additions\nor deletions. The other side is computed as the difference between the\ntwo. This makes the apportioning between additions and deletions less\naccurate due to rounding errors, but it is much less noticeable than two\nfiles with the same amount of change showing bars of different length.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 3550c18..76d4724 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1273,13 +1273,17 @@ const char mime_boundary_leader[] = \"------------\";\n \n static int scale_linear(int it, int width, int max_change)\n {\n+\tif (!it)\n+\t\treturn 0;\n \t/*\n-\t * make sure that at least one '-' is printed if there were deletions,\n-\t * and likewise for '+'.\n+\t * make sure that at least one '-' or '+' is printed if\n+\t * there is any change to this path. The easiest way is to\n+\t * scale linearly as if all the quantities were one smaller\n+\t * than they actually are, and then add one to the result.\n \t */\n \tif (max_change < 2)\n-\t\treturn it;\n-\treturn ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n+\t\treturn 1;\n+\treturn 1 + ((it - 1) * (width - 1) / (max_change - 1));\n }\n \n static void show_name(FILE *file,\n@@ -1495,8 +1499,19 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tdels += del;\n \n \t\tif (width <= max_change) {\n-\t\t\tadd = scale_linear(add, width, max_change);\n-\t\t\tdel = scale_linear(del, width, max_change);\n+\t\t\tint total = add + del;\n+\n+\t\t\ttotal = scale_linear(add + del, width, max_change);\n+\t\t\tif (total < 2 && add && del)\n+\t\t\t\t/* width >= 2 due to the sanity check */\n+\t\t\t\ttotal = 2;\n+\t\t\tif (add < del) {\n+\t\t\t\tadd = scale_linear(add, width, max_change);\n+\t\t\t\tdel = total - add;\n+\t\t\t} else {\n+\t\t\t\tdel = scale_linear(del, width, max_change);\n+\t\t\t\tadd = total - del;\n+\t\t\t}\n \t\t}\n \t\tfprintf(options->file, \"%s\", line_prefix);\n \t\tshow_name(options->file, prefix, name, len);\n"},{"id":"184704","messageId":"20120214215318.GA24802@sigill.intra.peff.net","threadId":"29619","inReplyTo":"7vty2t5bmw.fsf@alter.siamese.dyndns.org","subject":"Re: diff --stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T21:53:18Z","receivedAt":"2012-02-14T21:53:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2012 at 01:49:11PM -0800, Junio C Hamano wrote:\n\n> static int scale_linear(int it, int width, int max_change)\n> {\n> +\tif (!it)\n> +\t\treturn 0;\n> \t/*\n> -\t * make sure that at least one '-' is printed if there were deletions,\n> -\t * and likewise for '+'.\n> +\t * make sure that at least one '-' or '+' is printed if\n> +\t * there is any change to this path. The easiest way is to\n> +\t * scale linearly as if all the quantities were one smaller\n> +\t * than they actually are, and then add one to the result.\n> \t */\n> \tif (max_change < 2)\n> -\t\treturn it;\n> -\treturn ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);\n> +\t\treturn 1;\n> +\treturn 1 + ((it - 1) * (width - 1) / (max_change - 1));\n\nI'm not sure I understand why the \"it - 1\" and \"max_change - 1\" bits are\nstill there, or what they are doing in the first place. I.e., why is it\nnot simply \"scale linearly to width-1, then add 1\" as I posted earlier?\n\n-Peff\n"},{"id":"184707","messageId":"7vipj95au5.fsf@alter.siamese.dyndns.org","threadId":"29619","inReplyTo":"20120214215318.GA24802@sigill.intra.peff.net","subject":"Re: diff --stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T22:06:26Z","receivedAt":"2012-02-14T22:06:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure I understand why the \"it - 1\" and \"max_change - 1\" bits are\n> still there,...\n\nSimple; I wasn't thinking straight.\n"}]}