[PATCH 0/5] "diff --stat" counting fixes

STALE3738d

8 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH 0/5] "diff --stat" counting fixes

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:26

It turns out that there are at least two bugs in the diffstat
counting code.  This series comes on top of the earlier 74faaa1 (Fix
"git diff --stat" for interesting - but empty - file changes,
2012-10-17) to fix them.

Junio C Hamano (5):
  test: add failing tests for "diff --stat" to t4049
  diff --stat: status of unmodified pair in diff-q is not zero
  diff --stat: use "file" temporary variable to refer to data->files[i]
  diff --stat: move the "total count" logic to the last loop
  diff --stat: do not count "unmerged" entries

 diff.c                     | 49 +++++++++++++++++++++++++---------------------
 t/t4049-diff-stat-count.sh | 46 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 23 deletions(-)

-- 
1.8.0.1.331.g808d2af

[PATCH 1/5] test: add failing tests for "diff --stat" to t4049

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:26

There are a few problems in diff.c around --stat area, partially
caused by the recent 74faaa1 (Fix "git diff --stat" for interesting
- but empty - file changes, 2012-10-17), and largely caused by the
earlier change that introduced when --stat-count was added.

Add a few test pieces to t4049 to expose the issues.

Signed-off-by: Junio C Hamano <redacted>
---
 t/t4049-diff-stat-count.sh | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/t/t4049-diff-stat-count.sh b/t/t4049-diff-stat-count.sh
index 7b3ef00..e212b11 100755
--- a/t/t4049-diff-stat-count.sh
+++ b/t/t4049-diff-stat-count.sh

@@ -4,12 +4,17 @@
 test_description='diff --stat-count'
 . ./test-lib.sh
 
-test_expect_success setup '
+test_expect_success 'setup' '
 	>a &&
 	>b &&
 	>c &&
 	>d &&
 	git add a b c d &&
+	git commit -m initial
+'
+
+test_expect_success 'limit output to 2 (simple)' '
+	git reset --hard &&
 	chmod +x c d &&
 	echo a >a &&
 	echo b >b &&

@@ -23,4 +28,43 @@ test_expect_success setup '
 	test_i18ncmp expect actual
 '
 
+test_expect_failure 'binary changes do not count in lines' '
+	git reset --hard &&
+	chmod +x c d &&
+	echo a >a &&
+	echo b >b &&
+	cat "$TEST_DIRECTORY"/test-binary-1.png >d &&
+	cat >expect <<-\EOF
+	 a | 1 +
+	 b | 1 +
+	 ...
+	 4 files changed, 2 insertions(+)
+	EOF
+	git diff --stat --stat-count=2 >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_failure 'exclude unmerged entries from total file count' '
+	git reset --hard &&
+	echo a >a &&
+	echo b >b &&
+	git ls-files -s a >x &&
+	git rm -f d &&
+	for stage in 1 2 3
+	do
+		sed -e "s/ 0	a/ $stage	d/" x
+	done |
+	git update-index --index-info &&
+	echo d >d &&
+	chmod +x c d &&
+	cat >expect <<-\EOF
+	 a | 1 +
+	 b | 1 +
+	 ...
+	 4 files changed, 3 insertions(+)
+	EOF
+	git diff --stat --stat-count=2 >actual &&
+	test_i18ncmp expect actual
+'
+
 test_done

-- 
1.8.0.1.331.g808d2af

[PATCH 2/5] diff --stat: status of unmodified pair in diff-q is not zero

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:26

It is spelled DIFF_STATUS_UNKNOWN these days, and is different from zero.

Signed-off-by: Junio C Hamano <redacted>
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 95bbad6..ce6baa4 100644
--- a/diff.c
+++ b/diff.c

@@ -2411,7 +2411,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 	}
 
 	data = diffstat_add(diffstat, name_a, name_b);
