{"thread":{"id":"27129","subject":"RFC: \"negative\" dirstat","startedAt":"2011-04-18T21:02:22Z","lastAt":"2011-04-19T04:51:13Z","messageCount":4,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"166044","messageId":"BANLkTi=it7r7UsAZGYJC1mntL6wtFipB9g@mail.gmail.com","threadId":"27129","inReplyTo":null,"subject":"RFC: \"negative\" dirstat","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-04-18T21:02:22Z","receivedAt":"2011-04-18T21:02:22Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"Ok, so this is just an RFC patch, but the concept is pretty simple..\n\nIn the kernel, the ARM platform is growing boundlessly, with platform\nfiles being added for every random SoC out there. One of the things\nthat Russell King (arm maintainer) has worried about is that when they\ntry to clean stuff up, code *removal* also ends up adding to the\ndamage, so if ARM ever gets its act together and is able to\nconsolidate a lot of this, it's still going to look very bad in the\nstatistics because there will be a lot of damage due to removed files.\n\nIn the regular \"--diffstat\" output, this is all very obvious, because\nif you actually remove more lines than you add, it will say so, and\npeople will be very happy.\n\nBut in --dirstat, removed lines are always counted towards damage.\n\nSo here's a total hacky RFC patch to add a \"--negative\" option, which\nallows for dirstat to actually take the amount of added/removed code\ninto account, and make \"damage\" a signed integer instead.\n\nExample case right now for my pulls today:\n\n[torvalds@i5 linux]$ git diff --dirstat=1 --cumulative @{6am}..\n  22.6% Documentation/input/\n   1.4% arch/powerpc/include/asm/\n   3.7% arch/powerpc/kernel/\n   6.6% arch/powerpc/\n   6.3% block/\n   6.0% drivers/input/\n  15.7% drivers/md/\n  22.1% drivers/\n  38.4% fs/btrfs/\n  39.1% fs/\n   2.7% include/linux/\n[torvalds@i5 linux]$ git diff --dirstat=1 --cumulative --negative @{6am}..\n  56.2% Documentation/input/\n   1.9% arch/powerpc/include/asm/\n   4.3% arch/powerpc/kernel/\n   1.3% arch/powerpc/platforms/pseries/\n   1.4% arch/powerpc/platforms/\n   8.4% arch/powerpc/\n   2.4% block/\n   1.6% drivers/input/misc/\n   8.4% drivers/input/\n  -5.6% drivers/md/\n   2.7% drivers/\n  28.5% fs/btrfs/\n  29.0% fs/\n\nie note how with \"--negative\", it becomes obvious that drivers/md\nactually removed more than it added, while some subdirectories were\nall about adding (Documentation/input got a new file), while others\nwere more about changing existing lines with not a lot of additional\nactual new code.\n\nThe diffstat for drivers/md looks like this:\n\n 7 files changed, 103 insertions(+), 137 deletions(-)\n\nand I think most of the insertions were shorter lines than the deletions too.\n\nNOTE! This is known-buggy in that you may end up in a situation where\nthe percentages are > 100% (the \"total change\" may be arbitrarily\nsmall, since they all add up). In fact, you might get a\ndivision-by-zero if the total change ends up being zero. This is a RFC\npatch, nothing more. I don't know what the \"right\" solution for the\npercentages should be (except that it obviously should never cause a\ndivide-by-zero).\n\nComments?\n\n                        Linus \"yeah, that option name sucks\" Torvalds\n\n\n diff.c |   26 +++++++++++++++++---------\n diff.h |    1 +\n 2 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9fa841010cc2..c93fdaaa541f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1447,7 +1447,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)\n \n struct dirstat_file {\n \tconst char *name;\n-\tunsigned long changed;\n+\tlong changed;\n };\n \n struct dirstat_dir {\n@@ -1458,7 +1458,7 @@ struct dirstat_dir {\n static long gather_dirstat(struct diff_options *opt, struct dirstat_dir *dir,\n \t\tunsigned long changed, const char *base, int baselen)\n {\n-\tunsigned long this_dir = 0;\n+\tlong this_dir = 0;\n \tunsigned int sources = 0;\n \tconst char *line_prefix = \"\";\n \tstruct strbuf *msg = NULL;\n@@ -1499,12 +1499,14 @@ static long gather_dirstat(struct diff_options *opt, struct dirstat_dir *dir,\n \t *    under this directory (sources == 1).\n \t */\n \tif (baselen && sources != 1) {\n-\t\tint permille = this_dir * 1000 / changed;\n+\t\tint permille = labs(this_dir) * 1000 / changed;\n \t\tif (permille) {\n-\t\t\tint percent = permille / 10;\n+\t\t\tdouble percent = permille / 10.0;\n \t\t\tif (percent >= dir->percent) {\n-\t\t\t\tfprintf(opt->file, \"%s%4d.%01d%% %.*s\\n\", line_prefix,\n-\t\t\t\t\tpercent, permille % 10, baselen, base);\n+\t\t\t\tif (this_dir < 0)\n+\t\t\t\t\tpercent = -percent;\n+\t\t\t\tfprintf(opt->file, \"%s%6.1f%% %.*s\\n\", line_prefix,\n+\t\t\t\t\tpercent, baselen, base);\n \t\t\t\tif (!dir->cumulative)\n \t\t\t\t\treturn 0;\n \t\t\t}\n@@ -1537,7 +1539,8 @@ static void show_dirstat(struct diff_options *options)\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tconst char *name;\n-\t\tunsigned long copied, added, damage;\n+\t\tunsigned long copied, added, deleted;\n+\t\tlong damage;\n \n \t\tname = p->one->path ? p->one->path : p->two->path;\n \n@@ -1567,7 +1570,11 @@ static void show_dirstat(struct diff_options *options)\n \t\t * damaged files, not damaged lines. This is done by\n \t\t * counting only a single damaged line per file.\n \t\t */\n-\t\tdamage = (p->one->size - copied) + added;\n+\t\tdeleted = p->one->size - copied;\n+\t\tif (DIFF_OPT_TST(options, DIFFSTAT_NEGATIVE))\n+\t\t\tdamage = added - deleted;\n+\t\telse\n+\t\t\tdamage = added + deleted;\n \t\tif (DIFF_OPT_TST(options, DIRSTAT_BY_FILE) && damage > 0)\n \t\t\tdamage = 1;\n \n@@ -3139,7 +3146,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\t\t   &options->dirstat_percent)) {\n \t\toptions->output_format |= DIFF_FORMAT_DIRSTAT;\n \t\tDIFF_OPT_SET(options, DIRSTAT_BY_FILE);\n-\t}\n+\t} else if (!strcmp(arg, \"--negative\"))\n+\t\tDIFF_OPT_SET(options, DIFFSTAT_NEGATIVE);\n \telse if (!strcmp(arg, \"--check\"))\n \t\toptions->output_format |= DIFF_FORMAT_CHECKDIFF;\n \telse if (!strcmp(arg, \"--summary\"))\ndiff --git a/diff.h b/diff.h\nindex 007a0554d4b2..95d6e65247ae 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -78,6 +78,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)\n #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)\n #define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)\n+#define DIFF_OPT_DIFFSTAT_NEGATIVE   (1 << 28)\n \n #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)\n"},{"id":"166048","messageId":"7vzknn6y7y.fsf@alter.siamese.dyndns.org","threadId":"27129","inReplyTo":"BANLkTi=it7r7UsAZGYJC1mntL6wtFipB9g@mail.gmail.com","subject":"Re: RFC: \"negative\" dirstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-18T21:33:53Z","receivedAt":"2011-04-18T21:33:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Ok, so this is just an RFC patch, but the concept is pretty simple..\n>\n> In the kernel, the ARM platform is growing boundlessly, with platform\n> files being added for every random SoC out there. One of the things\n> that Russell King (arm maintainer) has worried about is that when they\n> try to clean stuff up, code *removal* also ends up adding to the\n> damage, so if ARM ever gets its act together and is able to\n> consolidate a lot of this, it's still going to look very bad in the\n> statistics because there will be a lot of damage due to removed files.\n\nAre these removal really \"remove old cruft, replace with a better version\nwhich does not have much common to removed stuff\", or are they more like\n\"remove N duplicated similar copies of old cruft, refactoring them\nproperly and the result is used by N callsites\"?\n\nThe second reason you gave in an earlier discussion why dirstat uses the\ndamage assessor code was to disregard code movements. It appears to me\nthat if you spanhash the contents of _all_ files in the preimage and the\npostimage of ARM tree and compute literal-added vs src-copied within the\nwhole tree, I wonder if you can mitigate this \"false damage -- because the\nrefactoring involved code movement across files but within the same\nsubsystem\".\n\nI guess what it boils down to is what you are trying to measure as the\n\"goodness\" value of a change. Adding a lot of Documentation may be good,\nadding a lot of \"subarchs that do not deserve to be\" may be bad, and\nmoving common logic from one existing subarch to a common file (which\ncounts towards \"literal-added\" in that new common file, at the same time\ncounting towards deletion, i.e. \"size - copied\", from the original) and\nreusing it in a new subarch by simply calling that common infrastructure\nis a very good thing. At least, if you count literal-added vs src-copied\nacross the files within the subarch, instead of doing it per-file, you\nwould be able to detect the \"moving\" part more accurately. Of course, you\nstill cannot tell between good and bad kinds of additions, and you cannot\ntell that the new subarch that reuses the result of refactoring by calling\ninto the refactored code, without understanding the source code, and I\ndon't think that is within the scope of dirstat.\n"},{"id":"166051","messageId":"BANLkTimEUSb9Na7Z+tnmytsqeeKLkQb9xA@mail.gmail.com","threadId":"27129","inReplyTo":"7vzknn6y7y.fsf@alter.siamese.dyndns.org","subject":"Re: RFC: \"negative\" dirstat","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-04-18T21:46:24Z","receivedAt":"2011-04-18T21:46:24Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Apr 18, 2011 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Are these removal really \"remove old cruft, replace with a better version\n> which does not have much common to removed stuff\", or are they more like\n> \"remove N duplicated similar copies of old cruft, refactoring them\n> properly and the result is used by N callsites\"?\n\nRight now, there not so much of either.\n\nThe _hope_ is that there will be \"remove <n> different implementations\nof some gpio driver, and replace them with a couple of generic ones\nand some setup code\".\n\nSo no, it wouldn't necessarily be about code movement at all, but\nabout totally different code.\n\n> The second reason you gave in an earlier discussion why dirstat uses the\n> damage assessor code was to disregard code movements.\n\nNo, that only works within a file, which sometimes is common when you\nhave to re-order functions because or some re-organization.\n\nSee for example commit 9d412a43c3b2 (\"vfs: split off vfsmount-related\nparts of vfs_kern_mount()\") in the kernel for an example of why line\nordering shouldn't necessarily count against damage.\n\nBut what I'm talking about is really different code, not\nre-organization of existing one.\n\n> I guess what it boils down to is what you are trying to measure as the\n> \"goodness\" value of a change.\n\nYes.\n\n>                               Adding a lot of Documentation may be good,\n> adding a lot of \"subarchs that do not deserve to be\" may be bad, and\n> moving common logic from one existing subarch to a common file (which\n> counts towards \"literal-added\" in that new common file, at the same time\n> counting towards deletion, i.e. \"size - copied\", from the original) and\n> reusing it in a new subarch by simply calling that common infrastructure\n> is a very good thing.\n\nYes. If I see a lot of lines added in Documentation/, I really don't\nmind at all. And if I see a diffstat that says something like\n\n   1057 lines added, 2901 lines deleted\n\nI'm extremely happy. It's very different from one that would say\n\n   2958 lines added, 0 lines deleted\n\neven though --dirstat might consider the two equivalent right now.\n\n> At least, if you count literal-added vs src-copied\n> across the files within the subarch, instead of doing it per-file, you\n> would be able to detect the \"moving\" part more accurately.\n\nYes. However, \"moving\" is in many ways still not as interesting as\n\"actually removed\".\n\nMoving, on it's own, is a pretty neutral operation. So I really don't\nwant to concentrate on the moving part. It's really about \"some\noperations actually clean up code and remove code in the process\".\n\n                  Linus\n"},{"id":"166061","messageId":"7vtydu6dz2.fsf@alter.siamese.dyndns.org","threadId":"27129","inReplyTo":"BANLkTimEUSb9Na7Z+tnmytsqeeKLkQb9xA@mail.gmail.com","subject":"Re: RFC: \"negative\" dirstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-19T04:51:13Z","receivedAt":"2011-04-19T04:51:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Mon, Apr 18, 2011 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n>> I guess what it boils down to is what you are trying to measure as the\n>> \"goodness\" value of a change.\n>\n> Yes.\n>\n> Yes. If I see a lot of lines added in Documentation/, I really don't\n> mind at all. And if I see a diffstat that says something like\n>\n>    1057 lines added, 2901 lines deleted\n>\n> I'm extremely happy. It's very different from one that would say\n>\n>    2958 lines added, 0 lines deleted\n>\n> even though --dirstat might consider the two equivalent right now.\n\nI am afraid that this is ultimately \"how to compare apples and oranges\"\n(or \"which is juicier, oranges or squirrels?\") question.\n\nIf you are willing to say\n\n    1000 lines added, 700 lines deleted\n\nmeans \"It changes 700 lines (removing the original 700 and replacing them\nwith the new 700) and then adds 300 lines of new material\", then you could\nsimply use the max(added, p->one->size - copied) instead of using the sum\nof them as the damage count.  Of course, depending on the nature of the\nchange, the above interpretation does not apply in general.\n\n>> At least, if you count literal-added vs src-copied across the files\n>> within the subarch, instead of doing it per-file, you would be able to\n>> detect the \"moving\" part more accurately.\n>\n> Yes. However, \"moving\" is in many ways still not as interesting as\n> \"actually removed\".  Moving, on it's own, is a pretty neutral operation.\n\nI think I was unclear, but I was shooting for the same thing.\n\nI meant \"by doing the added/copied computation across all files, the real\nadditions and deletions would reflect in the result without the noise from\nthe 'moving' part.\"\n"}]}