RFC: "negative" dirstat

Ok, so this is just an RFC patch, but the concept is pretty simple..

In the kernel, the ARM platform is growing boundlessly, with platform files being added for every random SoC out there. One of the things that Russell King (arm maintainer) has worried about is that when they try to clean stuff up, code *removal* also ends up adding to the damage, so if ARM ever gets its act together and is able to consolidate a lot of this, it's still going to look very bad in the statistics because there will be a lot of damage due to removed files.

In the regular "--diffstat" output, this is all very obvious, because if you actually remove more lines than you add, it will say so, and people will be very happy.

But in --dirstat, removed lines are always counted towards damage.

So here's a total hacky RFC patch to add a "--negative" option, which allows for dirstat to actually take the amount of added/removed code into account, and make "damage" a signed integer instead.

Example case right now for my pulls today:

[torvalds@i5 linux]$ git diff --dirstat=1 --cumulative @{6am}..
  22.6% Documentation/input/
   1.4% arch/powerpc/include/asm/
   3.7% arch/powerpc/kernel/
   6.6% arch/powerpc/
   6.3% block/
   6.0% drivers/input/
  15.7% drivers/md/
  22.1% drivers/
  38.4% fs/btrfs/
  39.1% fs/
   2.7% include/linux/
[torvalds@i5 linux]$ git diff --dirstat=1 --cumulative --negative @{6am}..
  56.2% Documentation/input/
   1.9% arch/powerpc/include/asm/
   4.3% arch/powerpc/kernel/
   1.3% arch/powerpc/platforms/pseries/
   1.4% arch/powerpc/platforms/
   8.4% arch/powerpc/
   2.4% block/
   1.6% drivers/input/misc/
   8.4% drivers/input/
  -5.6% drivers/md/
   2.7% drivers/
  28.5% fs/btrfs/
  29.0% fs/

ie note how with "--negative", it becomes obvious that drivers/md actually removed more than it added, while some subdirectories were all about adding (Documentation/input got a new file), while others were more about changing existing lines with not a lot of additional actual new code.

The diffstat for drivers/md looks like this:

 7 files changed, 103 insertions(+), 137 deletions(-)

and I think most of the insertions were shorter lines than the deletions too.

NOTE! This is known-buggy in that you may end up in a situation where the percentages are > 100% (the "total change" may be arbitrarily small, since they all add up). In fact, you might get a division-by-zero if the total change ends up being zero. This is a RFC patch, nothing more. I don't know what the "right" solution for the percentages should be (except that it obviously should never cause a divide-by-zero).

Comments?

                        Linus "yeah, that option name sucks" Torvalds

 diff.c |   26 +++++++++++++++++---------
 diff.h |    1 +
 2 files changed, 18 insertions(+), 9 deletions(-)

Linus Torvalds <torvalds@linux-foundation.org> writes:

Are these removal really "remove old cruft, replace with a better version which does not have much common to removed stuff", or are they more like "remove N duplicated similar copies of old cruft, refactoring them properly and the result is used by N callsites"?

The second reason you gave in an earlier discussion why dirstat uses the damage assessor code was to disregard code movements. It appears to me that if you spanhash the contents of _all_ files in the preimage and the postimage of ARM tree and compute literal-added vs src-copied within the whole tree, I wonder if you can mitigate this "false damage -- because the refactoring involved code movement across files but within the same subsystem".

I guess what it boils down to is what you are trying to measure as the "goodness" value of a change. Adding a lot of Documentation may be good, adding a lot of "subarchs that do not deserve to be" may be bad, and moving common logic from one existing subarch to a common file (which counts towards "literal-added" in that new common file, at the same time counting towards deletion, i.e. "size - copied", from the original) and reusing it in a new subarch by simply calling that common infrastructure is a very good thing. At least, if you count literal-added vs src-copied across the files within the subarch, instead of doing it per-file, you would be able to detect the "moving" part more accurately. Of course, you still cannot tell between good and bad kinds of additions, and you cannot tell that the new subarch that reuses the result of refactoring by calling into the refactored code, without understanding the source code, and I don't think that is within the scope of dirstat.

On Mon, Apr 18, 2011 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:

Right now, there not so much of either.

The _hope_ is that there will be "remove <n> different implementations of some gpio driver, and replace them with a couple of generic ones and some setup code".

So no, it wouldn't necessarily be about code movement at all, but about totally different code.

No, that only works within a file, which sometimes is common when you have to re-order functions because or some re-organization.

See for example commit 9d412a43c3b2 ("vfs: split off vfsmount-related parts of vfs_kern_mount()") in the kernel for an example of why line ordering shouldn't necessarily count against damage.

But what I'm talking about is really different code, not re-organization of existing one.

Yes.

Yes. If I see a lot of lines added in Documentation/, I really don't mind at all. And if I see a diffstat that says something like

   1057 lines added, 2901 lines deleted

I'm extremely happy. It's very different from one that would say

   2958 lines added, 0 lines deleted

even though --dirstat might consider the two equivalent right now.

Yes. However, "moving" is in many ways still not as interesting as "actually removed".

Moving, on it's own, is a pretty neutral operation. So I really don't want to concentrate on the moving part. It's really about "some operations actually clean up code and remove code in the process".

                  Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

I am afraid that this is ultimately "how to compare apples and oranges" (or "which is juicier, oranges or squirrels?") question.

If you are willing to say

    1000 lines added, 700 lines deleted

means "It changes 700 lines (removing the original 700 and replacing them with the new 700) and then adds 300 lines of new material", then you could simply use the max(added, p->one->size - copied) instead of using the sum of them as the damage count. Of course, depending on the nature of the change, the above interpretation does not apply in general.

I think I was unclear, but I was shooting for the same thing.

I meant "by doing the added/copied computation across all files, the real additions and deletions would reflect in the result without the noise from the 'moving' part."