-	data->is_interesting = p->status != 0;
+	data->is_interesting = p->status != DIFF_STATUS_UNKNOWN;
 
 	if (!one || !two) {
 		data->is_unmerged = 1;

-- 
1.8.0.1.331.g808d2af

[PATCH 5/5] diff --stat: do not count "unmerged" entries

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:26

Even though we show a separate *UNMERGED* entry in the patch and
diffstat output (or in the --raw format, for that matter) in
addition to and separately from the diff against the specified stage
(defaulting to #2) for unmerged paths, they should not be counted in
the total number of files affected---that would lead to counting the
same path twice.

The separation done by the previous step makes this fix simple and
straightforward.  Among the filepairs in diff_queue, paths that
weren't modified, and the extra "unmerged" entries do not count as
total number of files.

Signed-off-by: Junio C Hamano <redacted>
---
 diff.c                     | 6 ++++--
 t/t4049-diff-stat-count.sh | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 4105260..26ede82 100644
--- a/diff.c
+++ b/diff.c

@@ -1669,12 +1669,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		struct diffstat_file *file = data->files[i];
 		uintmax_t added = file->added;
 		uintmax_t deleted = file->deleted;
-		if (!file->is_interesting && (added + deleted == 0)) {
+
+		if (file->is_unmerged ||
+		    (!file->is_interesting && (added + deleted == 0))) {
 			total_files--;
 			continue;
 		}
 
-		if (!file->is_binary && !file->is_unmerged) {
+		if (!file->is_binary) {
 			adds += added;
 			dels += deleted;
 		}

diff --git a/t/t4049-diff-stat-count.sh b/t/t4049-diff-stat-count.sh
index 70ee073..37f50cd 100755
--- a/t/t4049-diff-stat-count.sh
+++ b/t/t4049-diff-stat-count.sh

@@ -44,7 +44,7 @@ test_expect_success 'binary changes do not count in lines' '
 	test_i18ncmp expect actual
 '
 
-test_expect_failure 'exclude unmerged entries from total file count' '
+test_expect_success 'exclude unmerged entries from total file count' '
 	git reset --hard &&
 	echo a >a &&
 	echo b >b &&

-- 
1.8.0.1.331.g808d2af

[PATCH 3/5] diff --stat: use "file" temporary variable to refer to data->files[i]

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:26

The generated code shouldn't change but it is easier to read.

Signed-off-by: Junio C Hamano <redacted>
---
 diff.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/diff.c b/diff.c
index ce6baa4..e4e70e5 100644
--- a/diff.c
+++ b/diff.c

@@ -1470,8 +1470,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	for (i = 0; (i < count) && (i < data->nr); i++) {
 		struct diffstat_file *file = data->files[i];
 		uintmax_t change = file->added + file->deleted;
-		if (!data->files[i]->is_interesting &&
-			 (change == 0)) {
+
+		if (!file->is_interesting && (change == 0)) {
 			count++; /* not shown == room for one more */
 			continue;
 		}

@@ -1586,13 +1586,13 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	 */
 	for (i = 0; i < count; i++) {
 		const char *prefix = "";
-		char *name = data->files[i]->print_name;
-		uintmax_t added = data->files[i]->added;
-		uintmax_t deleted = data->files[i]->deleted;
+		struct diffstat_file *file = data->files[i];
+		char *name = file->print_name;
+		uintmax_t added = file->added;
+		uintmax_t deleted = file->deleted;
 		int name_len;
 
-		if (!data->files[i]->is_interesting &&
-			 (added + deleted == 0)) {
+		if (!file->is_interesting && (added + deleted == 0)) {
 			total_files--;
 			continue;
 		}

@@ -1611,7 +1611,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 				name = slash;
 		}
 
-		if (data->files[i]->is_binary) {
+		if (file->is_binary) {
 			fprintf(options->file, "%s", line_prefix);
 			show_name(options->file, prefix, name, len);
 			fprintf(options->file, " %*s", number_width, "Bin");

@@ -1628,7 +1628,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 			fprintf(options->file, "\n");
 			continue;
 		}
-		else if (data->files[i]->is_unmerged) {
+		else if (file->is_unmerged) {
 			fprintf(options->file, "%s", line_prefix);
 			show_name(options->file, prefix, name, len);
 			fprintf(options->file, " Unmerged\n");

@@ -1668,10 +1668,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		fprintf(options->file, "\n");
 	}
 	for (i = count; i < data->nr; i++) {
-		uintmax_t added = data->files[i]->added;
-		uintmax_t deleted = data->files[i]->deleted;
-		if (!data->files[i]->is_interesting &&
-			 (added + deleted == 0)) {
+		struct diffstat_file *file = data->files[i];
+		uintmax_t added = file->added;
+		uintmax_t deleted = file->deleted;
+		if (!file->is_interesting && (added + deleted == 0)) {
 			total_files--;
 			continue;
 		}

-- 
1.8.0.1.331.g808d2af

[PATCH 4/5] diff --stat: move the "total count" logic to the last loop

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:26

The diffstat generation logic, with --stat-count limit, is
implemented as three loops.

 - The first counts the width necessary to show stats up to
   specified number of entries, and notes up to how many entries in
   the data we need to iterate to show the graph;

 - The second iterates that many times to draw the graph, adjusts
   the number of "total modified files", and counts the total
   added/deleted lines for the part that was shown in the graph;

 - The third iterates over the remainder and only does the part to
   count "total added/deleted lines" and to adjust "total modified
   files" without drawing anything.

Move the logic to count added/deleted lines and modified files from
the second loop to the third loop.

This incidentally fixes a bug.  The third loop was not filtering
binary changes (counted in bytes) from the total added/deleted as it
should.  The second loop implemented this correctly, so if a binary
change appeared earlier than the --stat-count cutoff, the code
counted number of added/deleted lines correctly, but if it appeared
beyond the cutoff, the number of lines would have mixed with the
byte count in the buggy third loop.

Signed-off-by: Junio C Hamano <redacted>
---
 diff.c                     | 21 ++++++++++++---------
 t/t4049-diff-stat-count.sh |  2 +-
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index e4e70e5..4105260 100644
--- a/diff.c
+++ b/diff.c

@@ -1498,7 +1498,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		if (max_change < change)
 			max_change = change;
 	}
-	count = i; /* min(count, data->nr) */
+	count = i; /* where we can stop scanning in data->files[] */
 
 	/*
 	 * We have width = stat_width or term_columns() columns total.

@@ -1592,10 +1592,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		uintmax_t deleted = file->deleted;
 		int name_len;
 
-		if (!file->is_interesting && (added + deleted == 0)) {
-			total_files--;
+		if (!file->is_interesting && (added + deleted == 0))
 			continue;
-		}
+
 		/*
 		 * "scale" the filename
 		 */

@@ -1640,8 +1639,6 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		 */
 		add = added;
 		del = deleted;
-		adds += add;
-		dels += del;
 
 		if (graph_width <= max_change) {
 			int total = add + del;

@@ -1667,7 +1664,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		show_graph(options->file, '-', del, del_c, reset);
 		fprintf(options->file, "\n");
 	}
-	for (i = count; i < data->nr; i++) {
+
+	for (i = 0; i < data->nr; i++) {
 		struct diffstat_file *file = data->files[i];
 		uintmax_t added = file->added;
 		uintmax_t deleted = file->deleted;

@@ -1675,8 +1673,13 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 			total_files--;
 			continue;
 		}
-		adds += added;
-		dels += deleted;
+
+		if (!file->is_binary && !file->is_unmerged) {
+			adds += added;
+			dels += deleted;
+		}
+		if (i < count)
+			continue;
 		if (!extra_shown)
 			fprintf(options->file, "%s ...\n", line_prefix);
 		extra_shown = 1;

diff --git a/t/t4049-diff-stat-count.sh b/t/t4049-diff-stat-count.sh
index e212b11..70ee073 100755
--- a/t/t4049-diff-stat-count.sh
+++ b/t/t4049-diff-stat-count.sh

@@ -28,7 +28,7 @@ test_expect_success 'limit output to 2 (simple)' '
 	test_i18ncmp expect actual
 '
 
-test_expect_failure 'binary changes do not count in lines' '
+test_expect_success 'binary changes do not count in lines' '
 	git reset --hard &&
 	chmod +x c d &&
 	echo a >a &&

-- 
1.8.0.1.331.g808d2af

[PATCH] t4049: avoid test failures on filemode challenged file systems (Windows)

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:55:27

From: Johannes Sixt <redacted>

The earlier change 74faaa16 (Fix "git diff --stat" for interesting - but
empty - file changes) needed to change the count of differing files
because the executable-bit changes of two empty files are now counted.

On file systems that do not record the executable bit, however, the old
file count was actually correct (and the updated tests fail) because the
mode change cannot be diagnosed by looking at the file system alone.

Change the mode not only on the file system, but also in the index;
compare the new state against the commit, so that the tests do not depend
on the file system's ability to record the executable bit, when possible.

The exception is the test for unmerged entries, which does depend on the
file system; we have to skip it.

Signed-off-by: Johannes Sixt <redacted>
---
Am 11/27/2012 22:21, schrieb Junio C Hamano:

It turns out that there are at least two bugs in the diffstat
counting code.  This series comes on top of the earlier 74faaa1 (Fix
"git diff --stat" for interesting - but empty - file changes,
2012-10-17) to fix them.

The tests still fail on Windows. I am not sure whether there is a
difference in comparing the file system against the index or a commit.
If there is, then the updated tests might not test the same thing.

-- Hannes

 t/t4049-diff-stat-count.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t4049-diff-stat-count.sh b/t/t4049-diff-stat-count.sh
index 37f50cd..9e29e71 100755
--- a/t/t4049-diff-stat-count.sh
+++ b/t/t4049-diff-stat-count.sh

@@ -15,7 +15,7 @@ test_expect_success 'setup' '
 
 test_expect_success 'limit output to 2 (simple)' '
 	git reset --hard &&
-	chmod +x c d &&
+	test_chmod +x c d &&
 	echo a >a &&
 	echo b >b &&
 	cat >expect <<-\EOF

@@ -24,13 +24,13 @@ test_expect_success 'limit output to 2 (simple)' '
 	 ...
 	 4 files changed, 2 insertions(+)
 	EOF
-	git diff --stat --stat-count=2 >actual &&
+	git diff --stat --stat-count=2 HEAD >actual &&
 	test_i18ncmp expect actual
 '
 
 test_expect_success 'binary changes do not count in lines' '
 	git reset --hard &&
-	chmod +x c d &&
+	test_chmod +x c d &&
 	echo a >a &&
 	echo b >b &&
 	cat "$TEST_DIRECTORY"/test-binary-1.png >d &&

@@ -40,11 +40,11 @@ test_expect_success 'binary changes do not count in lines' '
 	 ...
 	 4 files changed, 2 insertions(+)
 	EOF
-	git diff --stat --stat-count=2 >actual &&
+	git diff --stat --stat-count=2 HEAD >actual &&
 	test_i18ncmp expect actual
 '
 
-test_expect_success 'exclude unmerged entries from total file count' '
+test_expect_success FILEMODE 'exclude unmerged entries from total file count' '
 	git reset --hard &&
 	echo a >a &&
 	echo b >b &&

-- 
1.8.0.1.1524.gaf6675c

Re: [PATCH 0/5] "diff --stat" counting fixes

From: Antoine Pelisse <hidden>
Date: 2016-06-15 22:55:28

Hi Junio,

That does make a lot of sense and I would have indeed missed a couple
of things here.

I've been thinking about that "Unmerged" line quite a lot, and I can't
get myself any good reason to keep it.
Would you mind taking a couple of minutes to make it clear ?

I feel like (but I can obviously be wrong):
1. The info is redundant. When performing a merge, all diffs (without
--staged flag) are unmerged
2. While status shows the line once, while diff shows the diff for the file
once, while diff --shortstat counts the file once, diff --stat shows two
lines for the file.
3. diff --numstat shows two lines for the same file. As a script
writer (I guess that's what it's meant for), I would definitely expect
uniqueness in third column/filenames.

Cheers,
Antoine

On Tue, Nov 27, 2012 at 10:21 PM, Junio C Hamano [off-list ref] wrote:

It turns out that there are at least two bugs in the diffstat
counting code.  This series comes on top of the earlier 74faaa1 (Fix
"git diff --stat" for interesting - but empty - file changes,
2012-10-17) to fix them.

Junio C Hamano (5):
  test: add failing tests for "diff --stat" to t4049
  diff --stat: status of unmodified pair in diff-q is not zero
  diff --stat: use "file" temporary variable to refer to data->files[i]
  diff --stat: move the "total count" logic to the last loop
  diff --stat: do not count "unmerged" entries

 diff.c                     | 49 +++++++++++++++++++++++++---------------------
 t/t4049-diff-stat-count.sh | 46 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 23 deletions(-)

--
1.8.0.1.331.g808d2af

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help