"git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

"git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Matt McCutchen]

A bug report: I noticed that "git diff --ignore-space-change --stat"
lists files with only whitespace differences as having changed with 0
differing lines.  This is inconsistent with the behavior without --
stat, which doesn't list such files at all.  (Same behavior with all
the --ignore*space* flags.)  I can reproduce this with the current
"next", af746e4.  Quick test case:

echo ' ' >test1 && echo '  ' >test2 &&
git diff --stat --no-index --ignore-space-change test1 test2

This caused me some inconvenience in the following scenario: I was
reading a commit diff that had a bulk license change in all files
combined with code changes.  I attempted to revert the bulk license
change locally using "sed" to more easily read the code diff, but my
reversion left some whitespace diffs where the original files had
inconsistent whitespace.  So the diffstat after my reversion was
cluttered with these "0" entries.

Regards,
Matt

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Jeff King]

On Tue, Jan 17, 2017 at 09:01:55PM -0500, Matt McCutchen wrote:

> A bug report: I noticed that "git diff --ignore-space-change --stat"
> lists files with only whitespace differences as having changed with 0
> differing lines.  This is inconsistent with the behavior without --
> stat, which doesn't list such files at all.  (Same behavior with all
> the --ignore*space* flags.)  I can reproduce this with the current
> "next", af746e4.  Quick test case:

Hmm. This is pretty easy to do naively, but the special-casing for
addition/deletion (which I think we _do_ need, and which certainly we
fail t4205 without) makes me feel dirty. I'd worry there are other
cases, too (perhaps renames?). And I also notice that the
binary-diffstat code path just above my changes explicitly creates 0/0
diffstats, but I'm not even sure how one would trigger that.

So I dunno. A sensible rule to me is "iff -p would show a diff header,
then --stat should mention it". I think we'd want to somehow extract the
logic from builtin_diff() and reuse it.

---
diff --git a/diff.c b/diff.c
index e2eb6d66a..57ff5c1dc 100644
--- a/diff.c
+++ b/diff.c
@@ -2105,17 +2105,20 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o
 	gather_dirstat(options, &dir, changed, "", 0);
 }
 
+static void free_diffstat_file(struct diffstat_file *f)
+{
+	if (f->name != f->print_name)
+		free(f->print_name);
+	free(f->name);
+	free(f->from_name);
+	free(f);
+}
+
 static void free_diffstat_info(struct diffstat_t *diffstat)
 {
 	int i;
-	for (i = 0; i < diffstat->nr; i++) {
-		struct diffstat_file *f = diffstat->files[i];
-		if (f->name != f->print_name)
-			free(f->print_name);
-		free(f->name);
-		free(f->from_name);
-		free(f);
-	}
+	for (i = 0; i < diffstat->nr; i++)
+		free_diffstat_file(diffstat->files[i]);
 	free(diffstat->files);
 }
 
@@ -2603,6 +2606,23 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 		if (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
 				  &xpp, &xecfg))
 			die("unable to generate diffstat for %s", one->path);
+
+		/*
+		 * Omit diffstats where nothing changed. Even if
+		 * !same_contents, this might be the case due to ignoring
+		 * whitespace changes, etc.
+		 *
+		 * But note that we special-case additions and deletions,
+		 * as adding an empty file, for example, is still of interest.
+		 */
+		if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
+			struct diffstat_file *file =
+				diffstat->files[diffstat->nr - 1];
+			if (!file->added && !file->deleted) {
+				free_diffstat_file(file);
+				diffstat->nr--;
+			}
+		}
 	}
 
 	diff_free_filespec_data(one);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 289806d0c..2805db411 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -736,7 +736,7 @@ test_expect_success 'checkdiff allows new blank lines' '
 
 cat <<EOF >expect
 EOF
-test_expect_success 'whitespace-only changes not reported' '
+test_expect_success 'whitespace-only changes not reported (diff)' '
 	git reset --hard &&
 	echo >x "hello world" &&
 	git add x &&
@@ -746,6 +746,12 @@ test_expect_success 'whitespace-only changes not reported' '
 	test_cmp expect actual
 '
 
