[PATCH 2/3] --dirstat-by-file: Make it faster and more correct

[Johan Herland <johan@herland.net>]

Currently, when using --dirstat-by-file, it first does the full --dirstat
analysis (using diffcore_count_changes()), and then resets 'damage' to 1,
if any damage was found by diffcore_count_changes().

But --dirstat-by-file is not interested in the file damage per se. It only
cares if the file changed at all. In that sense it only cares if the blob
SHA1 for a file has changed. Fortunately, determining which files have
changed is already done when we build the diff_queue, and by the time we
get to show_dirstat(), we know that each entry in the queue correspond to
a changed file. Therefore we can skip the entire --dirstat analysis and
simply set 'damage' to 1 for each entry in the diff queue.

This makes --dirstat-by-file faster, and also bypasses --dirstat's issues
with detecting changes that merely rearrange lines.

The patch also contains an added testcase verifying that --dirstat-by-file
now detects changes that only rearrange lines.

Signed-off-by: Johan Herland <johan@herland.net>
---

I hope someone with more intimate diffcore knowledge can verify that
the diff queue indeed never contains entries that should be considered
"unchanged" by --dirstat-by-file.


...Johan

 diff.c                                             |   21 +++++++++++++++----
 t/t4013-diff-various.sh                            |    2 +
 .../diff.diff_--dirstat-by-file_initial_rearrange  |    3 ++
 3 files changed, 21 insertions(+), 5 deletions(-)
 create mode 100644 t/t4013/diff.diff_--dirstat-by-file_initial_rearrange

diff --git a/diff.c b/diff.c
index 9fa8410..28d9293 100644
--- a/diff.c
+++ b/diff.c
@@ -1541,6 +1541,20 @@ static void show_dirstat(struct diff_options *options)
 
 		name = p->one->path ? p->one->path : p->two->path;
 
+		if (DIFF_OPT_TST(options, DIRSTAT_BY_FILE)) {
+			/*
+			 * In --dirstat-by-file mode, we're only interested in
+			 * whether the file changed _at_all_.
+			 * We don't need to look at the actual file contents.
+			 * Assuming that the diff queue does not contain
+			 * unchanged entries, we can unconditionally add this
+			 * file to the list of results (with each file
+			 * contributing equal damage).
+			 */
+			damage = 1;
+			goto found_damage;
+		}
+
 		if (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {
 			diff_populate_filespec(p->one, 0);
 			diff_populate_filespec(p->two, 0);
@@ -1563,14 +1577,11 @@ static void show_dirstat(struct diff_options *options)
 		/*
 		 * Original minus copied is the removed material,
 		 * added is the new material.  They are both damages
-		 * made to the preimage. In --dirstat-by-file mode, count
-		 * damaged files, not damaged lines. This is done by
-		 * counting only a single damaged line per file.
+		 * made to the preimage.
 		 */
 		damage = (p->one->size - copied) + added;
-		if (DIFF_OPT_TST(options, DIRSTAT_BY_FILE) && damage > 0)
-			damage = 1;
 
+found_damage:
 		ALLOC_GROW(dir.files, dir.nr + 1, dir.alloc);
 		dir.files[dir.nr].name = name;
 		dir.files[dir.nr].changed = damage;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 8cc94ef..e8240f2 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -302,6 +302,8 @@ diff master master^ side
 diff --dirstat master~1 master~2
 # --dirstat does NOT pick up changes that simply rearrange existing lines
 diff --dirstat initial rearrange
+# ...but --dirstat-by-file DOES pick up rearranged lines
+diff --dirstat-by-file initial rearrange
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--dirstat-by-file_initial_rearrange b/t/t4013/diff.diff_--dirstat-by-file_initial_rearrange
new file mode 100644
index 0000000..e48e33f
--- /dev/null
+++ b/t/t4013/diff.diff_--dirstat-by-file_initial_rearrange
@@ -0,0 +1,3 @@
+$ git diff --dirstat-by-file initial rearrange
+ 100.0% dir/
+$
-- 
1.7.5.rc1