+test_expect_success 'whitespace-only changes not reported (diffstat)' '
+	# reuse state from previous test
+	git diff --stat -b >actual &&
+	test_cmp expect actual
+'
+
 cat <<EOF >expect
 diff --git a/x b/z
 similarity index NUM%

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I dunno. A sensible rule to me is "iff -p would show a diff header,
> then --stat should mention it".

True but tricky (you need a better definition of "a diff header").

In addition to a new and deleted file, does a file whose executable
bit was flipped need mention?  If so, then "diff --git" is the diff
header in the above.  Otherwise "@@ ... @@", iow, "iff -p would show
any hunk".

I think the patch implements the latter, which I think is sensible.

> +		/*
> +		 * Omit diffstats where nothing changed. Even if
> +		 * !same_contents, this might be the case due to ignoring
> +		 * whitespace changes, etc.
> +		 *
> +		 * But note that we special-case additions and deletions,
> +		 * as adding an empty file, for example, is still of interest.
> +		 */
> +		if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
> +			struct diffstat_file *file =
> +				diffstat->files[diffstat->nr - 1];
> +			if (!file->added && !file->deleted) {
> +				free_diffstat_file(file);
> +				diffstat->nr--;
> +			}
> +		}
>  	}

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Jeff King]

On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So I dunno. A sensible rule to me is "iff -p would show a diff header,
> > then --stat should mention it".
> 
> True but tricky (you need a better definition of "a diff header").
> 
> In addition to a new and deleted file, does a file whose executable
> bit was flipped need mention?  If so, then "diff --git" is the diff
> header in the above.  Otherwise "@@ ... @@", iow, "iff -p would show
> any hunk".
> 
> I think the patch implements the latter, which I think is sensible.

I would think the former is more sensible (and is what my patch is
working towards). Doing:

  >empty
  git add empty
  git diff --cached

shows a "diff --git" header, but no hunk. I think it should show a
diffstat (and does with my patch).

I was thinking the rule should be something like:

  if (p->status == DIFF_STATUS_MODIFIED &&
      !file->added && !file->deleted))

and otherwise include the entry, since it would be an add, delete,
rename, etc, which carries useful information.

Though a pure rename would not hit this code path at all, I would think
(it would not trigger "!same_contents"). And a rename where there was a
whitespace only change probably _should_ be omitted from "-b".

Ditto for a pure mode change, I think. We do not run the contents
through diff at all, so it does not hit this code path.

-Peff

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > So I dunno. A sensible rule to me is "iff -p would show a diff header,
>> > then --stat should mention it".
>> 
>> True but tricky (you need a better definition of "a diff header").
>> 
>> In addition to a new and deleted file, does a file whose executable
>> bit was flipped need mention?  If so, then "diff --git" is the diff
>> header in the above.  Otherwise "@@ ... @@", iow, "iff -p would show
>> any hunk".
>> 
>> I think the patch implements the latter, which I think is sensible.
>
> I would think the former is more sensible (and is what my patch is
> working towards).

Doh (yes, "diff --git" was what I meant).  As a mode-flipping patch
does not have hunk but does show the header, it wants to be included
in "git diff --stat" output, I would think, independent of -b issue.
In fact

	chmod +x README.md
	git diff --stat

does show a 0-line diffstat.


> Doing:
>
>   >empty
>   git add empty
>   git diff --cached
>
> shows a "diff --git" header, but no hunk. I think it should show a
> diffstat (and does with my patch).
>
> I was thinking the rule should be something like:
>
>   if (p->status == DIFF_STATUS_MODIFIED &&
>       !file->added && !file->deleted))
>
> and otherwise include the entry, since it would be an add, delete,
> rename, etc, which carries useful information.
>
> Though a pure rename would not hit this code path at all, I would think
> (it would not trigger "!same_contents"). And a rename where there was a
> whitespace only change probably _should_ be omitted from "-b".
>
> Ditto for a pure mode change, I think. We do not run the contents
> through diff at all, so it does not hit this code path.
>
> -Peff